[chromium] Rename visibleLayerRect to visibleContentRect as it lives in content space
Created attachment 151378 [details] Patch
Comment on attachment 151378 [details] Patch Attachment 151378 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13160632
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 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.
Created attachment 151486 [details] Patch
Created attachment 151488 [details] Patch
Comment on attachment 151488 [details] Patch Is it too much to ask to change opaqueRegionInLayerRect and updateLayerRect as well?
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.
(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.
Created attachment 151503 [details] Patch
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 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
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 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.
Created attachment 151540 [details] Patch
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?
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 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 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?
Created attachment 151731 [details] Patch for landing
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
Created attachment 151733 [details] Patch for landing
Comment on attachment 151733 [details] Patch for landing Clearing flags on attachment: 151733 Committed r122373: <http://trac.webkit.org/changeset/122373>
All reviewed patches have been landed. Closing bug.
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.
(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.