[chromium] Ensure compositing layers are up to date before entering doComposite
Created attachment 90639 [details] Patch
My testcase for this is extremely flaky and timing-depending, but the bad case is roughly that at the start of WebViewImpl::composite() we have a composited RenderLayer inside an iframe with a dirty z order list. When we call paintContents() on that layer, it lazily updates the z order list and re-evaluates the compositing requirements on that RenderLayerCompositor which can change the GraphicsLayer tree. This patch avoids the issue by explicitly updating compositing layers for every FrameView (which is 1:1 with RenderLayerCompositor) in the patch, syncing the compositing state for all FrameViews, and then not pumping the event loop until after all the paintContents() calls complete. I've also added some assertions to make it easier to detect this case in debug builds.
Comment on attachment 90639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90639&action=review Looks good to me. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:64 > +#ifndef NDEBUG > +bool LayerRendererChromium::s_inPaintContents = false; > +#endif Barf!
(In reply to comment #3) > (From update of attachment 90639 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90639&action=review > > Looks good to me. > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:64 > > +#ifndef NDEBUG > > +bool LayerRendererChromium::s_inPaintContents = false; > > +#endif > > Barf! Do you know of another way to do this?
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 90639 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=90639&action=review > > > > Looks good to me. > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:64 > > > +#ifndef NDEBUG > > > +bool LayerRendererChromium::s_inPaintContents = false; > > > +#endif > > > > Barf! > > Do you know of another way to do this? Minimally I'd scope the variable inside the LayerRendererChromium instance. But if the debugger makes it difficult to examine it in that case, don't worry about it.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 90639 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=90639&action=review > > > > > > Looks good to me. > > > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:64 > > > > +#ifndef NDEBUG > > > > +bool LayerRendererChromium::s_inPaintContents = false; > > > > +#endif > > > > > > Barf! > > > > Do you know of another way to do this? > > Minimally I'd scope the variable inside the LayerRendererChromium instance. But if the debugger makes it difficult to examine it in that case, don't worry about it. LayerChromiums are constructed without a LayerRendererChromium pointer, so that doesn't trivially work.
Committed r85112: <http://trac.webkit.org/changeset/85112>
http://trac.webkit.org/changeset/85112 might have broken Leopard Intel Debug (Build) and SnowLeopard Intel Release (Build)
Reverted r85112 for reason: Broke mac compile Committed r85116: <http://trac.webkit.org/changeset/85116>
FrameView.h is included from files in the WebKit project on Mac, so adding a header to FrameView.h requires adding that header to the forwarding headers. Given that CompositingUpdateAfterLayoutOrStyleChange and CompositingUpdateOnPaintingOrHitTest in practice do exactly the same thing I think I'll just not add the parameter to FrameView::updateCompositingLayers()
Created attachment 91377 [details] Patch
Take two - this patch only modifies chromium-specific code.
http://trac.webkit.org/changeset/85116 might have broken SnowLeopard Intel Release (Tests)
Comment on attachment 91377 [details] Patch Looks fine to me.
Comment on attachment 91377 [details] Patch Clearing flags on attachment: 91377 Committed r85136: <http://trac.webkit.org/changeset/85136>
All reviewed patches have been landed. Closing bug.