Bug 90843 - [chromium] Rename layerRect to contentRect for rects that live in content space
Summary: [chromium] Rename layerRect to contentRect for rects that live in content space
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-09 18:24 PDT by Dana Jansens
Modified: 2012-07-11 19:18 PDT (History)
11 users (show)

See Also:


Attachments
Patch (64.58 KB, patch)
2012-07-09 18:26 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (66.81 KB, patch)
2012-07-10 10:54 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (69.79 KB, patch)
2012-07-10 10:59 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (116.97 KB, patch)
2012-07-10 12:43 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-08 (1.07 MB, application/zip)
2012-07-10 13:39 PDT, WebKit Review Bot
no flags Details
Patch (136.22 KB, patch)
2012-07-10 15:44 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (135.70 KB, patch)
2012-07-11 10:35 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch for landing (136.31 KB, patch)
2012-07-11 11:05 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-07-09 18:24:30 PDT
[chromium] Rename visibleLayerRect to visibleContentRect as it lives in content space
Comment 1 Dana Jansens 2012-07-09 18:26:24 PDT
Created attachment 151378 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-09 18:55:29 PDT
Comment on attachment 151378 [details]
Patch

Attachment 151378 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13160632
Comment 3 Adrienne Walker 2012-07-09 20:37:27 PDT
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?
Comment 4 Dana Jansens 2012-07-10 10:27:11 PDT
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.
Comment 5 Dana Jansens 2012-07-10 10:54:15 PDT
Created attachment 151486 [details]
Patch
Comment 6 Dana Jansens 2012-07-10 10:59:38 PDT
Created attachment 151488 [details]
Patch
Comment 7 Adrienne Walker 2012-07-10 11:41:50 PDT
Comment on attachment 151488 [details]
Patch

Is it too much to ask to change opaqueRegionInLayerRect and updateLayerRect as well?
Comment 8 Dana Jansens 2012-07-10 11:48:59 PDT
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.
Comment 9 Adrienne Walker 2012-07-10 11:54:02 PDT
(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.
Comment 10 Dana Jansens 2012-07-10 12:43:21 PDT
Created attachment 151503 [details]
Patch
Comment 11 Dana Jansens 2012-07-10 12:44:23 PDT
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.
Comment 12 WebKit Review Bot 2012-07-10 13:39:19 PDT
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
Comment 13 WebKit Review Bot 2012-07-10 13:39:22 PDT
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
Comment 14 Dana Jansens 2012-07-10 14:31:32 PDT
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.
Comment 15 Dana Jansens 2012-07-10 15:44:59 PDT
Created attachment 151540 [details]
Patch
Comment 16 Adrienne Walker 2012-07-10 16:07:11 PDT
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?
Comment 17 Dana Jansens 2012-07-10 18:49:19 PDT
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.
Comment 18 Dana Jansens 2012-07-11 09:59:30 PDT
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());
Comment 19 Adrienne Walker 2012-07-11 10:31:53 PDT
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?
Comment 20 Dana Jansens 2012-07-11 10:35:26 PDT
Created attachment 151731 [details]
Patch for landing
Comment 21 WebKit Review Bot 2012-07-11 11:03:47 PDT
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
Comment 22 Dana Jansens 2012-07-11 11:05:31 PDT
Created attachment 151733 [details]
Patch for landing
Comment 23 WebKit Review Bot 2012-07-11 14:39:00 PDT
Comment on attachment 151733 [details]
Patch for landing

Clearing flags on attachment: 151733

Committed r122373: <http://trac.webkit.org/changeset/122373>
Comment 24 WebKit Review Bot 2012-07-11 14:39:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Hayato Ito 2012-07-11 18:19:31 PDT
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.
Comment 26 Alexandre Elias 2012-07-11 19:18:43 PDT
(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.