|Summary:||Fix the semantics of passing contentsVisible flag to GraphicsLayers|
|Product:||WebKit||Reporter:||Shawn Singh <shawnsingh>|
|Component:||Layout and Rendering||Assignee:||Shawn Singh <shawnsingh>|
|Severity:||Normal||CC:||dglazkov, jamesr, kbr, rob, simon.fraser, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:||77038|
Description Shawn Singh 2012-01-24 19:15:16 PST
Summary: The proposed patch is to fix a priority gmail bug in chromium. A reduced test case and a reproducible gmail bug were reported in http://code.google.com/p/chromium/issues/detail?id=107283. Part of this fix was done by http://trac.webkit.org/changeset/105471. It turns out, this only fixed Safari, and not chromium, where the original gmail bug still remained, and the new layout test still fails. Based on my investigations, it seems like one additional change should be made in WebCore, RenderLayerBacking::updateGraphicsLayerGeometry. The Gory Details: Looking closely at the code, I think WebCore had intended to have these semantics: (1) For RenderLayers, the hasContentsVisible flag refers to only that specific layer, independent of any descendant RenderLayers. (2) For GraphicsLayers (which may have many RenderLayers that contribute to it), it seems like the contentsVisible flag should be true if *any* contributing RenderLayers are visible (i.e. including non-compositing descendants) However, in the current state of the code, #2 is not true, the GraphicsLayer has contentsVisible==false if the top RenderLayer is not visible, even if it has visible descendants. As a result, GraphicsLayer::contentsVisible is false, even if that layer does have visible content. Because of this, chromium still fails the layout test compositing/visibility/layer-visible-content.html. A question comes up why Safari passes it - I looked briefly but not completely... perhaps Safari is ignoring the contentsVisible flag. Its possible that we have not come across any cases where Safari needs to pay attention to it. But chromium does use the visibility flag, and therefore fails. It does sound like something for a follow-up bug, but if we can move forward with this proposed change first, that would be helpful for chromium. I tested this proposed change on OS X: I ran layout tests (a) before the proposed fix and (b) after the proposed fix, and I did this for both chromium and CoreAnimation. As best as I could tell (there were a lot of other failures and flakes) this patch introduces no regressions, and only compositing/visibility/layer-visible-content.html (the new test case added by 105471) may need re-baselining, and I manually verified that test case passes on safari and chromium with the fix.
Comment 2 Shawn Singh 2012-01-24 19:32:47 PST
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 3 Simon Fraser (smfr) 2012-01-24 21:42:06 PST
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.
Comment 4 Shawn Singh 2012-01-25 11:53:29 PST
Committed r105906: <http://trac.webkit.org/changeset/105906>
Comment 5 Shawn Singh 2012-01-25 12:47:44 PST
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.
Comment 6 James Robinson 2012-01-27 12:23:58 PST
*** Bug 76716 has been marked as a duplicate of this bug. ***
Comment 8 Kenneth Russell 2012-01-30 18:35:10 PST
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?
Comment 9 James Robinson 2012-01-30 18:51:06 PST
Does this fix compositing/visibility/layer-visible-content.html on chromium now?
Comment 10 WebKit Review Bot 2012-01-30 19:24:39 PST
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
Comment 11 Kenneth Russell 2012-01-31 10:21:25 PST
(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.
Comment 12 Shawn Singh 2012-01-31 14:07:53 PST
(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?
Comment 13 Shawn Singh 2012-01-31 14:34:57 PST
Created attachment 124824 [details] Patch This patch is for landing, pending Simon's approval.
Comment 14 WebKit Review Bot 2012-02-01 02:31:32 PST
Comment on attachment 124824 [details] Patch Clearing flags on attachment: 124824 Committed r106459: <http://trac.webkit.org/changeset/106459>
Comment 15 WebKit Review Bot 2012-02-01 02:31:37 PST
All reviewed patches have been landed. Closing bug.