cc: Move SyncedScrollOffset to scroll tree

cc: Move SyncedScrollOffset to scroll tree

Now updating scrolling information on impl side can be independent of layer impl.
There are still some left-over changes corresponding to NoteLayerPropertiesChanged.

SyncedScrollOffset of scrollable layers are now stored in property trees instead of
layer impl. The main thread property tree has one copy while pending and active share
one.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/c36713a0336c00a0971fe33514adc01f4df693fa
Cr-Commit-Position: refs/heads/master@{#379116}

[sunxd, 2016-02-25 20:45:49]

Description was changed from

==========
Move SyncedScrollOffset to scroll tree

BUG=568830
==========

to

==========
Move SyncedScrollOffset to scroll tree

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-03-01 04:20:13]

Description was changed from

==========
Move SyncedScrollOffset to scroll tree

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move SyncedScrollOffset to scroll tree

Now updating scrolling information on impl side can be independent of layer
impl.
There are still some left-over changes corresponding to
NoteLayerPropertiesChanged.

SyncedScrollOffset of scrollable layers are now stored in property trees instead
of
layer impl. The main thread property tree has one copy while pending and active
share
one.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-03-01 18:42:46]

sunxd@chromium.org changed reviewers:
+ ajuma@chromium.org, enne@chromium.org, jaydasika@chromium.org,
vollick@chromium.org, weiliangc@chromium.org

[sunxd, 2016-03-01 18:42:47]

https://codereview.chromium.org/1736073002/diff/120001/cc/base/synced_property.h
File cc/base/synced_property.h (right):

https://codereview.chromium.org/1736073002/diff/120001/cc/base/synced_propert...
cc/base/synced_property.h:116: bool clobber_active_value() const { return
clobber_active_value_; }
Because we now bulk process scroll offset push from main to pending, we have to
save the clobber flag in the scroll offset map.

[ajuma, 2016-03-01 22:54:21]

This looks great at a high level! There are parts I still need to look at more
closely, but here are some comments from a first pass:

https://codereview.chromium.org/1736073002/diff/140001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/cc.gyp#newcode390
cc/cc.gyp:390: 'proto/synced_property_conversions.h',
This looks weird. Is it a tab instead of spaces?

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:890: ->PushFromMainThread(scroll_offset);
How about adding a SetScrollOffset method to ScrollTree, that takes an id and
scroll offset?

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:1292: const SyncedScrollOffset*
LayerImpl::synced_scroll_offset() const {
Would it make sense (in a follow up CL) to move all the logic that needs to
directly work with a SyncedScrollOffset into the scroll tree, so LayerImpl
doesn't need know about how scroll offsets happen to be stored?

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:469: // ScrollTree::UpdateScrollOffsetMap()
Please make this a complete sentence.

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:470: void PushScrollOffsetFromMainThread(const
gfx::ScrollOffset& scroll_offset);
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)?

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:479: return synced_scroll_offset()->PendingBase();
Could PropertyTreeBuilder call CurrentScrollOffset instead (for both Layer and
LayerImpl)?

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.h:200: root_layer()->layer_list_impl());
Can this use layer_list_ directly (without going through root_layer() or
depending on the existence of root_layer())?

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:42: typedef
SyncedProperty<AdditionGroup<gfx::ScrollOffset>> SyncedScrollOffset;
This typedef also exists in layer_impl.h. Can it go into a common place?

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:577: // synced_scroll_offset is supposed to be called
by Layer/LayerImpl only
Would things break if something else called this?

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

https://codereview.chromium.org/1736073002/diff/140001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/cc.gyp#newcode390
cc/cc.gyp:390: 'proto/synced_property_conversions.h',
On 2016/03/01 22:54:20, ajuma wrote:
> This looks weird. Is it a tab instead of spaces?

Done.

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer.cc#new...
cc/layers/layer.cc:890: ->PushFromMainThread(scroll_offset);
On 2016/03/01 22:54:20, ajuma wrote:
> How about adding a SetScrollOffset method to ScrollTree, that takes an id and
> scroll offset?

Done.

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:1292: const SyncedScrollOffset*
LayerImpl::synced_scroll_offset() const {
On 2016/03/01 22:54:20, ajuma wrote:
> Would it make sense (in a follow up CL) to move all the logic that needs to
> directly work with a SyncedScrollOffset into the scroll tree, so LayerImpl
> doesn't need know about how scroll offsets happen to be stored?

Yeah, it makes sense. But there might be a lot of unit tests depending on these
interfaces. I will check it carefully in next CL.

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:469: // ScrollTree::UpdateScrollOffsetMap()
On 2016/03/01 22:54:20, ajuma wrote:
> Please make this a complete sentence.

Done.

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:470: void PushScrollOffsetFromMainThread(const
gfx::ScrollOffset& scroll_offset);
On 2016/03/01 22:54:20, ajuma wrote:
> 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)?

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.

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:479: return synced_scroll_offset()->PendingBase();
On 2016/03/01 22:54:20, ajuma wrote:
> Could PropertyTreeBuilder call CurrentScrollOffset instead (for both Layer and
> LayerImpl)?

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.

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.h:200: root_layer()->layer_list_impl());
On 2016/03/01 22:54:20, ajuma wrote:
> Can this use layer_list_ directly (without going through root_layer() or
> depending on the existence of root_layer())?

Since the layer_list change is reverted. I can keep using layer_tree_impl here.
So the problem is resolved currently.

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:42: typedef
SyncedProperty<AdditionGroup<gfx::ScrollOffset>> SyncedScrollOffset;
On 2016/03/01 22:54:20, ajuma wrote:
> This typedef also exists in layer_impl.h. Can it go into a common place?

Done.

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:577: // synced_scroll_offset is supposed to be called
by Layer/LayerImpl only
On 2016/03/01 22:54:20, ajuma wrote:
> Would things break if something else called this?

It would not. This is only a logical restriction. Once the scroll offset is
passed from main thread to impl thread, only layers can modify the deltas.

I also make scroll tree itself call this function.

[ajuma, 2016-03-02 18:43:10]

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:470: void PushScrollOffsetFromMainThread(const
gfx::ScrollOffset& scroll_offset);
On 2016/03/02 17:45:34, sunxd wrote:
> On 2016/03/01 22:54:20, ajuma wrote:
> > 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)?
> 
> 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.

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

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:479: return synced_scroll_offset()->PendingBase();
On 2016/03/02 17:45:34, sunxd wrote:
> On 2016/03/01 22:54:20, ajuma wrote:
> > Could PropertyTreeBuilder call CurrentScrollOffset instead (for both Layer
and
> > LayerImpl)?
> 
> 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.

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.)

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:577: // synced_scroll_offset is supposed to be called
by Layer/LayerImpl only
On 2016/03/02 17:45:34, sunxd wrote:
> On 2016/03/01 22:54:20, ajuma wrote:
> > Would things break if something else called this?
> 
> It would not. This is only a logical restriction. Once the scroll offset is
> passed from main thread to impl thread, only layers can modify the deltas.
> 
> I also make scroll tree itself call this function.

Should this be a TODO then? (about removing this)

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer.cc#new...
cc/layers/layer.cc:934: ->PushFromMainThread(scroll_offset);
How about calling ScrollTree::SetScrollOffset here?

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:51: scoped_refptr<SyncedScrollOffset> scroll_offset)
scroll_offset seems to be unused now, so should be removed.

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.cc:1302: currently_scrolling_node_id_ = -1;
The layer_id_to_scroll_offset_map_ is intentionally omitted here, right? Please
add a comment explaining this.

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.h:580: void UpdateScrollOffsetMapEntry(int key,
Can this be private?

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

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:470: void PushScrollOffsetFromMainThread(const
gfx::ScrollOffset& scroll_offset);
On 2016/03/02 18:43:09, ajuma wrote:
> On 2016/03/02 17:45:34, sunxd wrote:
> > On 2016/03/01 22:54:20, ajuma wrote:
> > > 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)?
> > 
> > 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.
> 
> Refactoring the tests in another CL seems fine. What about renaming this to
> PushScrollOffsetFromMainThreadForTesting though? Does that seem reasonable to
do
> here?

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.

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:479: return synced_scroll_offset()->PendingBase();
On 2016/03/02 18:43:09, ajuma wrote:
> On 2016/03/02 17:45:34, sunxd wrote:
> > On 2016/03/01 22:54:20, ajuma wrote:
> > > Could PropertyTreeBuilder call CurrentScrollOffset instead (for both Layer
> and
> > > LayerImpl)?
> > 
> > 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.
> 
> 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.)

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.

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:577: // synced_scroll_offset is supposed to be called
by Layer/LayerImpl only
On 2016/03/02 18:43:09, ajuma wrote:
> On 2016/03/02 17:45:34, sunxd wrote:
> > On 2016/03/01 22:54:20, ajuma wrote:
> > > Would things break if something else called this?
> > 
> > It would not. This is only a logical restriction. Once the scroll offset is
> > passed from main thread to impl thread, only layers can modify the deltas.
> > 
> > I also make scroll tree itself call this function.
> 
> Should this be a TODO then? (about removing this)

I think the prerequisite is that layer/layer_impl no longer manipulates
scroll_offset. I'll add it here.

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:577: // synced_scroll_offset is supposed to be called
by Layer/LayerImpl only
On 2016/03/02 18:43:09, ajuma wrote:
> On 2016/03/02 17:45:34, sunxd wrote:
> > On 2016/03/01 22:54:20, ajuma wrote:
> > > Would things break if something else called this?
> > 
> > It would not. This is only a logical restriction. Once the scroll offset is
> > passed from main thread to impl thread, only layers can modify the deltas.
> > 
> > I also make scroll tree itself call this function.
> 
> Should this be a TODO then? (about removing this)

Done.

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer.cc#new...
cc/layers/layer.cc:934: ->PushFromMainThread(scroll_offset);
On 2016/03/02 18:43:09, ajuma wrote:
> How about calling ScrollTree::SetScrollOffset here?

Done.

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:51: scoped_refptr<SyncedScrollOffset> scroll_offset)
On 2016/03/02 18:43:09, ajuma wrote:
> scroll_offset seems to be unused now, so should be removed.

Done.

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.cc:1302: currently_scrolling_node_id_ = -1;
On 2016/03/02 18:43:10, ajuma wrote:
> The layer_id_to_scroll_offset_map_ is intentionally omitted here, right?
Please
> add a comment explaining this.

Done.

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.h:580: void UpdateScrollOffsetMapEntry(int key,
On 2016/03/02 18:43:10, ajuma wrote:
> Can this be private?

Done.

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

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:470: void PushScrollOffsetFromMainThread(const
gfx::ScrollOffset& scroll_offset);
On 2016/03/02 18:43:09, ajuma wrote:
> On 2016/03/02 17:45:34, sunxd wrote:
> > On 2016/03/01 22:54:20, ajuma wrote:
> > > 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)?
> > 
> > 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.
> 
> Refactoring the tests in another CL seems fine. What about renaming this to
> PushScrollOffsetFromMainThreadForTesting though? Does that seem reasonable to
do
> here?

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.

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:479: return synced_scroll_offset()->PendingBase();
On 2016/03/02 18:43:09, ajuma wrote:
> On 2016/03/02 17:45:34, sunxd wrote:
> > On 2016/03/01 22:54:20, ajuma wrote:
> > > Could PropertyTreeBuilder call CurrentScrollOffset instead (for both Layer
> and
> > > LayerImpl)?
> > 
> > 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.
> 
> 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.)

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.

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:577: // synced_scroll_offset is supposed to be called
by Layer/LayerImpl only
On 2016/03/02 18:43:09, ajuma wrote:
> On 2016/03/02 17:45:34, sunxd wrote:
> > On 2016/03/01 22:54:20, ajuma wrote:
> > > Would things break if something else called this?
> > 
> > It would not. This is only a logical restriction. Once the scroll offset is
> > passed from main thread to impl thread, only layers can modify the deltas.
> > 
> > I also make scroll tree itself call this function.
> 
> Should this be a TODO then? (about removing this)

I think the prerequisite is that layer/layer_impl no longer manipulates
scroll_offset. I'll add it here.

https://codereview.chromium.org/1736073002/diff/140001/cc/trees/property_tree...
cc/trees/property_tree.h:577: // synced_scroll_offset is supposed to be called
by Layer/LayerImpl only
On 2016/03/02 18:43:09, ajuma wrote:
> On 2016/03/02 17:45:34, sunxd wrote:
> > On 2016/03/01 22:54:20, ajuma wrote:
> > > Would things break if something else called this?
> > 
> > It would not. This is only a logical restriction. Once the scroll offset is
> > passed from main thread to impl thread, only layers can modify the deltas.
> > 
> > I also make scroll tree itself call this function.
> 
> Should this be a TODO then? (about removing this)

Done.

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer.cc#new...
cc/layers/layer.cc:934: ->PushFromMainThread(scroll_offset);
On 2016/03/02 18:43:09, ajuma wrote:
> How about calling ScrollTree::SetScrollOffset here?

Done.

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:51: scoped_refptr<SyncedScrollOffset> scroll_offset)
On 2016/03/02 18:43:09, ajuma wrote:
> scroll_offset seems to be unused now, so should be removed.

Done.

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.cc:1302: currently_scrolling_node_id_ = -1;
On 2016/03/02 18:43:10, ajuma wrote:
> The layer_id_to_scroll_offset_map_ is intentionally omitted here, right?
Please
> add a comment explaining this.

Done.

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1736073002/diff/160001/cc/trees/property_tree...
cc/trees/property_tree.h:580: void UpdateScrollOffsetMapEntry(int key,
On 2016/03/02 18:43:10, ajuma wrote:
> Can this be private?

Done.

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

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1736073002/diff/140001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:479: return synced_scroll_offset()->PendingBase();
On 2016/03/02 20:52:55, sunxd wrote:
> On 2016/03/02 18:43:09, ajuma wrote:
> > On 2016/03/02 17:45:34, sunxd wrote:
> > > On 2016/03/01 22:54:20, ajuma wrote:
> > > > Could PropertyTreeBuilder call CurrentScrollOffset instead (for both
Layer
> > and
> > > > LayerImpl)?
> > > 
> > > 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.
> > 
> > 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.)
> 
> 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.

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?

[sunxd, 2016-03-03 16:01:25]

https://codereview.chromium.org/1736073002/diff/220001/cc/layers/layer_impl_u...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/1736073002/diff/220001/cc/layers/layer_impl_u...
cc/layers/layer_impl_unittest.cc:332:
VERIFY_NEEDS_UPDATE_DRAW_PROPERTIES(layer->DidUpdateScrollOffset());
This is the change in the test. It seems to me that we can probably remove this
line since DidUpdateScrollOffset is called in the previous line?

[ajuma, 2016-03-03 18:05:06]

https://codereview.chromium.org/1736073002/diff/220001/cc/layers/layer_impl_u...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/1736073002/diff/220001/cc/layers/layer_impl_u...
cc/layers/layer_impl_unittest.cc:332:
VERIFY_NEEDS_UPDATE_DRAW_PROPERTIES(layer->DidUpdateScrollOffset());
On 2016/03/03 16:01:25, sunxd wrote:
> This is the change in the test. It seems to me that we can probably remove
this
> line since DidUpdateScrollOffset is called in the previous line?

The previous line is testing LayerImpl::PushScrollOFfsetFromMainThread, which is
now only called in tests, right? Even though this happens to lead to
LayerImpl::DidUpdateScrollOffset, a test that calls DidUpdateScrollOffset
directly seems worth having.

https://codereview.chromium.org/1736073002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1736073002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:10202: pending_base);
The way this logic worked before relied on the pending and active layers sharing
the same persistent SyncedProperty instance. But now we get a new SyncedProperty
instance each time we build property trees. Do you also need to make sure that
the active_base_ and active_delta_ (not just the pending_base_) get restored to
the previous value? Also, please add a comment explaining why the scroll offset
needs to be updated this way.

[sunxd, 2016-03-03 18:50:13]

https://codereview.chromium.org/1736073002/diff/220001/cc/layers/layer_impl_u...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/1736073002/diff/220001/cc/layers/layer_impl_u...
cc/layers/layer_impl_unittest.cc:332:
VERIFY_NEEDS_UPDATE_DRAW_PROPERTIES(layer->DidUpdateScrollOffset());
On 2016/03/03 18:05:06, ajuma wrote:
> On 2016/03/03 16:01:25, sunxd wrote:
> > This is the change in the test. It seems to me that we can probably remove
> this
> > line since DidUpdateScrollOffset is called in the previous line?
> 
> The previous line is testing LayerImpl::PushScrollOFfsetFromMainThread, which
is
> now only called in tests, right? Even though this happens to lead to
> LayerImpl::DidUpdateScrollOffset, a test that calls DidUpdateScrollOffset
> directly seems worth having.

Done.

https://codereview.chromium.org/1736073002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1736073002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:10202: pending_base);
On 2016/03/03 18:05:06, ajuma wrote:
> The way this logic worked before relied on the pending and active layers
sharing
> the same persistent SyncedProperty instance. But now we get a new
SyncedProperty
> instance each time we build property trees. Do you also need to make sure that
> the active_base_ and active_delta_ (not just the pending_base_) get restored
to
> the previous value? Also, please add a comment explaining why the scroll
offset
> needs to be updated this way.

I think we don't need to. ScrollTree only clear itself when built from main
thread, when building from impl thread, it calls synced_scroll_offset to do the
updates where synced_scroll_offset can fetch the existing offset.

[ajuma, 2016-03-03 19:14:35]

Thanks, lgtm.

[sunxd, 2016-03-03 19:17:11]

The CQ bit was checked by sunxd@chromium.org

[commit-bot: I haz the power, 2016-03-03 19:17:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736073002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736073002/240001

[commit-bot: I haz the power, 2016-03-03 20:41:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-03 20:41:38]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sunxd, 2016-03-03 22:05:50]

On 2016/03/03 20:41:38, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

I checked several other CL's buildbot results, it seems views_unittests:
WidgetTest.SysCommandMoveOnNCLButtonDownOnCaptionAndMoveTest often fails (with
or without patch), so I'll just commit this CL and file a bug.

[sunxd, 2016-03-03 22:22:33]

The CQ bit was checked by sunxd@chromium.org

[commit-bot: I haz the power, 2016-03-03 22:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1736073002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1736073002/240001

[commit-bot: I haz the power, 2016-03-03 22:31:19]

Description was changed from

==========
cc: Move SyncedScrollOffset to scroll tree

Now updating scrolling information on impl side can be independent of layer
impl.
There are still some left-over changes corresponding to
NoteLayerPropertiesChanged.

SyncedScrollOffset of scrollable layers are now stored in property trees instead
of
layer impl. The main thread property tree has one copy while pending and active
share
one.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move SyncedScrollOffset to scroll tree

Now updating scrolling information on impl side can be independent of layer
impl.
There are still some left-over changes corresponding to
NoteLayerPropertiesChanged.

SyncedScrollOffset of scrollable layers are now stored in property trees instead
of
layer impl. The main thread property tree has one copy while pending and active
share
one.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-03-03 22:31:20]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-03-03 22:32:35]

Description was changed from

==========
cc: Move SyncedScrollOffset to scroll tree

Now updating scrolling information on impl side can be independent of layer
impl.
There are still some left-over changes corresponding to
NoteLayerPropertiesChanged.

SyncedScrollOffset of scrollable layers are now stored in property trees instead
of
layer impl. The main thread property tree has one copy while pending and active
share
one.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move SyncedScrollOffset to scroll tree

Now updating scrolling information on impl side can be independent of layer
impl.
There are still some left-over changes corresponding to
NoteLayerPropertiesChanged.

SyncedScrollOffset of scrollable layers are now stored in property trees instead
of
layer impl. The main thread property tree has one copy while pending and active
share
one.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/c36713a0336c00a0971fe33514adc01f4df693fa
Cr-Commit-Position: refs/heads/master@{#379116}
==========

[commit-bot: I haz the power, 2016-03-03 22:32:36]

Patchset 13 (id:??) landed as
https://crrev.com/c36713a0336c00a0971fe33514adc01f4df693fa
Cr-Commit-Position: refs/heads/master@{#379116}