This bug focuses on testing that calculateDrawTransformsAndVisibility computes correct transforms. The metabug for all unit tests is https://bugs.webkit.org/show_bug.cgi?id=68942 This first patch is only for review. The tests are complete and working, except for the last set of tests in "verifyTransformsForRenderSurfaceHierarchy"... But the implementation will have the same pattern. Vangelis, could you please review this? Thanks in advance!
Created attachment 109383 [details] Patch
Comment on attachment 109383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109383&action=review Looks good! Thanks for getting the ball rolling on this. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:95 > +void executeCalculateDrawTransformsAndVisibilityWithDummyVariables(LayerChromium * rootLayer) nit: Shortening the name of this function (e.g. getting rid of the WithDummyVariables part) will make the rest of the code somewhat easier to read. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:354 > +TEST(CCLayerTreeHostCommonTest, verifyTransformsForRenderSurfaceHierarchy) Since this test doesn't test anything, it should be removed.
> > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:354 > > +TEST(CCLayerTreeHostCommonTest, verifyTransformsForRenderSurfaceHierarchy) > > Since this test doesn't test anything, it should be removed. Thanks for looking at this. Actually I've already implemented this function (it was related to transforms), and will submit another patch for final review by tonight. ~Shawn
Created attachment 109569 [details] Patch
On my part this code is final and ready for commit. But I'll be happy to make any changes based on reviews. Thanks!
Created attachment 109572 [details] fixed a badly misleading comment
Created attachment 110028 [details] fixed a few more incorrect comments
Hi all, still waiting for final review on this. Thanks!
Comment on attachment 110028 [details] fixed a few more incorrect comments Looks good. Unofficial r+ from me.
Comment on attachment 110028 [details] fixed a few more incorrect comments View in context: https://bugs.webkit.org/attachment.cgi?id=110028&action=review 3 small nits and this is good to go > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:39 > +// NOTE CAREFULLY: > +// > +// In these tests, the relationship between anchor point, position, and bounds is not intuitive. The following two points > +// should help clarify the code: This comment is very nice but I'm not sure why it's in this test file in particular. Should this be on LayerChromium.h instead? I think it's more likely that people trying to understand an interface will look at the header, and not at a comment in a test file. > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:81 > +void setLayerPropertiesForTesting(LayerChromium * layer, const TransformationMatrix& transform, const TransformationMatrix& sublayerTransform, const FloatPoint& anchor, const FloatPoint& position, const IntSize& bounds, bool preserves3D) WebKit style is to put the * with the typename - so for instance "LayerChromium* layer". > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:91 > +void executeCalculateDrawTransformsAndVisibility(LayerChromium * rootLayer) "LayerChromium *" -> "LayerChromium*" > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:161 > + setLayerPropertiesForTesting(layer.get(), identityMatrix, arbitraryTranslation, FloatPoint(0.0f, 0.0f), FloatPoint(0.0f, 0.0f), IntSize(0, 0), false); note: for the future, there's a FloatPoint::zero() convenience function to get a 0,0 FloatPoint, or you can just say FloatPoint()
Comment on attachment 110028 [details] fixed a few more incorrect comments View in context: https://bugs.webkit.org/attachment.cgi?id=110028&action=review >> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:39 >> +// should help clarify the code: > > This comment is very nice but I'm not sure why it's in this test file in particular. Should this be on LayerChromium.h instead? I think it's more likely that people trying to understand an interface will look at the header, and not at a comment in a test file. OK, putting it in the source rather than tests makes sense. But I think the appropriate place for this comment is in LayerTreeHostCommon.cpp, merged with the existing large comment there. How's that? >> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:81 >> +void setLayerPropertiesForTesting(LayerChromium * layer, const TransformationMatrix& transform, const TransformationMatrix& sublayerTransform, const FloatPoint& anchor, const FloatPoint& position, const IntSize& bounds, bool preserves3D) > > WebKit style is to put the * with the typename - so for instance "LayerChromium* layer". oops. I would have expected the webkit style script to catch this... >> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:161 >> + setLayerPropertiesForTesting(layer.get(), identityMatrix, arbitraryTranslation, FloatPoint(0.0f, 0.0f), FloatPoint(0.0f, 0.0f), IntSize(0, 0), false); > > note: for the future, there's a FloatPoint::zero() convenience function to get a 0,0 FloatPoint, or you can just say FloatPoint() OK I'll use zero(). Thanks!
Putting it in CCLayerTreeHostCommon sounds great too!
Created attachment 110174 [details] Patch
For the most part I did not clobber the existing comments, only restructured and extended. But I still think vangelis may want to review those comments in CCLayerTreeHostCommon before this lands. Fixed the other two style issues, and everything else is unchanged since last patch.
Created attachment 110176 [details] Patch
Comment on attachment 110176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110176&action=review Thanks for taking the time to untangle this transformation mess! > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:105 > + // intuitiveLayerPosition.x = position.x - (anchorPoint.x * bounds.width) Since intuitive is somewhat subjective, I think what you need to say here is the top left corner before any transforms are applied. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:110 > + // lower left even though the coordinates the browser gives us I find this somewhat confusing. The only reason we have to do the flip is because in GL the world is upside-down. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:137 > + // Tr[anchor].inverse() translates from the anchor offset back to the layer origin. Is Tr[anchor] == Tr[c] ? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:140 > + // intuitively described as: The way I think about this is that the child transforms are expressed in terms of the top left corner of their parent (and not the center like the draw transform)
I'll submit the tests as a separate patch (the code is exactly the same as what was reviewed already). I'll submit this comment update in CCLayerTreeHostCommon.cpp as a separate patch in a separate bug. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:110 > > + // lower left even though the coordinates the browser gives us > > I find this somewhat confusing. The only reason we have to do the flip is because in GL the world is upside-down. > I hesitated to change existing comments =) So you're already certain about the GL flip, or do I need to dig into it and verify? Then I'll change this comment accordingly. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:137 > > + // Tr[anchor].inverse() translates from the anchor offset back to the layer origin. > > Is Tr[anchor] == Tr[c] ? Tr[c] = "intuitivePositionTrasnform" * Tr[anchor]. This confusion probably exists because I tried to leave existing comments un-changed. But if you're OK with it, I'll redo the terminology so that its more consistent with the new stuff, too. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:140 > > + // intuitively described as: > > The way I think about this is that the child transforms are expressed in terms of the top left corner of their parent (and not the center like the draw transform) OK, I'll try to update this, too.
Created attachment 110223 [details] Patch
Comment on attachment 110223 [details] Patch R=me on the tests
Comment on attachment 110223 [details] Patch Clearing flags on attachment: 110223 Committed r96986: <http://trac.webkit.org/changeset/96986>
All reviewed patches have been landed. Closing bug.