First round of unit tests for CCLayerTreeHostCommon

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

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!

[Shawn Singh]

Created attachment 109383 [details]
Patch

[Vangelis Kokkevis]

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.

[Shawn Singh]

> 
> > 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

[Shawn Singh]

Created attachment 109569 [details]
Patch

[Shawn Singh]

On my part this code is final and ready for commit.  But I'll be happy to make any changes based on reviews.   Thanks!

[Shawn Singh]

Created attachment 109572 [details]
fixed a badly misleading comment

[Shawn Singh]

Created attachment 110028 [details]
fixed a few more incorrect comments

[Shawn Singh]

Hi all, still waiting for final review on this.  Thanks!

[Vangelis Kokkevis]

Comment on attachment 110028 [details]
fixed a few more incorrect comments

Looks good.  Unofficial r+ from me.

[James Robinson]

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()

[Shawn Singh]

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!

[James Robinson]

Putting it in CCLayerTreeHostCommon sounds great too!

[Shawn Singh]

Created attachment 110174 [details]
Patch

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 110176 [details]
Patch

[Vangelis Kokkevis]

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)

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 110223 [details]
Patch

[James Robinson]

Comment on attachment 110223 [details]
Patch

R=me on the tests

[WebKit Review Bot]

Comment on attachment 110223 [details]
Patch

Clearing flags on attachment: 110223

Committed r96986: <http://trac.webkit.org/changeset/96986>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.