Bug 244564

Created attachment 462019 [details] Patch

Comment on attachment 462019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462019&action=review > Source/WebCore/platform/graphics/texmap/TextureMapper.h:86 > + virtual void setDepthRange(float, float) { } Need argument names here. It’s not obvious what the two floats are. This class is mixing float with double. TransformationMatrix uses double, but we also use FloatRect and FloatPoint. It’s not obvious to me what would be best. > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:872 > +static inline TransformationMatrix createProjectionMatrix(const IntSize& size, bool mirrored, float zNear, float zFar) Here we are converting floats to doubles when we put the results of our expression into a TransformationMatrix object. Maybe they should just be doubles from the start? I am not sure this mix of doubles and floats in the existing code is OK. I’d like to see tests that cover this somehow in the future. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:38 > +struct TextureMapperComputeTransformData; I think this struct should be a member of the TextureMapperLayer class. It’s an implementation detail. I suggest naming it TextureMapperLayer::ComputeTransformsData.

Comment on attachment 462019 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462019&action=review >> Source/WebCore/platform/graphics/texmap/TextureMapper.h:86 >> + virtual void setDepthRange(float, float) { } > > Need argument names here. It’s not obvious what the two floats are. > > This class is mixing float with double. TransformationMatrix uses double, but we also use FloatRect and FloatPoint. It’s not obvious to me what would be best. Will add argument names. WebCore is heavily depending on float values, and mixing floats and doubles. I don't think it's a problem to use float here. >> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:872 >> +static inline TransformationMatrix createProjectionMatrix(const IntSize& size, bool mirrored, float zNear, float zFar) > > Here we are converting floats to doubles when we put the results of our expression into a TransformationMatrix object. Maybe they should just be doubles from the start? I am not sure this mix of doubles and floats in the existing code is OK. > > I’d like to see tests that cover this somehow in the future. Those values come from TransformationMatrix::mapPoint. It inputs FloatPoint3D and outputs FloatPoint3D. FloatPoint3D TransformationMatrix::mapPoint(const FloatPoint3D&) I want the double version of FloatPoint3D, and TransformationMatrix::mapPoint should take it. >> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:38 >> +struct TextureMapperComputeTransformData; > > I think this struct should be a member of the TextureMapperLayer class. It’s an implementation detail. I suggest naming it TextureMapperLayer::ComputeTransformsData. Will fix.

Created attachment 462068 [details] Patch

Created attachment 462069 [details] Patch

Comment on attachment 462069 [details] Patch I found out a regression for this page. http://wpt.live/css/css-transforms/perspective-split-by-zero-w.html

Created attachment 462089 [details] test case of w<0 In the test case, the perspective is 500px, and the part of the element is placed at z>500px. Then, w <= 0. It's beyond the infinity.

Created attachment 462090 [details] Patch

This patch introduces new warnings. /app/webkit/Source/WebCore/platform/graphics/texmap/TextureMapper.h:86:39: warning: unused parameter ‘zNear’ [-Wunused-parameter] /app/webkit/Source/WebCore/platform/graphics/texmap/TextureMapper.h:86:53: warning: unused parameter ‘zFar’ [-Wunused-parameter]

Created attachment 462091 [details] Patch

Comment on attachment 462091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462091&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:136 > + return std::numeric_limits<double>::max(); It should be possible to calculate zfar and znear more precisely by intersecting with the surface rect.

<rdar://problem/99633080>

Created attachment 462648 [details] [fast-cq] Patch for landing

Committed 254897@main (922f8f09f0ca): <https://commits.webkit.org/254897@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 462648 [details].