Bug 90843 – [chromium] Rename layerRect to contentRect for rects that live in content space

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Rename visibleLayerRect to visibleContentRect as it lives in content space

[Dana Jansens]

Created attachment 151378 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151378 [details]
Patch

Attachment 151378 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13160632

[Adrienne Walker]

Comment on attachment 151378 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:107
>      // Always call updateLayerRect() but with an empty layer rectangle when
>      // layer doesn't draw contents.
>      if (drawsContent())
> -        layerRect = visibleLayerRect();
> +        layerRect = visibleContentRect();
>  
>      updateLayerRect(updater, layerRect, occlusion);

What about changing the four other incorrect references to "layer" here too?

[Dana Jansens]

Comment on attachment 151378 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:107
>>      updateLayerRect(updater, layerRect, occlusion);
> 
> What about changing the four other incorrect references to "layer" here too?

oh doh, got the ones below not these ones. thanks.

[Dana Jansens]

Created attachment 151486 [details]
Patch

[Dana Jansens]

Created attachment 151488 [details]
Patch

[Adrienne Walker]

Comment on attachment 151488 [details]
Patch

Is it too much to ask to change opaqueRegionInLayerRect and updateLayerRect as well?

[Dana Jansens]

Oh, I was thinking to change methods around in another patch, was just trying to change the visibleLayerRect() specifically here. I can make a broader change if you like.

[Adrienne Walker]

(In reply to comment #8)
> Oh, I was thinking to change methods around in another patch, was just trying to change the visibleLayerRect() specifically here. I can make a broader change if you like.

If you don't mind just changing them all at once, that'd be great.  It's weird to see function names and parameters disagree.

[Dana Jansens]

Created attachment 151503 [details]
Patch

[Dana Jansens]

I got an extra method in CCLayerTilingData as well, and made the use of layerRect() with layerTransform() consistent (vs quadRect() with quadTransform()) when drawing a RenderSurface quad. The layerRect and quadRect are the same in that case, but it uses layerTransform, so prefer the former rect.

[WebKit Review Bot]

Comment on attachment 151503 [details]
Patch

Attachment 151503 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13208073

New failing tests:
compositing/geometry/vertical-scroll-composited.html
compositing/direct-image-compositing.html
compositing/geometry/fixed-position-transform-composited-page-scale.html
platform/chromium/compositing/tiny-layer-rotated.html
platform/chromium/compositing/huge-layer-rotated.html

[WebKit Review Bot]

Created attachment 151508 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Dana Jansens]

Comment on attachment 151503 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:151
> +    IntRect layerRect(IntPoint(), m_bounds);
> +    return CCSharedQuadState::create(quadTransform(), drawTransform(), layerRect, m_scissorRect, drawOpacity(), opaque());

Ah, this changed the AA computation, it maps this rect with quadTransform() etc.. Which isn't right if this is supposed to be layer space.

[Dana Jansens]

Created attachment 151540 [details]
Patch

[Adrienne Walker]

Comment on attachment 151540 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Dropped the layerTransform() from CCSharedQuadState, as nothing should be
> +        using it to draw with. RenderPasses need a weird drawTransform right now
> +        which was stored in layerTransform, so moved this to the RenderPass quad.

What breaks when you replace the quad transform in the shared quad state with the draw transform? Aren't you passing the same content rect to the shared quad state as you are for the quad itself?

[Dana Jansens]

Oh I was going to say culling as it used to use it but now I doubt myself. So good thought.. will double check that.

[Dana Jansens]

Comment on attachment 151540 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        which was stored in layerTransform, so moved this to the RenderPass quad.
> 
> What breaks when you replace the quad transform in the shared quad state with the draw transform? Aren't you passing the same content rect to the shared quad state as you are for the quad itself?

Ah, metrics do use the quadTransform, which is always an origin transform from content to target.

In CCQuadCuller.cpp:
    occlusionTracker.overdrawMetrics().didCullForDrawing(drawQuad->quadTransform(), drawQuad->quadRect(), culledRect);
    occlusionTracker.overdrawMetrics().didDraw(drawQuad->quadTransform(), culledRect, drawQuad->opaqueRect());

[Adrienne Walker]

Comment on attachment 151540 [details]
Patch

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

Given the mess around surfaces and transforms, I think ironing out those wrinkles are going to be a little bit more involved.  let's land this and punt on the surface issue.  R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPassDrawQuad.h:60
> +    WebKit::WebTransformationMatrix m_drawTransform;

Can you add a FIXME here that this should be merged into the quad transform in the shared quad state?

[Dana Jansens]

Created attachment 151731 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151731 [details]
Patch for landing

Rejecting attachment 151731 [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:
/graphics/chromium/LayerRendererChromium.cpp: In member function 'void WebCore::LayerRendererChromium::drawRenderPassQuad(const WebCore::CCRenderPassDrawQuad*)':
Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:613: error: 'const class WebCore::CCRenderPassDrawQuad' has no member named 'layerTransform'
make: *** [out/Debug/obj.target/webcore_chromium_compositor/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.o] Error 1
make: *** Waiting for unfinished jobs....

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

[Dana Jansens]

Created attachment 151733 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 151733 [details]
Patch for landing

Clearing flags on attachment: 151733

Committed r122373: <http://trac.webkit.org/changeset/122373>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hayato Ito]

FYI.
I fixed a build break in http://trac.webkit.org/changeset/122396

I cannot judge whether we should initialize opaque with 'true' or 'false'. Please check it.

[Alexandre Elias]

(In reply to comment #25)
> FYI.
> I fixed a build break in http://trac.webkit.org/changeset/122396
> 
> I cannot judge whether we should initialize opaque with 'true' or 'false'. Please check it.

Thanks.  This was actually introduced in Bug 90582, not this one.  The value 'false' is fine.