Scrollbar fade animation broken on Android regression resolved.

cc/trees/layer_tree_host_impl_unittest.cc

Index: cc/trees/layer_tree_host_impl_unittest.cc
diff --git a/cc/trees/layer_tree_host_impl_unittest.cc b/cc/trees/layer_tree_host_impl_unittest.cc
index d73bf72807aa82371b3e37d916cdf2fa5c093a3d..51204be43a425bcd16a337905813b59d4a3dd564 100644
--- a/cc/trees/layer_tree_host_impl_unittest.cc
+++ b/cc/trees/layer_tree_host_impl_unittest.cc
@@ -686,7 +686,6 @@ TEST_F(LayerTreeHostImplTest, ScrollRootCallsCommitAndRedraw) {
   EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(gfx::Point(0, 10),
                                                       InputHandler::WHEEL));
   host_impl_->ScrollEnd(EndState().get());
-  host_impl_->ClearCurrentlyScrollingLayerForTesting();
   EXPECT_FALSE(host_impl_->IsCurrentlyScrollingLayerAt(gfx::Point(),
                                                        InputHandler::WHEEL));
   EXPECT_TRUE(did_request_redraw_);
@@ -710,7 +709,6 @@ TEST_F(LayerTreeHostImplTest, ScrollActiveOnlyAfterScrollMovement) {
   host_impl_->ScrollBy(UpdateState(gfx::Point(), gfx::Vector2d(0, 10)).get());
   EXPECT_TRUE(host_impl_->IsActivelyScrolling());
   host_impl_->ScrollEnd(EndState().get());
-  host_impl_->ClearCurrentlyScrollingLayerForTesting();
   EXPECT_FALSE(host_impl_->IsActivelyScrolling());
 }
 
@@ -1037,7 +1035,6 @@ TEST_F(LayerTreeHostImplTest, NonFastScrollableRegionBasic) {
   EXPECT_FALSE(host_impl_->IsCurrentlyScrollingLayerAt(
       gfx::Point(25, 25), InputHandler::TOUCHSCREEN));
   host_impl_->ScrollEnd(EndState().get());
-  host_impl_->ClearCurrentlyScrollingLayerForTesting();
   EXPECT_FALSE(host_impl_->IsCurrentlyScrollingLayerAt(
       gfx::Point(75, 75), InputHandler::TOUCHSCREEN));
 
@@ -1050,7 +1047,6 @@ TEST_F(LayerTreeHostImplTest, NonFastScrollableRegionBasic) {
       gfx::Point(75, 75), InputHandler::TOUCHSCREEN));
   host_impl_->ScrollBy(UpdateState(gfx::Point(), gfx::Vector2d(0, 10)).get());
   host_impl_->ScrollEnd(EndState().get());
-  host_impl_->ClearCurrentlyScrollingLayerForTesting();
   EXPECT_FALSE(host_impl_->IsCurrentlyScrollingLayerAt(
       gfx::Point(75, 75), InputHandler::TOUCHSCREEN));
 }
@@ -1113,7 +1109,6 @@ TEST_F(LayerTreeHostImplTest, ScrollHandlerPresent) {
                           InputHandler::TOUCHSCREEN);
   EXPECT_TRUE(host_impl_->scroll_affects_scroll_handler());
   host_impl_->ScrollEnd(EndState().get());
-  host_impl_->ClearCurrentlyScrollingLayerForTesting();
   EXPECT_FALSE(host_impl_->scroll_affects_scroll_handler());
 }
 
@@ -2787,7 +2782,6 @@ class LayerTreeHostImplTestScrollbarAnimation : public LayerTreeHostImplTest {
     EXPECT_TRUE(animation_task_.Equals(base::Closure()));
 
     host_impl_->ScrollEnd(EndState().get());
-    host_impl_->ClearCurrentlyScrollingLayerForTesting();
     EXPECT_FALSE(did_request_next_frame_);
     EXPECT_FALSE(did_request_redraw_);
     if (expecting_animations) {
@@ -2881,7 +2875,6 @@ class LayerTreeHostImplTestScrollbarAnimation : public LayerTreeHostImplTest {
     EXPECT_TRUE(animation_task_.Equals(base::Closure()));
 
     host_impl_->ScrollEnd(EndState().get());
-    host_impl_->ClearCurrentlyScrollingLayerForTesting();
     EXPECT_FALSE(did_request_next_frame_);
     EXPECT_FALSE(did_request_redraw_);
     EXPECT_EQ(base::TimeDelta(), requested_animation_delay_);
@@ -9567,7 +9560,6 @@ TEST_F(LayerTreeHostImplVirtualViewportTest, FlingScrollBubblesToInner) {
     EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollEnd(EndState().get());
-    host_impl_->ClearCurrentlyScrollingLayerForTesting();
     EXPECT_EQ(nullptr, host_impl_->CurrentlyScrollingLayer());
 
     EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
@@ -9593,7 +9585,6 @@ TEST_F(LayerTreeHostImplVirtualViewportTest, FlingScrollBubblesToInner) {
     EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollEnd(EndState().get());
-    host_impl_->ClearCurrentlyScrollingLayerForTesting();
     EXPECT_EQ(nullptr, host_impl_->CurrentlyScrollingLayer());
 
     EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
@@ -9651,7 +9642,6 @@ TEST_F(LayerTreeHostImplVirtualViewportTest,
     outer_expected += scroll_delta;
     inner_expected += scroll_delta;
     host_impl_->ScrollEnd(EndState().get());
-    host_impl_->ClearCurrentlyScrollingLayerForTesting();
     EXPECT_FALSE(host_impl_->IsCurrentlyScrollingLayerAt(
         gfx::Point(), InputHandler::TOUCHSCREEN));
 
@@ -9724,7 +9714,6 @@ TEST_F(LayerTreeHostImplVirtualViewportTest,
             .did_scroll);
     EXPECT_EQ(host_impl_->CurrentlyScrollingLayer(), child_scroll);
     host_impl_->ScrollEnd(EndState().get());
-    host_impl_->ClearCurrentlyScrollingLayerForTesting();
     EXPECT_FALSE(host_impl_->IsCurrentlyScrollingLayerAt(
         gfx::Point(), InputHandler::TOUCHSCREEN));
   }
@@ -10374,7 +10363,6 @@ TEST_F(LayerTreeHostImplTimelinesTest, ScrollAnimatedAborted) {
                                                       InputHandler::WHEEL));
   std::unique_ptr<ScrollState> scroll_state_end = EndState();
   host_impl_->ScrollEnd(scroll_state_end.get());
-  host_impl_->ClearCurrentlyScrollingLayerForTesting();
   EXPECT_FALSE(host_impl_->IsCurrentlyScrollingLayerAt(gfx::Point(),
                                                        InputHandler::WHEEL));
 
@@ -10887,6 +10875,45 @@ TEST_F(LayerTreeHostImplTest, WheelScrollWithPageScaleFactorOnInnerLayer) {
   }
 }
 
+TEST_F(LayerTreeHostImplTest, ScrollbarAnimationEndsAfterScrollEnd) {

[bokan, 2016/10/25 17:29:22]

This tests that the CL fixes the bug, but we also added a new cache/restore
functionality which should also be tested. You could either add a new test and
make sure a fling restores the state properly, or just add to this test (and
change the test name).

[sahel, 2016/10/25 17:53:41]

I've already written tests for latching. In this patch, I changed the latching
logic to restore the state rather than not clearing it.
Isn't it enough that it still passes the latching tests, do I need to write a
test to show that the states are restored properly?

[bokan, 2016/10/25 19:19:22]

Good point, ok, existing tests should be enough.

+  LayerTreeSettings settings = DefaultSettings();
+  settings.scrollbar_fade_delay = base::TimeDelta::FromMilliseconds(500);
+  settings.scrollbar_fade_duration = base::TimeDelta::FromMilliseconds(300);
+  settings.scrollbar_animator = LayerTreeSettings::LINEAR_FADE;
+  CreateHostImpl(settings, CreateCompositorFrameSink());
+  SetupScrollAndContentsLayers(gfx::Size(100, 100));
+  host_impl_->SetViewportSize(gfx::Size(50, 50));
+
+  std::unique_ptr<SolidColorScrollbarLayerImpl> scrollbar =
+      SolidColorScrollbarLayerImpl::Create(host_impl_->active_tree(), 400,
+                                           VERTICAL, 10, 0, false, true);
+  LayerImpl* scroll = host_impl_->active_tree()->OuterViewportScrollLayer();
+  LayerImpl* root = host_impl_->active_tree()->InnerViewportContainerLayer();
+  scrollbar->SetScrollLayerId(scroll->id());
+  root->test_properties()->AddChild(std::move(scrollbar));
+  host_impl_->active_tree()->BuildPropertyTreesForTesting();
+  host_impl_->active_tree()->DidBecomeActive();
+  ScrollbarAnimationController* scrollbar_animation_controller =
+      host_impl_->ScrollbarAnimationControllerForId(scroll->id());
+
+  EXPECT_TRUE(scrollbar_animation_controller);
+
+  EXPECT_FALSE(scrollbar_animation_controller->currently_scrolling());
+  gfx::Vector2dF scroll_delta(0, 5);
+
+  EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
+            host_impl_
+                ->ScrollBegin(BeginState(gfx::Point()).get(),
+                              InputHandler::TOUCHSCREEN)
+                .thread);
+  EXPECT_VECTOR_EQ(gfx::Vector2dF(), scroll->CurrentScrollOffset());
+
+  host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
+  EXPECT_TRUE(scrollbar_animation_controller->currently_scrolling());
+  host_impl_->ScrollEnd(EndState().get());
+  EXPECT_FALSE(scrollbar_animation_controller->currently_scrolling());

[bokan, 2016/10/25 17:29:22]

You're basically testing that we call ClearCurrentlyScrollingLayer at ScrollEnd
right? I would just check host_impl_->CurrentlyScrollingLayer and make sure it's
cleared. That way you don't have to add the testing-only method to the
ScrollbarAnimationController, nor setup any of the scrollbar state above. The
scrollbars not fading out is just a consequence of the fact the current
scrolling layer isn't cleared so it's fine to test the root cause.

[sahel, 2016/10/26 14:11:44]

The name of the ClearCurrentlyScrollingLayer doesn't imply that scrollbar
animations are called through it. The regression happened because I thought
clearing the currently scrolling layer in scrollBegin is enough and it's not
necessary to call it in scrollEnd as well. This unittest shows the dependency of
scrollbar animations to clearing/setting the currently scrolling layer. I think
it helps to prevent such regressions in future. But I can remove it if it
doesn't help.
Checking host_impl_->CurrentlyScrollingLayer()==nullptr is already happening in
several tests.

[bokan, 2016/10/26 14:17:43]

Acknowledged.

+}
+
 class LayerTreeHostImplCountingLostSurfaces : public LayerTreeHostImplTest {
  public:
   LayerTreeHostImplCountingLostSurfaces() : num_lost_surfaces_(0) {}