[webkit-reviews] review granted: [Bug 81363] [chromium] Transform animation state should be inherited from parents : [Attachment 132371] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has granted Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 81363: [chromium] Transform animation state should be inherited from
parents
https://bugs.webkit.org/show_bug.cgi?id=81363

Attachment 132371: Patch
https://bugs.webkit.org/attachment.cgi?id=132371&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=132371&action=review


Looks good in general.	R=me, with nits.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:95
> +    bool targetSurfaceTransformsAreAnimating() const { return
m_targetSurfaceTransformsAreAnimating; }
> +    void setTargetSurfaceTransformsAreAnimating(bool animating) {
m_targetSurfaceTransformsAreAnimating = animating; }
> +    bool screenSpaceTransformsAreAnimating() const { return
m_screenSpaceTransformsAreAnimating; }
> +    void setScreenSpaceTransformsAreAnimating(bool animating) {
m_screenSpaceTransformsAreAnimating = animating; }

Why is screen space transform plural here? Isn't there just one?

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:724
> +   
EXPECT_TRUE(renderSurface2->renderSurface()->targetSurfaceTransformsAreAnimatin
g());

Can you add a test where renderSurface()->targetSurfaceTransformsAreAnimating()
is false?

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:729
> +    // Verify drawTransformsAnimatingInScreen values
> +    //

Can you add a test where screenSpaceTransformIsAnimating ends up being false?