Summary: | Fix the semantics of passing contentsVisible flag to GraphicsLayers | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shawn Singh <shawnsingh> | ||||||||
Component: | Layout and Rendering | Assignee: | Shawn Singh <shawnsingh> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, jamesr, kbr, rob, simon.fraser, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 77038 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Shawn Singh
2012-01-24 19:15:16 PST
Created attachment 123875 [details]
Patch
Simon, could you please review this? It is a follow-up to the original fix you made in http://trac.webkit.org/changeset/105471 Thanks very much in advance =) Comment on attachment 123875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123875&action=review > Source/WebCore/rendering/RenderLayerBacking.cpp:410 > + m_graphicsLayer->setContentsVisible(m_owningLayer->hasVisibleContent() || hasVisibleNonCompositingDescendantLayers()); I think it would be better to use RenderLayer's hasVisibleDescendant() instead of hasVisibleNonCompositingDescendantLayers(), since it's faster. Committed r105906: <http://trac.webkit.org/changeset/105906> Somehow the test case is still not passing on the bots, so we rolled it back. With any luck, it is just a simple minor oversight in my patch... the fix works fine on my machine locally, both layout tests and manual testing. Unfortunately I will be unavailable to fix it for the next few days. If someone else has not taken over by Tuesday next week, I will continue then. *** Bug 76716 has been marked as a duplicate of this bug. *** Created attachment 124652 [details]
Patch
I've built and tested Shawn's patch from http://trac.webkit.org/changeset/105906 in WebKit on Snow Leopard with a full layout test run, and in Chromium with a subsetted layout test run. I don't see any other regressions introduced by this patch so I'm submitting it for review again. Simon, could you please take a look? What were the failures seen on the bots the first time? Does this fix compositing/visibility/layer-visible-content.html on chromium now? Comment on attachment 124652 [details] Patch Attachment 124652 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11372145 New failing tests: compositing/visibility/layer-visible-content.html compositing/visibility/visibility-image-layers-dynamic.html (In reply to comment #9) > Does this fix compositing/visibility/layer-visible-content.html on chromium now? It does locally (on Mac OS), both in DRT and the browser. We don't understand the cr-linux EWS failure; Shawn will continue investigating. (In reply to comment #11) > (In reply to comment #9) > > Does this fix compositing/visibility/layer-visible-content.html on chromium now? > > It does locally (on Mac OS), both in DRT and the browser. We don't understand the cr-linux EWS failure; Shawn will continue investigating. Update: (1) The original patch we tried to land actually did have the correct fix. There was some platform-dependent height in the hidden layer-tree text dump that caused TEXT mismatch failure. (2) Using hasVisibleDescendant() causes compositing/image-layers-dynamic.html to fail. Using hasVisibleNonCompositingDescendantLayers(), the test passes, (tested on osx and linux). Using hasVisibleNonCompositingDescendantLayers() is more correct anyway -- hasVisibleDescendant() could return true for a composited descendant, too, which is not the intended semantics. Simon, can you please confirm its OK with you if we use hasVisibleNonCompositingDescendantLayers() since it performs correctly, even if its slower? If you want, in a separate bug I can create a boolean flag to cache this value for performance? Created attachment 124824 [details]
Patch
This patch is for landing, pending Simon's approval.
Comment on attachment 124824 [details] Patch Clearing flags on attachment: 124824 Committed r106459: <http://trac.webkit.org/changeset/106459> All reviewed patches have been landed. Closing bug. |