Scrollbar fade animation broken on Android regression resolved.

cc/trees/layer_tree_host_impl.cc

Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index 9a6842fd7ca9c44f83171964d264e19cc052c027..079b28bdd71a0fe3d2230a78e6ce374ec11a58bb 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -264,6 +264,9 @@ LayerTreeHostImpl::LayerTreeHostImpl(
   top_controls_manager_ =
       TopControlsManager::Create(this, settings.top_controls_show_threshold,
                                  settings.top_controls_hide_threshold);
+  cached_scrolling_variables_state_ = ScrollingVariablesState(

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

These should all be at their default values anyway so there's no need to
initialize here. We're guaranteed to have a ScrollEnd before a fling anyway,
right?

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

I thought it's better for maintainability of the code, in case the default for
any of these values changes, if it's not the case, I can remove the
initialization.

For touchpad scrolling yes, we get a scroll end and handle the fling by calling
scroll begin. For touch scroll, no, we get the scroll end only when the scroll
event is finished. if the scroll is followed by a fling, we get the scrollend
only after the fling.

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

Ah, that's unfortunate. Ok then, initializing is fine but put this in the
initializer list above.

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

Done.

+      did_lock_scrolling_layer_, scroll_affects_scroll_handler_,
+      accumulated_root_overscroll_);
 }
 
 LayerTreeHostImpl::~LayerTreeHostImpl() {
@@ -2643,49 +2646,55 @@ InputHandler::ScrollStatus LayerTreeHostImpl::ScrollBegin(
   scroll_status.main_thread_scrolling_reasons =
       MainThreadScrollingReason::kNotScrollingOnMain;
   TRACE_EVENT0("cc", "LayerTreeHostImpl::ScrollBegin");
-
+  LayerImpl* scrolling_layer_impl = nullptr;
   // On Mac a scroll begin with |inertial_phase| = true happens to handle a
-  // fling.
-  if (scroll_state->is_in_inertial_phase())
-    return FlingScrollBegin();
-
-  ClearCurrentlyScrollingLayer();
+  // fling. Also, touchpad fling starts are handled by calling ScrollBegin
+  // instead of FlingScrollBegin.
+  if (scroll_state->is_in_inertial_phase()) {
+    scrolling_layer_impl =
+        active_tree_->LayerById(active_tree_->LastScrolledLayerId());

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

I'd prefer if we just store the CurrentlyScrollingLayer's id as part of the
CacheScrollingVariablesState

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

I preferred to use the one that is already cached. Because it makes the struct
smaller, but if you think it's easier to read/follow, I can add it to the
struct.

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

Please do. In general, unless something is known to be performance sensitive, we
should err on the side of readibility/maintainability.

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

Done.

+    RestoreCachedScrollingVariablesState();
+  }
 
-  gfx::Point viewport_point(scroll_state->position_x(),
-                            scroll_state->position_y());
+  if (!scrolling_layer_impl) {
+    ClearCurrentlyScrollingLayer();
 
-  gfx::PointF device_viewport_point = gfx::ScalePoint(
-      gfx::PointF(viewport_point), active_tree_->device_scale_factor());
-  LayerImpl* layer_impl =
-      active_tree_->FindLayerThatIsHitByPoint(device_viewport_point);
+    gfx::Point viewport_point(scroll_state->position_x(),
+                              scroll_state->position_y());
 
-  if (layer_impl) {
-    LayerImpl* scroll_layer_impl =
-        active_tree_->FindFirstScrollingLayerOrScrollbarLayerThatIsHitByPoint(
-            device_viewport_point);
-    if (scroll_layer_impl &&
-        !HasScrollAncestor(layer_impl, scroll_layer_impl)) {
-      scroll_status.thread = SCROLL_UNKNOWN;
-      scroll_status.main_thread_scrolling_reasons =
-          MainThreadScrollingReason::kFailedHitTest;
-      return scroll_status;
+    gfx::PointF device_viewport_point = gfx::ScalePoint(
+        gfx::PointF(viewport_point), active_tree_->device_scale_factor());
+    LayerImpl* layer_impl =
+        active_tree_->FindLayerThatIsHitByPoint(device_viewport_point);
+
+    if (layer_impl) {
+      LayerImpl* scroll_layer_impl =
+          active_tree_->FindFirstScrollingLayerOrScrollbarLayerThatIsHitByPoint(
+              device_viewport_point);
+      if (scroll_layer_impl &&
+          !HasScrollAncestor(layer_impl, scroll_layer_impl)) {
+        scroll_status.thread = SCROLL_UNKNOWN;
+        scroll_status.main_thread_scrolling_reasons =
+            MainThreadScrollingReason::kFailedHitTest;
+        return scroll_status;
+      }
     }
-  }
 
-  bool scroll_on_main_thread = false;
-  LayerImpl* scrolling_layer_impl = FindScrollLayerForDeviceViewportPoint(
-      device_viewport_point, type, layer_impl, &scroll_on_main_thread,
-      &scroll_status.main_thread_scrolling_reasons);
+    bool scroll_on_main_thread = false;
+    scrolling_layer_impl = FindScrollLayerForDeviceViewportPoint(
+        device_viewport_point, type, layer_impl, &scroll_on_main_thread,
+        &scroll_status.main_thread_scrolling_reasons);
 
-  if (scrolling_layer_impl)
-    scroll_affects_scroll_handler_ =
-        scrolling_layer_impl->layer_tree_impl()->have_scroll_event_handlers();
+    if (scrolling_layer_impl)
+      scroll_affects_scroll_handler_ =
+          scrolling_layer_impl->layer_tree_impl()->have_scroll_event_handlers();
 
-  if (scroll_on_main_thread) {
-    RecordCompositorSlowScrollMetric(type, MAIN_THREAD);
+    if (scroll_on_main_thread) {
+      RecordCompositorSlowScrollMetric(type, MAIN_THREAD);
 
-    scroll_status.thread = SCROLL_ON_MAIN_THREAD;
-    return scroll_status;
+      scroll_status.thread = SCROLL_ON_MAIN_THREAD;
+      return scroll_status;
+    }
   }
 
   return ScrollBeginImpl(scroll_state, scrolling_layer_impl, type);

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

Is it intentional that we're now calling ScrollBeginImpl for fling scrolls
rather than FlingScrollBegin? If so, the only other place we call
FlingScrollBegin is InputHandlerProxy::HandleGestureFlingStart (other than
tests) so we should replace all uses.

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

On Mac we don't get a fling start and that's why on other platforms while
scrolling with touchpad a flingStart is handled by scrollBegin.

For touch scroll though, fling is handled by a HandleGestureFlingStart.

I think the correct approach isn't changing the HandleGestureFlingStart. It's
better to call HandleGestureFlingStart on mac as well, instead of scrollBegin,
and make touchpad consistent with touch.

Anyway, I'll do some refactoring later on, to make touch and touchpad scrolls
follow similar/identical code paths. But I prefer to do it, in a separate patch.

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

Ok, sgtm, thanks.

@@ -2725,11 +2734,7 @@ InputHandler::ScrollStatus LayerTreeHostImpl::ScrollAnimatedBegin(
     ScrollStateData scroll_state_end_data;
     scroll_state_end_data.is_ending = true;
     ScrollState scroll_state_end(scroll_state_end_data);
-    // TODO(Sahel): Once the touchpad scroll latching for Non-mac devices is
-    // implemented, the current scrolling layer should not get cleared after
-    // each animation (crbug.com/526463).
     ScrollEnd(&scroll_state_end);
-    ClearCurrentlyScrollingLayer();
   }
   return scroll_status;
 }
@@ -2853,11 +2858,7 @@ InputHandler::ScrollStatus LayerTreeHostImpl::ScrollAnimated(
     }
   }
   scroll_state.set_is_ending(true);
-  // TODO(Sahel): Once the touchpad scroll latching for Non-mac devices is
-  // implemented, the current scrolling layer should not get cleared after
-  // each animation (crbug.com/526463).
   ScrollEnd(&scroll_state);
-  ClearCurrentlyScrollingLayer();
 
   return scroll_status;
 }
@@ -3166,6 +3167,11 @@ void LayerTreeHostImpl::SetSynchronousInputHandlerRootScrollOffset(
 
 void LayerTreeHostImpl::ClearCurrentlyScrollingLayer() {
   active_tree_->ClearCurrentlyScrollingLayer();
+  // Cached scrolling variables state is restored in a ScrollBegin with
+  // inertial phase (fling) to latch it to its corresponding scroll. When
+  // reseting a variable here, update ScrollingVariablesState struct as well
+  // to maintain the proper caching.
+  CacheScrollingVariablesState();
   did_lock_scrolling_layer_ = false;
   scroll_affects_scroll_handler_ = false;
   accumulated_root_overscroll_ = gfx::Vector2dF();
@@ -3178,11 +3184,7 @@ void LayerTreeHostImpl::ScrollEnd(ScrollState* scroll_state) {
   DistributeScrollDelta(scroll_state);
   top_controls_manager_->ScrollEnd();
 
-  if (scroll_state->is_in_inertial_phase()) {
-    // Only clear the currently scrolling layer if we know the scroll is done.
-    // A non-inertial scroll end could be followed by an inertial scroll.
-    ClearCurrentlyScrollingLayer();
-  }
+  ClearCurrentlyScrollingLayer();
 }
 
 InputHandler::ScrollStatus LayerTreeHostImpl::FlingScrollBegin() {
@@ -4035,11 +4037,7 @@ void LayerTreeHostImpl::ScrollOffsetAnimationFinished() {
   // TODO(majidvp): We should pass in the original starting scroll position here
   ScrollStateData scroll_state_data;
   ScrollState scroll_state(scroll_state_data);
-  // TODO(Sahel): Once the touchpad scroll latching for Non-mac devices is
-  // implemented, the current scrolling layer should not get cleared after
-  // each animation (crbug.com/526463).
   ScrollEnd(&scroll_state);
-  ClearCurrentlyScrollingLayer();
 }
 
 gfx::ScrollOffset LayerTreeHostImpl::GetScrollOffsetForAnimation(
@@ -4107,4 +4105,18 @@ void LayerTreeHostImpl::SetWorkerContextVisibility(bool is_visible) {
   }
 }
 
+void LayerTreeHostImpl::CacheScrollingVariablesState() {
+  cached_scrolling_variables_state_ = ScrollingVariablesState(
+      did_lock_scrolling_layer_, scroll_affects_scroll_handler_,
+      accumulated_root_overscroll_);
+}
+void LayerTreeHostImpl::RestoreCachedScrollingVariablesState() {
+  did_lock_scrolling_layer_ =
+      cached_scrolling_variables_state_.didLockScrollingLayer();
+  scroll_affects_scroll_handler_ =
+      cached_scrolling_variables_state_.scrollAffectsScrollHandler();
+  accumulated_root_overscroll_ =
+      cached_scrolling_variables_state_.accumulatedRootOverscroll();
+}
+
 }  // namespace cc