Bug 76975 - Fix the semantics of passing contentsVisible flag to GraphicsLayers
Summary: Fix the semantics of passing contentsVisible flag to GraphicsLayers
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
: 76716 (view as bug list)
Depends on: 77038
  Show dependency treegraph
Reported: 2012-01-24 19:15 PST by Shawn Singh
Modified: 2012-02-01 02:31 PST (History)
6 users (show)

See Also:

Patch (4.18 KB, patch)
2012-01-24 19:31 PST, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (6.15 KB, patch)
2012-01-30 18:32 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2012-01-31 14:34 PST, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-01-24 19:15:16 PST

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 1 Shawn Singh 2012-01-24 19:31:24 PST
Created attachment 123875 [details]
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]

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 7 Kenneth Russell 2012-01-30 18:32:49 PST
Created attachment 124652 [details]
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]

Attachment 124652 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11372145

New failing tests:
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.


(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]

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]

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.