cc: Move SyncedScrollOffset to scroll tree

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 7f4f92283c6a550b77038a9155b2d395ec231670..e58ec178ccb9fe4c7d3003a66cbc5635e11849c2 100644
--- a/cc/layers/layer_impl.h
+++ b/cc/layers/layer_impl.h
@@ -258,6 +258,7 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   }
 
   LayerTreeImpl* layer_tree_impl() const { return layer_tree_impl_; }
+  LayerListImpl* layer_list_impl() const { return layer_list_impl_; }
 
   void PopulateSharedQuadState(SharedQuadState* state) const;
   void PopulateScaledSharedQuadState(SharedQuadState* state, float scale) const;
@@ -463,17 +464,20 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   gfx::Vector2dF bounds_delta() const { return bounds_delta_; }
 
   void SetCurrentScrollOffset(const gfx::ScrollOffset& scroll_offset);
+  // must only be called by tests when updating scroll offset of a single layer,
+  // the standard process to update scroll offsets is
+  // ScrollTree::UpdateScrollOffsetMap()

[ajuma, 2016/03/01 22:54:20]

Please make this a complete sentence.

[sunxd, 2016/03/02 17:45:34]

Done.

   void PushScrollOffsetFromMainThread(const gfx::ScrollOffset& scroll_offset);

[ajuma, 2016/03/01 22:54:20]

If this is only for tests, it should be called something like
PushScrollOffsetFromMainThreadForTesting.

But are tests that call this still meaningful now? Or should they really be
calling something else that more closely matches reality (or deleted if they're
no longer relevant)?

[sunxd, 2016/03/02 17:45:34]

I think some tests call this function to set the initial value of scroll offset,
but there are still some that want to utilize the synced property feature. Can I
leave it for the next CL? Since this might also be related to the refactoring of
moving scroll offset logic to property trees and the number of callers may make
this CL inconvenient to review.

[ajuma, 2016/03/02 18:43:09]

Refactoring the tests in another CL seems fine. What about renaming this to
PushScrollOffsetFromMainThreadForTesting though? Does that seem reasonable to do
here?

[sunxd, 2016/03/02 20:52:55]

I think the function will probably go away in next CL and it is called by about
70 tests, so it makes sense to me to deal with it then to avoid duplicated
refactoring work.

-  // This method is similar to PushScrollOffsetFromMainThread but will cause the
-  // scroll offset given to clobber any scroll changes on the active tree in the
-  // time until this value is pushed to the active tree.
-  void PushScrollOffsetFromMainThreadAndClobberActiveValue(
-      const gfx::ScrollOffset& scroll_offset);
-  gfx::ScrollOffset PullDeltaForMainThread();
+
   gfx::ScrollOffset CurrentScrollOffset() const;
   gfx::ScrollOffset BaseScrollOffset() const;
   gfx::Vector2dF ScrollDelta() const;
   void SetScrollDelta(const gfx::Vector2dF& delta);
+  // This is a mirror function of Layer::scroll_offset, which is supposed to be
+  // called only by PropertyTreeBuilder
+  gfx::ScrollOffset scroll_offset() const {
+    return synced_scroll_offset()->PendingBase();

[ajuma, 2016/03/01 22:54:20]

Could PropertyTreeBuilder call CurrentScrollOffset instead (for both Layer and
LayerImpl)?

[sunxd, 2016/03/02 17:45:34]

Currently CurrentScrollOffset returns the combined offset, but here we only want
the pending base. I think we can consider this after moving the logic to
property tree in the next CL.

[ajuma, 2016/03/02 18:43:09]

These would only have different values if there are tests that are rebuilding
property trees in a state where the active/pending values are different. Could
we fix the tests instead? (It just seems weird to have LayerImpl::scroll_offset
return the pending base value even if the LayerImpl itself is in the active
tree.)

[sunxd, 2016/03/02 20:52:55]

The unit test that breaks is
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...:
it tries to push scroll offset to a layer_impl, then push the same offset and
verify if the layer properties changed. But the macro used here forces a rebuild
of property tree, the rebuild can take the base+delta as base if I call
CurrentScrollOffset. I'm not sure if fixing the test breaks the purpose of it.

[ajuma, 2016/03/02 22:31:08]

That test isn't really testing anything meaningful now, since it's checking the
behavior of a function we don't call outside of tests. How about changing that
so it only tests that LayerImpl::DidUpdateScrollOffset does trigger
needs_push_properties?

+  }
 
   gfx::ScrollOffset MaxScrollOffset() const;
   gfx::ScrollOffset ClampScrollOffsetToLimits(gfx::ScrollOffset offset) const;
@@ -507,8 +511,6 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
 
   bool user_scrollable(ScrollbarOrientation orientation) const;
 
-  void ApplySentScrollDeltasFromAbortedCommit();
-
   void set_main_thread_scrolling_reasons(
       uint32_t main_thread_scrolling_reasons);
   uint32_t main_thread_scrolling_reasons() const {
@@ -637,7 +639,8 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   }
   void GatherFrameTimingRequestIds(std::vector<int64_t>* request_ids);
 
-  SyncedScrollOffset* synced_scroll_offset() { return scroll_offset_.get(); }
+  const SyncedScrollOffset* synced_scroll_offset() const;
+  SyncedScrollOffset* synced_scroll_offset();
 
   // Get the correct invalidation region instead of conservative Rect
   // for layers that provide it.
@@ -687,6 +690,7 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   }
 
   void NoteLayerPropertyChanged();
+  void DidUpdateScrollOffset();
 
   void PushLayerPropertyChangedForSubtree();
 
@@ -720,8 +724,6 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
  private:
   void ValidateQuadResourcesInternal(DrawQuad* quad) const;
 
-  void PushScrollOffset(const gfx::ScrollOffset* scroll_offset);
-  void DidUpdateScrollOffset();
   void NoteLayerPropertyChangedForDescendantsInternal();
   void PushLayerPropertyChangedForSubtreeInternal();
 
@@ -752,8 +754,6 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver,
   LayerTreeImpl* layer_tree_impl_;
   LayerListImpl* layer_list_impl_;
 
-  // Properties dynamically changeable on active tree.
-  scoped_refptr<SyncedScrollOffset> scroll_offset_;
   gfx::Vector2dF bounds_delta_;
 
   // Properties synchronized from the associated Layer.