Bug 76975 – Fix the semantics of passing contentsVisible flag to GraphicsLayers

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 123875 [details]
Patch

[Shawn Singh]

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 =)

[Simon Fraser (smfr)]

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.

[Shawn Singh]

Committed r105906: <http://trac.webkit.org/changeset/105906>

[Shawn Singh]

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.

[James Robinson]

*** Bug 76716 has been marked as a duplicate of this bug. ***

[Kenneth Russell]

Created attachment 124652 [details]
Patch

[Kenneth Russell]

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?

[James Robinson]

Does this fix compositing/visibility/layer-visible-content.html on chromium now?

[WebKit Review Bot]

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

[Kenneth Russell]

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

[Shawn Singh]

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

[Shawn Singh]

Created attachment 124824 [details]
Patch

This patch is for landing, pending Simon's approval.

[WebKit Review Bot]

Comment on attachment 124824 [details]
Patch

Clearing flags on attachment: 124824

Committed r106459: <http://trac.webkit.org/changeset/106459>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.