Move animation update to follow paint phase for SPv2.

third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp

Index: third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
diff --git a/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp b/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
index 3772c766de4d08a1a18cc4fd521339c4bb7a1939..9e77af835388f2d16378923d90ca4e7ff4f4f84e 100644
--- a/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
+++ b/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
@@ -27,12 +27,14 @@
 
 #include "core/svg/graphics/SVGImage.h"
 
+#include "core/animation/DocumentAnimations.h"
 #include "core/animation/DocumentTimeline.h"
 #include "core/dom/NodeTraversal.h"
 #include "core/dom/shadow/FlatTreeTraversal.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/Settings.h"
+#include "core/layout/LayoutView.h"
 #include "core/layout/svg/LayoutSVGRoot.h"
 #include "core/loader/FrameLoadRequest.h"
 #include "core/paint/FloatClipRecorder.h"
@@ -523,9 +525,26 @@ void SVGImage::serviceAnimations(double monotonicAnimationStartTime) {
   // actually generating painted output, not only for performance reasons,
   // but to preserve correct coherence of the cache of the output with
   // the needsRepaint bits of the PaintLayers in the image.
-  toLocalFrame(m_page->mainFrame())
-      ->view()
-      ->updateAllLifecyclePhasesExceptPaint();
+  FrameView* frameView = toLocalFrame(m_page->mainFrame())->view();
+  frameView->updateAllLifecyclePhasesExceptPaint();
+
+  // For SPv2 we run updateAnimations after the paint phase, but per above
+  // comment we don't want to run lifecycle through to paint for SVG images.
+  // Since we know that SVG images never have composited animations, we can just
+  // update animations directly here without worrying about including
+  // PaintArtifactCompositor analysis of whether animations should be
+  // composited.
+  // TODO(wkorman): I'm not actually sure whether this is kosher. Is it possible

[wkorman, 2017/02/16 23:42:08]

This patch now fixes the SVG image test that was failing, but see my TODO
comment here.

To gain confidence and remove this TODO I believe we'd need to know that all
code paths that involve adding an animation are assured to have moved document
lifecycle past paint before this method in SVGImage is called.

I will look into that, your feedback as add'l checkpoint helpful.

[wkorman, 2017/02/17 19:35:14]

I looked through things and I think this is ok too.

Lingering concern is that if we have multiple SVGImages I believe they each have
their own timer, and so depending on when they fire we could call
updateAnimations() multiple times. But I think that will just cause performance
overhead, which hopefully we'd see in perf benchmarks or UMA or otherwise
realize if it were problematic, and we could find a way to aggregate or
otherwise resolve then.

+  // that a potentially-compositable-animation, call it 'A', is added, but
+  // FrameView hasn't yet run updateAnimations(), and then our SVG animation
+  // timer fires and we run updateAnimations() below, but without the
+  // PAC-provided info we need to potentially decide not to composite 'A'. Then
+  // updateAnimations() will analyze and start all animations, and could
+  // potentially end up compositing something we'd want not to composite.
+  if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
+    DocumentAnimations::updateAnimations(frameView->layoutView()->document(),
+                                         DocumentLifecycle::LayoutClean);
+  }
 }
 
 void SVGImage::advanceAnimationForTesting() {