Bug 91807 – [chromium] Make all compositor screen space transforms operate on content rects

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] Make all compositor screen space transforms operate on content rects

[Adrienne Walker]

Created attachment 153388 [details]
Patch

[Dana Jansens]

Comment on attachment 153388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153388&action=review

This LGTM!

> Source/WebCore/platform/graphics/chromium/cc/CCDebugRectHistory.cpp:77
> +        updateContentRect.scale(layer->contentBounds().width() / static_cast<float>(layer->bounds().width()), layer->contentBounds().height() / static_cast<float>(layer->bounds().height()));

want updateRect in content space again..

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:737
> -        WebTransformationMatrix screenSpaceTransform = layer->screenSpaceTransform();
> -        // FIXME: These should be consistent.
> -        // The layer's screen space transform operates on layer rects, but the surfaces
> -        // screen space transform operates on surface rects, which are in physical pixels,
> -        // so we have to 'undo' the scale here.
> -        screenSpaceTransform.scale(1 / contentsScale);
> -        renderSurface->setScreenSpaceTransform(screenSpaceTransform);
> +        renderSurface->setScreenSpaceTransform(layer->screenSpaceTransform());

yayyy. this makes me super happy.

[Adrienne Walker]

Created attachment 153656 [details]
Rebased onto patch sequence

[Adrienne Walker]

Comment on attachment 153656 [details]
Rebased onto patch sequence

View in context: https://bugs.webkit.org/attachment.cgi?id=153656&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:588
> +        nextHierarchyMatrix.scale(1 / contentsScale);

Other than test changes, this is the difference between this patch and the previous.  Without this, the duplicateChildNonOwner case in CCLayerTreeHostCommonTest.verifyRenderSurfaceTransformsInHighDPI gets a double-scaled screen space matrix.

Since originTransform (i.e. layer->drawTransform) includes a content scale, but nextHierarchyMatrix already included a content scale, then this is a double-scale and we need to undo one of them.  I think this just happened to cancel out previously because screen space transforms were dealing with layer rects.

[Adrienne Walker]

Created attachment 153683 [details]
Rebased

[Adrienne Walker]

I sorted it out.  The 1 / contentsScale line needed to be there because of the change to drawTransform and is now a part of the patch in bug 91417.  Everything now is explained.

This patch is now equivalent to patch 1, except for some extra test changes.

[Dana Jansens]

Comment on attachment 153683 [details]
Rebased

View in context: https://bugs.webkit.org/attachment.cgi?id=153683&action=review

ok still lgtm (with nit similar to that other cl)

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:3688
> +    EXPECT_TRANSFORMATION_MATRIX_EQ(parent->screenSpaceTransform(), expectedParentTransform);

nit: expected result should come first.

[Adrienne Walker]

Created attachment 154392 [details]
Rebased

[Dana Jansens]

@kbr or @senorblanco for review

[Kenneth Russell]

Comment on attachment 154392 [details]
Rebased

rs=me based on danakj's review.

[WebKit Review Bot]

Comment on attachment 154392 [details]
Rebased

Rejecting attachment 154392 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
CLayerTreeHostCommonTest.cpp
Hunk #1 succeeded at 2806 (offset -100 lines).
Hunk #2 succeeded at 3303 (offset -110 lines).
Hunk #3 succeeded at 3319 (offset -110 lines).
Hunk #4 succeeded at 3338 (offset -110 lines).
Hunk #5 succeeded at 3385 (offset -117 lines).
patching file Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kenneth Ru..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13342420

[Adrienne Walker]

Created attachment 154485 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 154485 [details]
Patch for landing

Rejecting attachment 154485 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13342445

[Adrienne Walker]

Committed r123685: <http://trac.webkit.org/changeset/123685>