Suppress layer creation for descendants of GPU-rasterized layers

Suppress layer creation for descendants of GPU-rasterized layers

This suppresses layer creation in descendants (in stacking
order) of layers that are compositing for GPU-rasterization
hints. More specifically, compositing for animations and for
will-change is suppressed in these layers.

BUG=313532
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171908

[Ian Vollick, 2014-04-14 17:02:44]

https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
File LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html
(right):

https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html:29: //
Force an update of the compositing triggers.
This is hacky. If forceCompositingUpdate doesn't work, can we come up with a
cleaner way to do this? E.g., window.internals.settings.setXXX might be the
right place to force the triggers to update?

https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html:35:
testRunner.waitUntilDone();
No need for wait until done (or notify done), I don't think? There are no
timeouts, and the test shouldn't end until the load handler finishes.

[ajuma, 2014-04-14 18:09:33]

PTAL.

https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
File LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html
(right):

https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html:29: //
Force an update of the compositing triggers.
On 2014/04/14 17:02:45, Ian Vollick wrote:
> This is hacky. If forceCompositingUpdate doesn't work, can we come up with a
> cleaner way to do this? E.g., window.internals.settings.setXXX might be the
> right place to force the triggers to update?

Switched this to forceCompositingUpdate. See also
https://codereview.chromium.org/207513002#msg5 for more context about this.

https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html:35:
testRunner.waitUntilDone();
On 2014/04/14 17:02:45, Ian Vollick wrote:
> No need for wait until done (or notify done), I don't think? There are no
> timeouts, and the test shouldn't end until the load handler finishes.

Done.

[Ian Vollick, 2014-04-14 18:18:47]

On 2014/04/14 18:09:33, ajuma wrote:
> PTAL.
> 
>
https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
> File LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html
> (right):
> 
>
https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
> LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html:29: //
> Force an update of the compositing triggers.
> On 2014/04/14 17:02:45, Ian Vollick wrote:
> > This is hacky. If forceCompositingUpdate doesn't work, can we come up with a
> > cleaner way to do this? E.g., window.internals.settings.setXXX might be the
> > right place to force the triggers to update?
> 
> Switched this to forceCompositingUpdate. See also
> https://codereview.chromium.org/207513002#msg5 for more context about this.
> 
>
https://codereview.chromium.org/233063004/diff/190001/LayoutTests/compositing...
> LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html:35:
> testRunner.waitUntilDone();
> On 2014/04/14 17:02:45, Ian Vollick wrote:
> > No need for wait until done (or notify done), I don't think? There are no
> > timeouts, and the test shouldn't end until the load handler finishes.
> 
> Done.

Cool. lgtm.

[Ian Vollick, 2014-04-14 18:18:53]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-04-14 18:19:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/233063004/210001

[commit-bot: I haz the power, 2014-04-14 19:24:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-14 19:24:34]

Failed to apply patch for
Source/core/rendering/compositing/CompositingReasonFinder.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/compositing/CompositingReasonFinder.h
  Hunk #3 FAILED at 50.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/rendering/compositing/CompositingReasonFinder.h.rej

Patch:       Source/core/rendering/compositing/CompositingReasonFinder.h
Index: Source/core/rendering/compositing/CompositingReasonFinder.h
diff --git a/Source/core/rendering/compositing/CompositingReasonFinder.h
b/Source/core/rendering/compositing/CompositingReasonFinder.h
index
c13987cdf5037b7c52c6eb032855bfa4c3e5692c..e03874fce73e1183921165f7bdf8f0d79aae563e
100644
--- a/Source/core/rendering/compositing/CompositingReasonFinder.h
+++ b/Source/core/rendering/compositing/CompositingReasonFinder.h
@@ -20,7 +20,7 @@ class CompositingReasonFinder {
 public:
     explicit CompositingReasonFinder(RenderView&);
 
-    CompositingReasons styleDeterminedReasons(RenderObject*) const;
+    CompositingReasons styleDeterminedReasons(const RenderLayer*) const;
     CompositingReasons directReasons(const RenderLayer*, bool*
needToRecomputeCompositingRequirements) const;
 
     void updateTriggers();
@@ -41,6 +41,8 @@ private:
 
     CompositingReasons nonStyleDeterminedDirectReasons(const RenderLayer*,
bool* needToRecomputeCompositingRequirements) const;
 
+    CompositingReasons suppressStyleDeterminedReasonsIfPossible(const
RenderLayer*, CompositingReasons styleReasons) const;
+
     bool requiresCompositingForAnimation(RenderObject*) const;
     bool requiresCompositingForTransform(RenderObject*) const;
     bool requiresCompositingForBackfaceVisibilityHidden(RenderObject*) const;
@@ -48,7 +50,8 @@ private:
     bool requiresCompositingForOverflowScrollingParent(const RenderLayer*)
const;
     bool requiresCompositingForOutOfFlowClipping(const RenderLayer*) const;
     bool requiresCompositingForOverflowScrolling(const RenderLayer*) const;
-    bool requiresCompositingForWillChange(const RenderObject*) const;
+    bool requiresCompositingForWillChangeCompositingHint(const RenderObject*)
const;
+    bool requiresCompositingForWillChangeGpuRasterizationHint(const
RenderObject*) const;
 
     RenderView& m_renderView;
     CompositingTriggerFlags m_compositingTriggers;

[ajuma, 2014-04-14 19:48:04]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-04-14 19:48:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/233063004/230001

[abarth-chromium, 2014-04-14 20:20:24]

The CQ bit was unchecked by abarth@chromium.org

[abarth-chromium, 2014-04-14 20:25:53]

https://codereview.chromium.org/233063004/diff/230001/LayoutTests/compositing...
File LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html
(right):

https://codereview.chromium.org/233063004/diff/230001/LayoutTests/compositing...
LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html:37: }
Why do we need to force a compositing update here?

https://codereview.chromium.org/233063004/diff/230001/LayoutTests/compositing...
File LayoutTests/compositing/will-change/gpu-rasterization-hint-removed.html
(right):

https://codereview.chromium.org/233063004/diff/230001/LayoutTests/compositing...
LayoutTests/compositing/will-change/gpu-rasterization-hint-removed.html:37: }
Same question

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:508: void
setSuppressingCompositedLayerCreation(bool b) {
m_compositingProperties.suppressingCompositedLayerCreation = b; }
Please add an ASSERT that we're allowed to query compositing properties.

Ideally, the way we'd do this is to access the m_compositingProperties through a
getter and have that getter contain the assert.  If that triggers asserts from
the other places m_compositingProperties is used, please add a FIXME comment to
those places.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:761: bool suppressingCompositedLayerCreation
: 1;
Please move this earlier in the struct so that it packs correctly with the other
bools.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:92:
CompositingReasons
CompositingReasonFinder::suppressStyleDeterminedReasonsIfPossible(const
RenderLayer* layer, CompositingReasons styleReasons) const
Can we give this function a more semantic name?  Suppress which reasons and why?

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:105: 
Please remove this extra blank line.

[abarth-chromium, 2014-04-14 20:29:53]

not lgtm

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:152: directReasons
= suppressStyleDeterminedReasonsIfPossible(layer, directReasons);
Maybe it would be clearer to just inline this function here.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingRequirementsUpdater.cpp:298:
layer->setSuppressingCompositedLayerCreation(currentRecursionData.m_suppressLayerCreation);
Why is this a property of the RenderLayer?  It seems more like a temporary
variable used by this algorithm.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingRequirementsUpdater.cpp:306: if
(needToRecomputeStyleReasons)
There's no such thing as a need to recompute style-determined reasons.  The
style-determined reasons are just that: determined by style.  Maybe it would be
more appropriate to phrase this code in terms of how we interpret those reasons
instead of mutating state that was computed earlier in the pipeline.

[ajuma, 2014-04-15 19:11:33]

PTAL.

https://codereview.chromium.org/233063004/diff/230001/LayoutTests/compositing...
File LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html
(right):

https://codereview.chromium.org/233063004/diff/230001/LayoutTests/compositing...
LayoutTests/compositing/will-change/gpu-rasterization-hint-added.html:37: }
On 2014/04/14 20:25:53, abarth wrote:
> Why do we need to force a compositing update here?

This is to ensure that we've updated compositing with the old value of
will-change before we set its new value (since otherwise, this test wouldn't
really be testing that we get correct behavior when will-change changes (from
"top" to "contents"), it would only be testing the behavior for the new value of
will-change ("contents").

https://codereview.chromium.org/233063004/diff/230001/LayoutTests/compositing...
File LayoutTests/compositing/will-change/gpu-rasterization-hint-removed.html
(right):

https://codereview.chromium.org/233063004/diff/230001/LayoutTests/compositing...
LayoutTests/compositing/will-change/gpu-rasterization-hint-removed.html:37: }
On 2014/04/14 20:25:53, abarth wrote:
> Same question

As in the other test, this is testing what happens when will-change changes, so
we need to make sure the old value of will-change is already reflected in the
current compositing state.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:508: void
setSuppressingCompositedLayerCreation(bool b) {
m_compositingProperties.suppressingCompositedLayerCreation = b; }
On 2014/04/14 20:25:53, abarth wrote:
> Please add an ASSERT that we're allowed to query compositing properties.
> 
> Ideally, the way we'd do this is to access the m_compositingProperties through
a
> getter and have that getter contain the assert.  If that triggers asserts from
> the other places m_compositingProperties is used, please add a FIXME comment
to
> those places.

Added a getter for m_compositingProperties with an assert, and added FIXMEs in
places where this assert is getting tripped.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.h:761: bool suppressingCompositedLayerCreation
: 1;
On 2014/04/14 20:25:53, abarth wrote:
> Please move this earlier in the struct so that it packs correctly with the
other
> bools.

Done.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositingReasonFinder.cpp (right):

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:92:
CompositingReasons
CompositingReasonFinder::suppressStyleDeterminedReasonsIfPossible(const
RenderLayer* layer, CompositingReasons styleReasons) const
On 2014/04/14 20:25:53, abarth wrote:
> Can we give this function a more semantic name?  Suppress which reasons and
why?

Done.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingReasonFinder.cpp:105: 
On 2014/04/14 20:25:53, abarth wrote:
> Please remove this extra blank line.

Done.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
File Source/core/rendering/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingRequirementsUpdater.cpp:298:
layer->setSuppressingCompositedLayerCreation(currentRecursionData.m_suppressLayerCreation);
On 2014/04/14 20:29:53, abarth wrote:
> Why is this a property of the RenderLayer?  It seems more like a temporary
> variable used by this algorithm.

As long as we continue to have incremental compositing updates, we need to know
this elsewhere too (in particular, in the latest patch set, this is used in
RenderLayerCompositor::updateDirectCompositingReasons). If we don't take
suppression into account during incremental updates, we'll think (during the
incremental update) that a non-composited layer needs to become composited even
when it doesn't (because its only compositing reasons are being suppressed);
this will cause RenderLayerCompositor::updateLayerCompositingState to make the
wrong decisions.

https://codereview.chromium.org/233063004/diff/230001/Source/core/rendering/c...
Source/core/rendering/compositing/CompositingRequirementsUpdater.cpp:306: if
(needToRecomputeStyleReasons)
On 2014/04/14 20:29:53, abarth wrote:
> There's no such thing as a need to recompute style-determined reasons.  The
> style-determined reasons are just that: determined by style.  Maybe it would
be
> more appropriate to phrase this code in terms of how we interpret those
reasons
> instead of mutating state that was computed earlier in the pipeline.

Fixed. Instead of mutating style-determined reasons, style reasons are now
stored separately in RenderLayer.

[abarth-chromium, 2014-04-17 17:29:51]

LGTM

I worry there's a minor bug (mentioned below).  It would be good to test that
case to make sure it's not a problem.  We can handle this issue in a follow up
change if you prefer.

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:234: DisableCompositingQueryAsserts
disabler;
Where is the access to composited state?  I don't see it here.

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:1207: return;
This mask argument might not be needed anymore now that we store the
style-determined reasons separately.

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1852: DisableCompositingQueryAsserts
disabler;
This should be asking whether we have any style-determined reasons for
compositing.

Maybe this will cause a bug with your CL because we'll think we have compositing
reasons but then we won't actually create a graphics layer because we'll
suppress the creation of the layer?

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/c...
File Source/core/rendering/compositing/RenderLayerCompositor.cpp (left):

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/c...
Source/core/rendering/compositing/RenderLayerCompositor.cpp:1168:
layer->setCompositingReasons(reasons,
CompositingReasonComboAllStyleDeterminedReasons);
Yeah, I'm pretty sure you can drop the mask argument now.

[ajuma, 2014-04-17 19:24:43]

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:234: DisableCompositingQueryAsserts
disabler;
On 2014/04/17 17:29:51, abarth wrote:
> Where is the access to composited state?  I don't see it here.

It's actually in ~CompositedLayerMapping(). I've moved the disabler there.

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:1207: return;
On 2014/04/17 17:29:51, abarth wrote:
> This mask argument might not be needed anymore now that we store the
> style-determined reasons separately.

RenderLayerCompositor::updateDirectCompositingReasons calls this with mask
CompositingReasonComboAllDirectReasons, so we still need the mask argument for
now.

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/233063004/diff/280001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1852: DisableCompositingQueryAsserts
disabler;
On 2014/04/17 17:29:51, abarth wrote:
> This should be asking whether we have any style-determined reasons for
> compositing.
> 
> Maybe this will cause a bug with your CL because we'll think we have
compositing
> reasons but then we won't actually create a graphics layer because we'll
> suppress the creation of the layer?

Yes, switching to styleDeterminedReasons() does cause a bug, where we fail to
repaint because we think we have a composited layer when we don't have one (test
case: http://jsfiddle.net/c59gC/). I'll switch this to styleDeterminedReasons()
(and fix the resulting bug) in a follow-up.

[abarth-chromium, 2014-04-17 19:52:50]

Thanks!

[ajuma, 2014-04-17 23:58:27]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-04-17 23:58:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/233063004/340001

[commit-bot: I haz the power, 2014-04-18 00:08:29]

Change committed as 171908