Don't always schedule visual update when setting paint invalidation

Only need to schedule visual update when the paint invalidation
needs to be processed in the next document cycle.

Previously in sync painting mode, we schedule visual update infinitely
when an out-of-viewport animated image is set PaintInvalidationDelayedFull
until the image becomes visible (PaintInvalidationDelayedFull upgraded
to PaintInvalidationFull).

BUG=568182,563496
TEST=FrameViewTest.SetPaintInvalidationDuringUpdateAllDocumentLifecycle
TEST=FrameViewTest.SetPaintInvalidationOutOfUpdateAllDocumentLifecycle

Review URL: https://codereview.chromium.org/1512353003

Cr-Commit-Position: refs/heads/master@{#364791}

5 files changed, 92 insertions, 22 deletions

diff --git a/third_party/WebKit/Source/core/core.gypi b/third_party/WebKit/Source/core/core.gypi
index cbcdbe9..bb84f87 100644
--- a/third_party/WebKit/Source/core/core.gypi
+++ b/third_party/WebKit/Source/core/core.gypi
@@ -3877,7 +3877,7 @@
             'fetch/ResourceTest.cpp',
             'fileapi/FileListTest.cpp',
             'fileapi/FileTest.cpp',
-            'frame/FrameViewDidPaintTest.cpp',
+            'frame/FrameViewTest.cpp',
             'frame/ImageBitmapTest.cpp',
             'frame/OriginsUsingFeaturesTest.cpp',
             'frame/RootFrameViewportTest.cpp',
diff --git a/third_party/WebKit/Source/core/frame/FrameView.cpp b/third_party/WebKit/Source/core/frame/FrameView.cpp
index f3aa77d..481ab6e 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameView.cpp
@@ -149,6 +149,7 @@ FrameView::FrameView(LocalFrame* frame)
     , m_viewportIntersectionValid(false)
     , m_hiddenForThrottling(false)
     , m_crossOriginForThrottling(false)
+    , m_isUpdatingAllLifecyclePhases(false)
 {
     ASSERT(m_frame);
     init();
@@ -2363,8 +2364,22 @@ void FrameView::updateLifecycleToLayoutClean()
     frame().localFrameRoot()->view()->updateLifecyclePhasesInternal(OnlyUpToLayoutClean);
 }
 
+void FrameView::scheduleVisualUpdateForPaintInvalidationIfNeeded()
+{
+    LocalFrame* localFrameRoot = frame().localFrameRoot();
+    if (!localFrameRoot->view()->m_isUpdatingAllLifecyclePhases || lifecycle().state() >= DocumentLifecycle::PaintInvalidationClean) {
+        // Schedule visual update to process the paint invalidation in the next cycle.
+        localFrameRoot->scheduleVisualUpdateUnlessThrottled();
+    }
+    // Otherwise the paint invalidation will be handled in paint invalidation phase of this cycle.
+}
+
 void FrameView::updateLifecyclePhasesInternal(LifeCycleUpdateOption phases)
 {
+    Optional<TemporaryChange<bool>> isUpdatingAllLifecyclePhasesScope;
+    if (phases == AllPhases)
+        isUpdatingAllLifecyclePhasesScope.emplace(m_isUpdatingAllLifecyclePhases, true);
+
     // This must be called from the root frame, since it recurses down, not up.
     // Otherwise the lifecycles of the frames might be out of sync.
     ASSERT(m_frame->isLocalRoot());
diff --git a/third_party/WebKit/Source/core/frame/FrameView.h b/third_party/WebKit/Source/core/frame/FrameView.h
index 2bb1247f..58563a2 100644
--- a/third_party/WebKit/Source/core/frame/FrameView.h
+++ b/third_party/WebKit/Source/core/frame/FrameView.h
@@ -240,6 +240,8 @@ public:
     // After calling this method, all frames will be in a lifecycle state >= LayoutClean (unless throttling is allowed).
     void updateLifecycleToLayoutClean();
 
+    void scheduleVisualUpdateForPaintInvalidationIfNeeded();
+
     bool invalidateViewportConstrainedObjects();
 
     void incrementVisuallyNonEmptyCharacterCount(unsigned);
@@ -895,6 +897,8 @@ private:
     // The content clip clips the document (= LayoutView) but not the scrollbars.
     // TODO(trchen): Going away in favor of Settings::rootLayerScrolls too.
     RefPtr<ClipPaintPropertyNode> m_contentClip;
+
+    bool m_isUpdatingAllLifecyclePhases;
 };
 
 inline void FrameView::incrementVisuallyNonEmptyCharacterCount(unsigned count)
diff --git a/third_party/WebKit/Source/core/frame/FrameViewDidPaintTest.cpp b/third_party/WebKit/Source/core/frame/FrameViewTest.cpp
index 5ddc236..332b7c0 100644
--- a/third_party/WebKit/Source/core/frame/FrameViewDidPaintTest.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameViewTest.cpp
@@ -8,6 +8,7 @@
 #include "bindings/core/v8/ExceptionStatePlaceholder.h"
 #include "core/frame/Settings.h"
 #include "core/html/HTMLElement.h"
+#include "core/layout/LayoutObject.h"
 #include "core/loader/EmptyClients.h"
 #include "core/page/Page.h"
 #include "core/testing/DummyPageHolder.h"
@@ -25,12 +26,15 @@
 // compositor).
 
 using testing::_;
+using testing::AnyNumber;
 
 namespace blink {
 namespace {
 
 class MockChromeClient : public EmptyChromeClient {
 public:
+    MockChromeClient() : m_hasScheduledAnimation(false) { }
+
     // ChromeClient
     GraphicsLayerFactory* graphicsLayerFactory() const override
     {
@@ -38,12 +42,45 @@ public:
     }
     MOCK_METHOD1(didPaint, void(const PaintArtifact&));
     MOCK_METHOD2(attachRootGraphicsLayer, void(GraphicsLayer*, LocalFrame* localRoot));
+
+    void scheduleAnimation() { m_hasScheduledAnimation = true; }
+    bool m_hasScheduledAnimation;
 };
 
-class FrameViewPaintTest : public testing::Test {
+class FrameViewTestBase : public testing::Test {
 protected:
-    FrameViewPaintTest()
+    FrameViewTestBase()
         : m_chromeClient(adoptPtrWillBeNoop(new MockChromeClient))
+    { }
+
+    void SetUp() override
+    {
+        Page::PageClients clients;
+        fillWithEmptyClients(clients);
+        clients.chromeClient = m_chromeClient.get();
+        m_pageHolder = DummyPageHolder::create(IntSize(800, 600), &clients);
+        m_pageHolder->page().settings().setAcceleratedCompositingEnabled(true);
+    }
+
+    Document& document() { return m_pageHolder->document(); }
+    MockChromeClient& chromeClient() { return *m_chromeClient; }
+
+private:
+    OwnPtrWillBePersistent<MockChromeClient> m_chromeClient;
+    OwnPtr<DummyPageHolder> m_pageHolder;
+};
+
+class FrameViewTest : public FrameViewTestBase {
+protected:
+    FrameViewTest()
+    {
+        EXPECT_CALL(chromeClient(), attachRootGraphicsLayer(_, _)).Times(AnyNumber());
+    }
+};
+
+class FrameViewSlimmingPaintV2Test : public FrameViewTestBase {
+protected:
+    FrameViewSlimmingPaintV2Test()
     {
         // We shouldn't attach a root graphics layer. In this mode, that's not
         // our responsibility.
@@ -53,11 +90,7 @@ protected:
     void SetUp() override
     {
         RuntimeEnabledFeatures::setSlimmingPaintV2Enabled(true);
-        Page::PageClients clients;
-        fillWithEmptyClients(clients);
-        clients.chromeClient = m_chromeClient.get();
-        m_pageHolder = DummyPageHolder::create(IntSize(800, 600), &clients);
-        m_pageHolder->page().settings().setAcceleratedCompositingEnabled(true);
+        FrameViewTestBase::SetUp();
     }
 
     void TearDown() override
@@ -65,23 +98,18 @@ protected:
         m_featuresBackup.restore();
     }
 
-    Document& document() { return m_pageHolder->document(); }
-    MockChromeClient& chromeClient() { return *m_chromeClient; }
-
 private:
     RuntimeEnabledFeatures::Backup m_featuresBackup;
-    OwnPtrWillBePersistent<MockChromeClient> m_chromeClient;
-    OwnPtr<DummyPageHolder> m_pageHolder;
 };
 
-TEST_F(FrameViewPaintTest, PaintOnce)
+TEST_F(FrameViewSlimmingPaintV2Test, PaintOnce)
 {
     EXPECT_CALL(chromeClient(), didPaint(_));
     document().body()->setInnerHTML("Hello world", ASSERT_NO_EXCEPTION);
     document().view()->updateAllLifecyclePhases();
 }
 
-TEST_F(FrameViewPaintTest, PaintAndRepaint)
+TEST_F(FrameViewSlimmingPaintV2Test, PaintAndRepaint)
 {
     EXPECT_CALL(chromeClient(), didPaint(_)).Times(2);
     document().body()->setInnerHTML("Hello", ASSERT_NO_EXCEPTION);
@@ -90,5 +118,27 @@ TEST_F(FrameViewPaintTest, PaintAndRepaint)
     document().view()->updateAllLifecyclePhases();
 }
 
+TEST_F(FrameViewTest, SetPaintInvalidationDuringUpdateAllLifecyclePhases)
+{
+    document().body()->setInnerHTML("<div id='a' style='color: blue'>A</div>", ASSERT_NO_EXCEPTION);
+    document().view()->updateAllLifecyclePhases();
+    document().getElementById("a")->setAttribute(HTMLNames::styleAttr, "color: green");
+    chromeClient().m_hasScheduledAnimation = false;
+    document().view()->updateAllLifecyclePhases();
+    EXPECT_FALSE(chromeClient().m_hasScheduledAnimation);
+}
+
+TEST_F(FrameViewTest, SetPaintInvalidationOutOfUpdateAllLifecyclePhases)
+{
+    document().body()->setInnerHTML("<div id='a' style='color: blue'>A</div>", ASSERT_NO_EXCEPTION);
+    document().view()->updateAllLifecyclePhases();
+    chromeClient().m_hasScheduledAnimation = false;
+    document().getElementById("a")->layoutObject()->setShouldDoFullPaintInvalidation();
+    EXPECT_TRUE(chromeClient().m_hasScheduledAnimation);
+    chromeClient().m_hasScheduledAnimation = false;
+    document().view()->updateAllLifecyclePhases();
+    EXPECT_FALSE(chromeClient().m_hasScheduledAnimation);
+}
+
 } // namespace
 } // namespace blink
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.cpp b/third_party/WebKit/Source/core/layout/LayoutObject.cpp
index 13f6274..9f80ca5 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.cpp
@@ -3273,16 +3273,17 @@ void LayoutObject::setShouldDoFullPaintInvalidation(PaintInvalidationReason reas
 
     bool isUpgradingDelayedFullToFull = m_bitfields.fullPaintInvalidationReason() == PaintInvalidationDelayedFull && reason != PaintInvalidationDelayedFull;
 
-    if (m_bitfields.fullPaintInvalidationReason() == PaintInvalidationNone || isUpgradingDelayedFullToFull) {
-        if (reason == PaintInvalidationFull)
-            reason = documentLifecycleBasedPaintInvalidationReason(document().lifecycle());
-        m_bitfields.setFullPaintInvalidationReason(reason);
-    }
+    if (m_bitfields.fullPaintInvalidationReason() != PaintInvalidationNone && !isUpgradingDelayedFullToFull)
+        return;
+
+    if (reason == PaintInvalidationFull)
+        reason = documentLifecycleBasedPaintInvalidationReason(document().lifecycle());
+    m_bitfields.setFullPaintInvalidationReason(reason);
 
     if (!isUpgradingDelayedFullToFull) {
         ASSERT(document().lifecycle().state() != DocumentLifecycle::InPaintInvalidation);
-        frame()->scheduleVisualUpdateUnlessThrottled(); // In case that this is called outside of FrameView::updateLayoutAndStyleForPainting().
         markContainerChainForPaintInvalidation();
+        frameView()->scheduleVisualUpdateForPaintInvalidationIfNeeded();
     }
 }
 
@@ -3292,7 +3293,7 @@ void LayoutObject::setMayNeedPaintInvalidation()
         return;
     m_bitfields.setMayNeedPaintInvalidation(true);
     markContainerChainForPaintInvalidation();
-    frame()->scheduleVisualUpdateUnlessThrottled(); // In case that this is called outside of FrameView::updateLayoutAndStyleForPainting().
+    frameView()->scheduleVisualUpdateForPaintInvalidationIfNeeded();
 }
 
 void LayoutObject::clearPaintInvalidationState(const PaintInvalidationState& paintInvalidationState)