Scrollbar fade animation broken on Android regression resolved.

cc/trees/layer_tree_host_impl.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 <stdint.h>
 
 #include <algorithm>
@@ skipping 246 matching lines @@
   active_tree_->property_trees()->is_active = true;
 
   viewport_ = Viewport::Create(this);
 
   TRACE_EVENT_OBJECT_CREATED_WITH_ID(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                                      "cc::LayerTreeHostImpl", id_);
 
   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() {
   DCHECK(task_runner_provider_->IsImplThread());
   TRACE_EVENT0("cc", "LayerTreeHostImpl::~LayerTreeHostImpl()");
   TRACE_EVENT_OBJECT_DELETED_WITH_ID(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                                      "cc::LayerTreeHostImpl", id_);
 
   // It is released before shutdown.
   DCHECK(!compositor_frame_sink_);
@@ skipping 2359 matching lines @@
   return ScrollBeginImpl(scroll_state, viewport()->MainScrollLayer(), type);
 }
 
 InputHandler::ScrollStatus LayerTreeHostImpl::ScrollBegin(
     ScrollState* scroll_state,
     InputHandler::ScrollInputType type) {
   ScrollStatus scroll_status;
   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. 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();
+  }
 
-  // On Mac a scroll begin with |inertial_phase| = true happens to handle a
-  // fling.
-  if (scroll_state->is_in_inertial_phase())
-    return FlingScrollBegin();
+  if (!scrolling_layer_impl) {
+    ClearCurrentlyScrollingLayer();
 
-  ClearCurrentlyScrollingLayer();
+    gfx::Point viewport_point(scroll_state->position_x(),
+                              scroll_state->position_y());
 
-  gfx::Point viewport_point(scroll_state->position_x(),
-                            scroll_state->position_y());
+    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::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;
+      }
+    }
 
-  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;
+    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 (scroll_on_main_thread) {
+      RecordCompositorSlowScrollMetric(type, MAIN_THREAD);
+
+      scroll_status.thread = SCROLL_ON_MAIN_THREAD;
       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);
-
-  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);
-
-    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.

 }
 
 InputHandler::ScrollStatus LayerTreeHostImpl::ScrollAnimatedBegin(
     const gfx::Point& viewport_point) {
   InputHandler::ScrollStatus scroll_status;
   scroll_status.main_thread_scrolling_reasons =
       MainThreadScrollingReason::kNotScrollingOnMain;
   ScrollTree& scroll_tree = active_tree_->property_trees()->scroll_tree;
   ScrollNode* scroll_node = scroll_tree.CurrentlyScrollingNode();
   if (scroll_node) {
@@ skipping 16 matching lines @@
   // ScrollAnimated is used for animated wheel scrolls. We find the first layer
   // that can scroll and set up an animation of its scroll offset. Note that
   // this does not currently go through the scroll customization machinery
   // that ScrollBy uses for non-animated wheel scrolls.
   scroll_status = ScrollBegin(&scroll_state, WHEEL);
   scroll_node = scroll_tree.CurrentlyScrollingNode();
   if (scroll_status.thread == SCROLL_ON_IMPL_THREAD) {
     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;
 }
 
 gfx::Vector2dF LayerTreeHostImpl::ComputeScrollDelta(
     ScrollNode* scroll_node,
     const gfx::Vector2dF& delta) {
   ScrollTree& scroll_tree = active_tree()->property_trees()->scroll_tree;
   float scale_factor = active_tree()->current_page_scale_factor();
 
@@ skipping 103 matching lines @@
         gfx::Vector2dF scroll_delta =
             ComputeScrollDelta(scroll_node, pending_delta);
         if (ScrollAnimationCreate(scroll_node, scroll_delta, delayed_by))
           return scroll_status;
 
         pending_delta -= scroll_delta;
       }
     }
   }
   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;
 }
 
 gfx::Vector2dF LayerTreeHostImpl::ScrollNodeWithViewportSpaceDelta(
     ScrollNode* scroll_node,
     const gfx::PointF& viewport_point,
     const gfx::Vector2dF& viewport_delta,
     ScrollTree* scroll_tree) {
   // Layers with non-invertible screen space transforms should not have passed
@@ skipping 288 matching lines @@
   client_->SetNeedsCommitOnImplThread();
   // After applying the synchronous input handler's scroll offset, tell it what
   // we ended up with.
   UpdateRootLayerStateForSynchronousInputHandler();
   SetFullViewportDamage();
   SetNeedsRedraw();
 }
 
 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();
 }
 
 void LayerTreeHostImpl::ScrollEnd(ScrollState* scroll_state) {
   DCHECK(scroll_state);
   DCHECK(scroll_state->delta_x() == 0 && scroll_state->delta_y() == 0);
 
   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() {
   InputHandler::ScrollStatus scroll_status;
   scroll_status.main_thread_scrolling_reasons =
       MainThreadScrollingReason::kNotScrollingOnMain;
   if (!CurrentlyScrollingLayer()) {
     scroll_status.thread = SCROLL_IGNORED;
     scroll_status.main_thread_scrolling_reasons =
         MainThreadScrollingReason::kNoScrollingLayer;
@@ skipping 832 matching lines @@
     return;
   LayerImpl* layer = tree->LayerByElementId(element_id);
   if (layer)
     layer->OnIsAnimatingChanged(mask, state);
 }
 
 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(
     ElementId element_id) const {
   if (active_tree()) {
     LayerImpl* layer = active_tree()->LayerByElementId(element_id);
     if (layer)
       return layer->ScrollOffsetForAnimation();
   }
 
@@ skipping 47 matching lines @@
   ContextProvider::ScopedContextLock hold(worker_context);
   if (is_visible) {
     worker_context_visibility_ =
         worker_context->CacheController()->ClientBecameVisible();
   } else {
     worker_context->CacheController()->ClientBecameNotVisible(
         std::move(worker_context_visibility_));
   }
 }
 
+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