Scrollbar fade animation broken on Android regression resolved.

cc/trees/layer_tree_host_impl_unittest.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/trees/layer_tree_host_impl.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 668 matching lines @@
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD, status.thread);
   EXPECT_EQ(MainThreadScrollingReason::kNotScrollingOnMain,
             status.main_thread_scrolling_reasons);
 
   EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(gfx::Point(),
                                                       InputHandler::WHEEL));
   host_impl_->ScrollBy(UpdateState(gfx::Point(), gfx::Vector2d(0, 10)).get());
   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_);
   EXPECT_TRUE(did_request_commit_);
 }
 
 TEST_F(LayerTreeHostImplTest, ScrollActiveOnlyAfterScrollMovement) {
   SetupScrollAndContentsLayers(gfx::Size(100, 100));
   host_impl_->active_tree()->BuildPropertyTreesForTesting();
 
   host_impl_->SetViewportSize(gfx::Size(50, 50));
   DrawFrame();
 
   InputHandler::ScrollStatus status = host_impl_->ScrollBegin(
       BeginState(gfx::Point()).get(), InputHandler::WHEEL);
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD, status.thread);
   EXPECT_EQ(MainThreadScrollingReason::kNotScrollingOnMain,
             status.main_thread_scrolling_reasons);
 
   EXPECT_FALSE(host_impl_->IsActivelyScrolling());
   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());
 }
 
 TEST_F(LayerTreeHostImplTest, ScrollWithoutRootLayer) {
   // We should not crash when trying to scroll an empty layer tree.
   InputHandler::ScrollStatus status = host_impl_->ScrollBegin(
       BeginState(gfx::Point()).get(), InputHandler::WHEEL);
   EXPECT_EQ(InputHandler::SCROLL_IGNORED, status.thread);
   EXPECT_EQ(MainThreadScrollingReason::kNoScrollingLayer,
             status.main_thread_scrolling_reasons);
@@ skipping 306 matching lines @@
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD, status.thread);
   EXPECT_EQ(MainThreadScrollingReason::kNotScrollingOnMain,
             status.main_thread_scrolling_reasons);
 
   EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(
       gfx::Point(75, 75), InputHandler::TOUCHSCREEN));
   host_impl_->ScrollBy(UpdateState(gfx::Point(), gfx::Vector2d(0, 10)).get());
   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));
 
   status = host_impl_->ScrollBegin(BeginState(gfx::Point(75, 75)).get(),
                                    InputHandler::TOUCHSCREEN);
   EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD, status.thread);
   EXPECT_EQ(MainThreadScrollingReason::kNotScrollingOnMain,
             status.main_thread_scrolling_reasons);
   EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(
       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));
 }
 
 TEST_F(LayerTreeHostImplTest, NonFastScrollableRegionWithOffset) {
   SetupScrollAndContentsLayers(gfx::Size(200, 200));
   host_impl_->SetViewportSize(gfx::Size(100, 100));
 
   LayerImpl* root = *host_impl_->active_tree()->begin();
   root->SetNonFastScrollableRegion(gfx::Rect(0, 0, 50, 50));
@@ skipping 42 matching lines @@
   SetupScrollAndContentsLayers(gfx::Size(200, 200));
   host_impl_->active_tree()->set_have_scroll_event_handlers(true);
   host_impl_->SetViewportSize(gfx::Size(50, 50));
   DrawFrame();
 
   EXPECT_FALSE(host_impl_->scroll_affects_scroll_handler());
   host_impl_->ScrollBegin(BeginState(gfx::Point()).get(),
                           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());
 }
 
 TEST_F(LayerTreeHostImplTest, ScrollByReturnsCorrectValue) {
   SetupScrollAndContentsLayers(gfx::Size(200, 200));
   host_impl_->SetViewportSize(gfx::Size(100, 100));
 
   DrawFrame();
 
   InputHandler::ScrollStatus status = host_impl_->ScrollBegin(
@@ skipping 1653 matching lines @@
     host_impl_->ScrollBegin(BeginState(gfx::Point()).get(),
                             InputHandler::WHEEL);
     host_impl_->ScrollBy(UpdateState(gfx::Point(), gfx::Vector2dF(0, 5)).get());
     EXPECT_FALSE(did_request_next_frame_);
     EXPECT_TRUE(did_request_redraw_);
     did_request_redraw_ = false;
     EXPECT_EQ(base::TimeDelta(), requested_animation_delay_);
     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) {
       EXPECT_EQ(base::TimeDelta::FromMilliseconds(20),
                 requested_animation_delay_);
       EXPECT_FALSE(animation_task_.Equals(base::Closure()));
     } else {
       EXPECT_EQ(base::TimeDelta(), requested_animation_delay_);
       EXPECT_TRUE(animation_task_.Equals(base::Closure()));
     }
@@ skipping 73 matching lines @@
     host_impl_->ScrollBegin(BeginState(gfx::Point()).get(),
                             InputHandler::WHEEL);
     host_impl_->ScrollBy(UpdateState(gfx::Point(), gfx::Vector2dF(5, 0)).get());
     EXPECT_FALSE(did_request_next_frame_);
     EXPECT_TRUE(did_request_redraw_);
     did_request_redraw_ = false;
     EXPECT_EQ(base::TimeDelta(), requested_animation_delay_);
     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_);
     EXPECT_TRUE(animation_task_.Equals(base::Closure()));
 
     // Changing page scale triggers scrollbar animation.
     host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 4.f);
     host_impl_->active_tree()->SetPageScaleOnActiveTree(1.1f);
     EXPECT_FALSE(did_request_next_frame_);
     EXPECT_FALSE(did_request_redraw_);
@@ skipping 6665 matching lines @@
               host_impl_->FlingScrollBegin().thread);
     EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     gfx::Vector2d scroll_delta(inner_viewport.width() / 2.f,
                                inner_viewport.height() / 2.f);
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
     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());
     EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());
 
     // Fling past the inner viewport boundry, make sure outer viewport scrolls.
     EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
               host_impl_
                   ->ScrollBegin(BeginState(gfx::Point()).get(),
                                 InputHandler::TOUCHSCREEN)
                   .thread);
     EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
     EXPECT_EQ(InputHandler::SCROLL_ON_IMPL_THREAD,
               host_impl_->FlingScrollBegin().thread);
     EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     inner_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
     EXPECT_EQ(outer_scroll, host_impl_->CurrentlyScrollingLayer());
 
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     outer_expected += gfx::Vector2dF(scroll_delta.x(), scroll_delta.y());
     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());
     EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());
   }
 }
 
 TEST_F(LayerTreeHostImplVirtualViewportTest,
        DiagonalScrollBubblesPerfectlyToInner) {
   gfx::Size content_size = gfx::Size(200, 320);
@@ skipping 37 matching lines @@
     // Now diagonal scroll across the outer viewport boundary in a single event.
     // The entirety of the scroll should be consumed, as bubbling between inner
     // and outer viewport layers is perfect.
     host_impl_->ScrollBy(
         UpdateState(gfx::Point(), gfx::ScaleVector2d(scroll_delta, 2)).get());
     EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(
         gfx::Point(), InputHandler::TOUCHSCREEN));
     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));
 
     EXPECT_VECTOR_EQ(inner_expected, inner_scroll->CurrentScrollOffset());
     EXPECT_VECTOR_EQ(outer_expected, outer_scroll->CurrentScrollOffset());
   }
 }
 
 TEST_F(LayerTreeHostImplVirtualViewportTest,
        TouchFlingDoesntSwitchScrollingLayer) {
@@ skipping 52 matching lines @@
     EXPECT_TRUE(
         ScrollInfoContains(*scroll_info, child_scroll->id(), scroll_delta));
     ExpectNone(*scroll_info, inner_scroll->id());
 
     // As the locked layer is at its limit, no further scrolling can occur.
     EXPECT_FALSE(
         host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get())
             .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));
   }
 }
 
 TEST_F(LayerTreeHostImplVirtualViewportTest,
        ScrollBeginEventThatTargetsViewportLayerSkipsHitTest) {
   gfx::Size content_size = gfx::Size(100, 160);
   gfx::Size outer_viewport = gfx::Size(50, 80);
   gfx::Size inner_viewport = gfx::Size(25, 40);
@@ skipping 629 matching lines @@
                                     InputHandler::WHEEL)
                 .thread);
   EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(gfx::Point(0, y),
                                                       InputHandler::WHEEL));
   host_impl_->ScrollBy(
       UpdateState(gfx::Point(0, y), gfx::Vector2d(0, 50)).get());
   EXPECT_TRUE(host_impl_->IsCurrentlyScrollingLayerAt(gfx::Point(0, y + 50),
                                                       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));
 
   // The instant scroll should have marked the smooth scroll animation as
   // aborted.
   EXPECT_FALSE(host_impl_->animation_host()->HasActiveAnimationForTesting(
       scrolling_layer->element_id()));
 
   EXPECT_VECTOR2DF_EQ(gfx::ScrollOffset(0, y + 50),
                       scrolling_layer->CurrentScrollOffset());
@@ skipping 492 matching lines @@
                   .thread);
     EXPECT_VECTOR_EQ(gfx::Vector2dF(), scroll_layer->CurrentScrollOffset());
 
     host_impl_->ScrollBy(UpdateState(gfx::Point(), scroll_delta).get());
     host_impl_->ScrollEnd(EndState().get());
     EXPECT_VECTOR_EQ(gfx::Vector2dF(0, 2.5),
                      scroll_layer->CurrentScrollOffset());
   }
 }
 
+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) {}
   void DidLoseCompositorFrameSinkOnImplThread() override {
     num_lost_surfaces_++;
   }
 
  protected:
   int num_lost_surfaces_;
 };
@@ skipping 573 matching lines @@
   EXPECT_TRUE(host_impl_->use_gpu_rasterization());
 
   // Re-initialize with a software output surface.
   compositor_frame_sink_ = FakeCompositorFrameSink::CreateSoftware();
   host_impl_->InitializeRenderer(compositor_frame_sink_.get());
   EXPECT_FALSE(host_impl_->use_gpu_rasterization());
 }
 
 }  // namespace
 }  // namespace cc