[SPv2] Decomposite otherwise-compositable animations that paint nothing.

third_party/WebKit/Source/core/frame/FrameView.cpp

Index: third_party/WebKit/Source/core/frame/FrameView.cpp
diff --git a/third_party/WebKit/Source/core/frame/FrameView.cpp b/third_party/WebKit/Source/core/frame/FrameView.cpp
index a81c4447504eac6862e5716524517fc60d42c3dc..9511ed7f8c050412b7b021f062df9b2c9257a215 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameView.cpp
@@ -3102,8 +3102,11 @@ void FrameView::updateLifecyclePhasesInternal(
           RuntimeEnabledFeatures::printBrowserEnabled())
         paintTree();
 
-      if (RuntimeEnabledFeatures::slimmingPaintV2Enabled())
-        pushPaintArtifactToCompositor();
+      Optional<CompositorElementIdSet> compositedAnimationElementIds;
+      if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
+        compositedAnimationElementIds.emplace();

[pdr., 2017/03/16 21:20:05]

Is "animation" correct here? Are we returning composited animation element ids
or just a list of all element ids with composited effects?

[wkorman, 2017/03/16 21:53:31]

Name needs improvement, agreed, let's come up with something better.

This is populated with the set of compositor element ids for all cc layers
created that had a compositor element id set on *either* the transform or effect
node.

Perhaps compositedLayerElementIds? I hate long names. compositedElementIds?

[pdr., 2017/03/16 22:16:01]

compositedElementIds SGTM

short is beautiful

+        pushPaintArtifactToCompositor(compositedAnimationElementIds.value());

[pdr., 2017/03/16 21:20:05]

Idea for not needing to pass this Optional around so much:
It looks like the Document has access to all Animations through the
AnimationTimeline it owns. Could we add an ElementId to Animation and then add a
function call here to
"suppressNonCompositedAnimations(compositedAnimationElementIds)". This function
would iterate through all Animations and suppress any where the ElementId is not
in compositedAnimationElementIds. Animation::canStartAnimationOnCompositor then
returns false for any suppressed animation.

[wkorman, 2017/03/16 21:53:31]

I like this general idea. How do you envision marking an animation as
"suppressed"? Add a bool to Animation? Some angst at that idea given:

1. how much complexity is already around whether an animation is compositable,
see my diagram and notes at http://crbug.com/692310#c7

2. the lifecycle of an animation, which can include being cancelled and such,
and the 'suppressed' bool would only be relevant when the animation is first
coming into existence, but would then hang around thereafter taking up space and
adding risk of bugs, potential misunderstanding/misuse by other eng, etc.

[pdr., 2017/03/16 22:16:01]

I was thinking about re-using setEffectSuppressed but you're right that that
isn't a great approach. Would a new bool be so bad? We could completely control
it (e.g.,
animation.setCompositorDeclinedAnimationResponsibility(!compositedElementIds.contains(animation.elementId)))
and include a little comment describing the use. I agree that it would add
complexity to the overall lifecycle, but isn't a bool just making explicit what
is implicitly happening when canStartAnimationOnCompositor returns false due to
compositing?

[wkorman, 2017/03/16 22:32:25]

Yes, but the overhead of an omnipresent bool field is easier to rationalize
when it's expected to be reused for more of the lifetime of the containing
object.

In this case we actually have the needed data from the very start
of the required code path (after call into PAC::update through the entire
code path that handles starting any composited animations), and we know
(well, we currently believe that) we'll never need to reference that data
again after the animation is started.

Having talked through it, it still feels to me more like we should just
pass it down, but I can be convinced otherwise. It's one bit on each
animation and we've already got similar bits like m_effectSuppressed
(I forgot about that one). Yeah, repurposing that seems risky as it's
intended to be used by the inspector (which is not documented anywhere
on the method or field, apparent only through code search, alas).

+      }
 
       DCHECK(!view.hasPendingSelection());
       DCHECK((m_frame->document()->printing() &&
@@ -3112,7 +3115,8 @@ void FrameView::updateLifecyclePhasesInternal(
 
       if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
         DocumentAnimations::updateAnimations(layoutView()->document(),

[pdr., 2017/03/16 21:20:05]

Can this call be moved right after pushPaintArtifactToCompositor to avoid
needing an Optional outside the SPV2 codepath?

[wkorman, 2017/03/16 21:53:31]

Yes, though I think we'd want to duplicate the two DCHECKs above within that
block, and put the ones here in an 'else' block for non-SPv2, since we want them
to precede the call to updateAnimations. Still seems worth moving it as you
suggest to avoid constructing Optional for non-SPv2.

[pdr., 2017/03/16 22:16:01]

Do the checks actually need to come before updateAnimations? I thought not, but
if they do then I could go either way (my original suggestion, or your original
code).

-                                             DocumentLifecycle::PaintClean);
+                                             DocumentLifecycle::PaintClean,
+                                             compositedAnimationElementIds);
       }
     }
 
@@ -3251,7 +3255,8 @@ void FrameView::paintGraphicsLayerRecursively(GraphicsLayer* graphicsLayer) {
     paintGraphicsLayerRecursively(child);
 }
 
-void FrameView::pushPaintArtifactToCompositor() {
+void FrameView::pushPaintArtifactToCompositor(
+    CompositorElementIdSet& compositedAnimationElementIds) {
   TRACE_EVENT0("blink", "FrameView::pushPaintArtifactToCompositor");
 
   DCHECK(RuntimeEnabledFeatures::slimmingPaintV2Enabled());
@@ -3272,7 +3277,8 @@ void FrameView::pushPaintArtifactToCompositor() {
   m_paintArtifactCompositor->update(
       m_paintController->paintArtifact(),
       m_paintController->paintChunksRasterInvalidationTrackingMap(),
-      m_isStoringCompositedLayerDebugInfo, *m_geometryMapper);
+      m_isStoringCompositedLayerDebugInfo, *m_geometryMapper,
+      compositedAnimationElementIds);
 }
 
 std::unique_ptr<JSONObject> FrameView::compositedLayersAsJSON(