[Android]Optimization of scrollbar animation.

cc/layers/layer_impl.cc

Index: cc/layers/layer_impl.cc
diff --git a/cc/layers/layer_impl.cc b/cc/layers/layer_impl.cc
index 5b39eb0e36262c2299be2fd73522def8ab8c897c..c4ae48085e9c8b28ef35996c51340baf9d4293ea 100644
--- a/cc/layers/layer_impl.cc
+++ b/cc/layers/layer_impl.cc
@@ -1275,18 +1275,25 @@ void LayerImpl::SetScrollbarPosition(ScrollbarLayerImplBase* scrollbar_layer,
     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