[Android]Optimization of scrollbar animation.

cc/layers/layer_impl.cc

 // Copyright 2012 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/layers/layer_impl.h"
 
 #include "base/debug/trace_event.h"
 #include "base/debug/trace_event_argument.h"
 #include "base/json/json_reader.h"
 #include "base/strings/stringprintf.h"
@@ skipping 1257 matching lines @@
   }
   // TODO(wjmaclean) Once we move to a model where the two-viewport model is
   // turned on in all builds, remove the next two lines. For now however, the
   // page scale layer may coincide with the clip layer, and so this is
   // necessary.
   if (page_scale_layer == scrollbar_clip_layer) {
     scroll_rect.Scale(layer_tree_impl()->total_page_scale_factor());
     current_offset.Scale(layer_tree_impl()->total_page_scale_factor());
   }
 
+  float vertical_adjust = scrollbar_clip_layer->bounds_delta().y();

[bokan, 2014/09/15 17:29:55]

I think this should only affect the vertical scrollbar (we vertically adjust the
bounds delta when top controls shrink the viewport).

+  float current_pos = (scrollbar_layer->orientation() == HORIZONTAL)

[weiliangc, 2014/09/15 17:14:37]

nit: Prefer one if statement than three ternary operator that checks the same
condition.

[bokan, 2014/09/15 17:29:55]

Agreed.

[MuVen, 2014/09/15 17:48:42]

Done.

+                          ? current_offset.x()
+                          : current_offset.y();
+  int maximum = (scrollbar_layer->orientation() == HORIZONTAL)
+                    ? (scroll_rect.width() - clip_rect.width())
+                    : (scroll_rect.height() - clip_rect.height());
+  float visible_ratio = (scrollbar_layer->orientation() == HORIZONTAL)

[bokan, 2014/09/15 17:29:55]

Do we need this? I think if the maximum changes the visible ratio must change as
well, no?

[MuVen, 2014/09/15 17:48:42]

if maximum changes, visible ration also changes as well.
On 2014/09/15 17:29:55, bokan wrote:

+                            ? (clip_rect.width() / scroll_rect.width())
+                            : (clip_rect.height() / scroll_rect.height());
+
+  if (!scrollbar_layer->ScrollbarNeedsUpdate(

[weiliangc, 2014/09/15 17:14:37]

As per comment in scrollbar_layer_impl_base file, no need to write a new
function. Setters functions already check whether values have changed.

[MuVen, 2014/09/15 17:48:41]

Yes i agree weiliangc, Setter functions are already checking whether values have
changed.
On 2014/09/15 17:14:37, weiliangc wrote:

+          vertical_adjust, current_pos, maximum, visible_ratio))
+    return;
+
   scrollbar_layer->SetVerticalAdjust(scrollbar_clip_layer->bounds_delta().y());
-  if (scrollbar_layer->orientation() == HORIZONTAL) {
-    float visible_ratio = clip_rect.width() / scroll_rect.width();
-    scrollbar_layer->SetCurrentPos(current_offset.x());
-    scrollbar_layer->SetMaximum(scroll_rect.width() - clip_rect.width());
-    scrollbar_layer->SetVisibleToTotalLengthRatio(visible_ratio);
-  } else {
-    float visible_ratio = clip_rect.height() / scroll_rect.height();
-    scrollbar_layer->SetCurrentPos(current_offset.y());
-    scrollbar_layer->SetMaximum(scroll_rect.height() - clip_rect.height());
-    scrollbar_layer->SetVisibleToTotalLengthRatio(visible_ratio);
-  }
+  scrollbar_layer->SetCurrentPos(current_pos);
+  scrollbar_layer->SetMaximum(maximum);
+  scrollbar_layer->SetVisibleToTotalLengthRatio(visible_ratio);
 
   layer_tree_impl()->set_needs_update_draw_properties();

[weiliangc, 2014/09/15 17:14:37]

The four Setter function seems to call this line when value changes, maybe we
don't need this line any more?

[bokan, 2014/09/15 17:29:55]

I think his idea is that we need to skip the DidScrollUpdate call below so that
we don't schedule an animation on the scrollbar. That said, I'd prefer you
remove the method and just do the check inline. ScrollbarNeedsUpdate is vague
and the helper method isn't saving us any duplication or readibility.

[MuVen, 2014/09/15 17:48:42]

Yes bokan, Main idea is to skip DidScrollUpdate call below so that we dont
schedule an animation on the scrollbar. To do inline check i think we need to
have a bool variable set when CurrentPos or Maximum or visible ratio has new
value.For Which Before calling setters of CurrentPos or Maximum or visible ratio
have to reset the bool variable to false and check it inline.

On 2014/09/15 17:29:55, bokan wrote:

[MuVen, 2014/09/15 17:48:42]

Done.

[MuVen, 2014/09/15 17:57:07]

or may be can i use LayerPropertyChanged() ??

[danakj, 2014/09/15 18:04:56]

That's not the same thing. When something changes that will affect
transforms/opacity/positions/sizes/clips/etc...
set_needs_update_draw_properties() needs to happen. If helper methods do then,
then that's fine.

   // TODO(wjmaclean) The scrollbar animator for the pinch-zoom scrollbars should
   // activate for every scroll on the main frame, not just the scrolls that move
   // the pinch virtual viewport (i.e. trigger from either inner or outer
   // viewport).
   if (scrollbar_animation_controller_) {

[weiliangc, 2014/09/15 17:14:37]

I am not too familiar with pinch-zoom scrollbar behaviors. cc bokan@ who works
on pinch-zoom scrollbars.

     // When both non-overlay and overlay scrollbars are both present, don't
     // animate the overlay scrollbars when page scale factor is at the min.
     // Non-overlay scrollbars also shouldn't trigger animations.
     bool is_animatable_scrollbar =
         scrollbar_layer->is_overlay_scrollbar() &&
         ((layer_tree_impl()->total_page_scale_factor() >
           layer_tree_impl()->min_page_scale_factor()) ||
          !layer_tree_impl()->settings().use_pinch_zoom_scrollbars);
     if (is_animatable_scrollbar)
       scrollbar_animation_controller_->DidScrollUpdate();
@@ skipping 233 matching lines @@
 }
 
 void LayerImpl::NotifyAnimationFinished(
     base::TimeTicks monotonic_time,
     Animation::TargetProperty target_property) {
   if (target_property == Animation::ScrollOffset)
     layer_tree_impl_->InputScrollAnimationFinished();
 }
 
 }  // namespace cc