Refactor delegated scrolling in LayerImpl

cc/layers/layer_impl.h

Index: cc/layers/layer_impl.h
diff --git a/cc/layers/layer_impl.h b/cc/layers/layer_impl.h
index b13dce911a61eeac191d853027b8edfd841c8595..31295d92287816d7db8ac2ea6f23e61c4a9eda96 100644
--- a/cc/layers/layer_impl.h
+++ b/cc/layers/layer_impl.h
@@ -80,14 +80,15 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
                             public LayerAnimationValueProvider,
                             public AnimationDelegate {
  public:
-  // Allows for the ownership of the total scroll offset to be delegated outside
-  // of the layer.
-  class ScrollOffsetDelegate {
+  // An embedder will use ExternalScrollOffsetListener to know when the
+  // LayerImpl's scroll offset has changed. ExternalScrollOffsetListener
+  // carries information of whether external fling is active.
+  class ExternalScrollOffsetListener {

[aelias_OOO_until_Jul13, 2015/04/06 19:47:49]

What do you think about going further and get rid of the listener class
entirely?  LayerImpl can just call layer_tree_impl()->IsExternalFlingActive()
and layer_tree_impl()->DidUpdateScrollOffset(id()).  DidUpdateScrollOffset would
take an int layer_id as argument and LayerTreeImpl would compare it to
InnerViewportScrollLayer()->id() and OuterViewportScrollLayer()->id() to know if
it's the root scroll, and ignore it otherwise.

[hush (inactive), 2015/04/06 23:08:03]

yes. I added DidUpdateScrollOffset and IsExternalFlingActive in LTI.
I also added an extra state in LTI to indicate the root layer scroll offset is
being distributed to the inner/ outer viewport scroll layers and LTI will ignore
DidUpdateScrollOffset in this case.

    public:
-    virtual void SetCurrentScrollOffset(const gfx::ScrollOffset& new_value) = 0;
-    virtual gfx::ScrollOffset GetCurrentScrollOffset() = 0;
     virtual bool IsExternalFlingActive() const = 0;
-    virtual void Update() const = 0;
+    // Tells the embedder to update because the LayerImpl has changed its scroll
+    // offset.
+    virtual void DidUpdateScrollOffset() const = 0;
   };
 
   typedef SyncedProperty<AdditionGroup<gfx::ScrollOffset>> SyncedScrollOffset;
@@ -390,10 +391,10 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   float contents_scale_y() const { return draw_properties_.contents_scale_y; }
   void SetContentsScale(float contents_scale_x, float contents_scale_y);
 
-  void SetScrollOffsetDelegate(ScrollOffsetDelegate* scroll_offset_delegate);
-  void RefreshFromScrollDelegate();
+  void SetExternalScrollOffsetListener(ExternalScrollOffsetListener* listener);
   bool IsExternalFlingActive() const;
 
+  // Visible for testing.

[aelias_OOO_until_Jul13, 2015/04/06 19:47:49]

It's not clear what methods below this comment refers to, and I think
non-test-code could still legitimately call SetCurrentScrollOffset in the
future, so I'd suggest removing the comment.

[hush (inactive), 2015/04/06 23:08:03]

Removed the comment.

   void SetCurrentScrollOffset(const gfx::ScrollOffset& scroll_offset);
   void PushScrollOffsetFromMainThread(const gfx::ScrollOffset& scroll_offset);
   // This method is similar to PushScrollOffsetFromMainThread but will cause the
@@ -672,7 +673,7 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   // Properties synchronized from the associated Layer.
   gfx::Point3F transform_origin_;
   gfx::Size bounds_;
-  ScrollOffsetDelegate* scroll_offset_delegate_;
+  ExternalScrollOffsetListener* scroll_offset_listener_;
   LayerImpl* scroll_clip_layer_;
   bool scrollable_ : 1;
   bool should_scroll_on_main_thread_ : 1;