Takeover MT initiated animations from the compositor

third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp

Index: third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp
diff --git a/third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp b/third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp
index 56f4bd2f2fdaea664973017fa35a5a201da736a3..9fa75f45101d86c424575bf44311d47ef2304c88 100644
--- a/third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp
+++ b/third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp
@@ -94,6 +94,34 @@ private:
     bool m_scrollAnimatorEnabled;
 };
 
+class TestScrollAnimator : public ScrollAnimator {
+public:
+    explicit TestScrollAnimator(ScrollableArea* scrollableArea, WTF::TimeFunction timingFunction = WTF::monotonicallyIncreasingTime)

[jbroman, 2016/03/07 18:38:41]

This is never constructed with the default argument; why have a default?

Also, two-argument constructors need not be explicit.

[ymalik, 2016/03/07 21:26:07]

Done.

+        : ScrollAnimator(scrollableArea, timingFunction) {};
+    ~TestScrollAnimator() override {};
+
+    void setShouldSendToCompositor(bool send)
+    {
+        m_shouldSendToCompositor = send;
+    }
+
+    bool sendAnimationToCompositor() override
+    {
+        if (m_shouldSendToCompositor) {
+            m_runState = ScrollAnimatorCompositorCoordinator::RunState::RunningOnCompositor;
+            m_compositorAnimationId = 1;
+            return true;
+        }
+        return false;
+    }
+
+protected:
+    void abortAnimation() override {};

[jbroman, 2016/03/07 18:38:41]

nit: superfluous semicolon

+
+private:
+    bool m_shouldSendToCompositor = false;
+};
+
 } // namespace
 
 static void reset(ScrollAnimator& scrollAnimator)
@@ -276,6 +304,57 @@ TEST(ScrollAnimatorTest, AnimatedScrollAborted)
     reset(*scrollAnimator);
 }
 
+// Test that a smooth scroll offset animation running on the compositor is
+// completed on the main thread.
+TEST(ScrollAnimatorTest, AnimatedScrollTakeover)
+{
+    OwnPtrWillBeRawPtr<MockScrollableArea> scrollableArea =
+        MockScrollableArea::create(true);
+    OwnPtrWillBeRawPtr<TestScrollAnimator> scrollAnimator = adoptPtrWillBeNoop(
+        new TestScrollAnimator(scrollableArea.get(), getMockedTime));
+
+    EXPECT_CALL(*scrollableArea, minimumScrollPosition()).Times(AtLeast(1))
+        .WillRepeatedly(Return(IntPoint()));
+    EXPECT_CALL(*scrollableArea, maximumScrollPosition()).Times(AtLeast(1))
+        .WillRepeatedly(Return(IntPoint(1000, 1000)));
+    EXPECT_CALL(*scrollableArea, setScrollOffset(_, _)).Times(3);

[jbroman, 2016/03/07 18:38:41]

It's not entirely obvious to me why we should expect three setScrollOffsets and
four registerForAnimations. Is this a meaningful expectation, or does this just
happen to be the number of calls?

[ymalik, 2016/03/07 21:26:07]

Actually setScrollOffset is called 2 times, and registerForAnimation is called 3
times (I missed the failure earlier as apparently the test still passes if the
EXPECT_CALL expectation is unsatisfied). 

The reason for the extra registerForAnimation is that when we are running an
animation on the compositor, we deregister for service because the animation is
serviced by the compositor. Thus, we need to registerForAnimation again if the
animation is cancelled or we need to takeover.

[jbroman, 2016/03/07 21:33:17]

Yeah, you'll have to make sure your mock object is collected (if, as here, it's
garbage-collected). I think tdresser ran into a similar issue recently.
A brief comment explaining where the magic numbers come from (unless it's
obvious to everyone working on ScrollAnimator) would be nice. :)

+    EXPECT_CALL(*scrollableArea, registerForAnimation()).Times(4);
+    EXPECT_CALL(*scrollableArea, scheduleAnimation()).Times(AtLeast(1))
+        .WillRepeatedly(Return(true));
+
+    EXPECT_FALSE(scrollAnimator->hasAnimationThatRequiresService());
+
+    // Smooth scroll.
+    ScrollResult result = scrollAnimator->userScroll(ScrollByLine, FloatSize(100, 0));
+    EXPECT_TRUE(scrollAnimator->hasAnimationThatRequiresService());
+    EXPECT_TRUE(result.didScrollX);
+    EXPECT_FLOAT_EQ(0.0, result.unusedScrollDeltaX);
+    EXPECT_TRUE(scrollAnimator->hasRunningAnimation());
+
+    // Update compositor animation.
+    gMockedTime += 0.05;
+    scrollAnimator->setShouldSendToCompositor(true);
+    scrollAnimator->updateCompositorAnimations();
+    EXPECT_EQ(scrollAnimator->m_runState,
+        ScrollAnimatorCompositorCoordinator::RunState::RunningOnCompositor);
+
+    // Takeover.
+    scrollAnimator->takeoverCompositorAnimation();
+    EXPECT_EQ(scrollAnimator->m_runState,
+        ScrollAnimatorCompositorCoordinator::RunState::RunningOnCompositorButNeedsTakeover);
+
+    // Animation should now be running on the main thread.
+    scrollAnimator->setShouldSendToCompositor(false);
+    scrollAnimator->updateCompositorAnimations();
+    EXPECT_EQ(scrollAnimator->m_runState,
+        ScrollAnimatorCompositorCoordinator::RunState::RunningOnMainThread);
+    scrollAnimator->tickAnimation(getMockedTime());
+    EXPECT_NE(100, scrollAnimator->currentPosition().x());
+    EXPECT_NE(0, scrollAnimator->currentPosition().x());
+    EXPECT_EQ(0, scrollAnimator->currentPosition().y());
+    reset(*scrollAnimator);
+}
+
 TEST(ScrollAnimatorTest, Disabled)
 {
     OwnPtrWillBeRawPtr<MockScrollableArea> scrollableArea = MockScrollableArea::create(false);