Clean LayerImpl's scroll offset callers in unit tests.

Clean LayerImpl's scroll offset callers in unit tests.

The getter ScrollDelta is left because it is called by
property tree builder.

LayerImpl is now not aware of the internal logic of synced
property.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[sunxd, 2016-03-04 21:29:12]

Description was changed from

==========
Clean LayerImpl's scroll offset callers in unit tests.

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

to

==========
Clean LayerImpl's scroll offset callers in unit tests.

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

[sunxd, 2016-03-07 18:14:22]

Description was changed from

==========
Clean LayerImpl's scroll offset callers in unit tests.

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

to

==========
Clean LayerImpl's scroll offset callers in unit tests.

The getter ScrollDelta is left because it is called by
property tree builder.

LayerImpl is now not aware of the internal logic of synced
property.

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

[sunxd, 2016-03-07 18:14:22]

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

[ajuma, 2016-03-07 19:10:19]

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_impl_un...
File cc/layers/layer_impl_unittest.cc (left):

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_impl_un...
cc/layers/layer_impl_unittest.cc:339: gfx::ScrollOffset(arbitrary_vector2d)));
For these removed lines, do we have equivalent test coverage elsewhere that
updating scrolls in the scroll tree does trigger needs_update_draw_properties?

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_impl_un...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_impl_un...
cc/layers/layer_impl_unittest.cc:176: gfx::ScrollOffset(arbitrary_vector2d.x(),
arbitrary_vector2d.y()));
Is this line needed? (It's not obvious which, if any, of the checks below depend
on this.)

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_positio...
File cc/layers/layer_position_constraint_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_positio...
cc/layers/layer_position_constraint_unittest.cc:199: // Layer_impl should not be
aware of synced property logics, this function is
LayerImpl

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_positio...
cc/layers/layer_position_constraint_unittest.cc:201: void
SetScrollOffsetDelta(LayerImpl* layer_impl,
static void

https://codereview.chromium.org/1766053002/diff/60001/cc/test/layer_tree_host...
File cc/test/layer_tree_host_common_test.h (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.h:164: void SetScrollOffsetDelta(LayerImpl*
layer_impl, const gfx::Vector2dF& delta);
static void

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:468: void
SetScrollOffsetDelta(LayerImpl* layer_impl,
This can be static.

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:75: ScrollTree* scroll_tree(LayerImpl*
layer_impl) {
Is this called anywhere?

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:78: ScrollTree*
scroll_tree(LayerImpl* layer_impl) {
This is probably better off as a helper function defined outside this class.
Also, it should be named something like GetScrollTree or ScrollTreeForLayer
(since naming it ScrollTree collides with the class name).

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:1411: void
SetScrollOffsetDelta(LayerImpl* layer_impl,
static

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

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.h:569: typedef std::map<int,
scoped_refptr<SyncedScrollOffset>> ScrollOffsetMap;
Why switch from unordered_map to map? (When does ordering matter?)

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:677:
data_for_children->scroll_tree->scroll_offset_map().erase(layer->id());
Is this only needed for tests? If so, please add a comment explaining. If it's
actually needed for non-test code, please also add a test.

[sunxd, 2016-03-09 01:51:21]

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_impl_un...
File cc/layers/layer_impl_unittest.cc (left):

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_impl_un...
cc/layers/layer_impl_unittest.cc:339: gfx::ScrollOffset(arbitrary_vector2d)));
On 2016/03/07 19:10:19, ajuma wrote:
> For these removed lines, do we have equivalent test coverage elsewhere that
> updating scrolls in the scroll tree does trigger needs_update_draw_properties?

Done.

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_impl_un...
File cc/layers/layer_impl_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_impl_un...
cc/layers/layer_impl_unittest.cc:176: gfx::ScrollOffset(arbitrary_vector2d.x(),
arbitrary_vector2d.y()));
On 2016/03/07 19:10:19, ajuma wrote:
> Is this line needed? (It's not obvious which, if any, of the checks below
depend
> on this.)

Done.

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_positio...
File cc/layers/layer_position_constraint_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_positio...
cc/layers/layer_position_constraint_unittest.cc:199: // Layer_impl should not be
aware of synced property logics, this function is
On 2016/03/07 19:10:19, ajuma wrote:
> LayerImpl

Done.

https://codereview.chromium.org/1766053002/diff/60001/cc/layers/layer_positio...
cc/layers/layer_position_constraint_unittest.cc:201: void
SetScrollOffsetDelta(LayerImpl* layer_impl,
On 2016/03/07 19:10:19, ajuma wrote:
> static void

Done.

https://codereview.chromium.org/1766053002/diff/60001/cc/test/layer_tree_host...
File cc/test/layer_tree_host_common_test.h (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.h:164: void SetScrollOffsetDelta(LayerImpl*
layer_impl, const gfx::Vector2dF& delta);
On 2016/03/07 19:10:19, ajuma wrote:
> static void

Done.

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:468: void
SetScrollOffsetDelta(LayerImpl* layer_impl,
On 2016/03/07 19:10:19, ajuma wrote:
> This can be static.

Done.

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:75: ScrollTree* scroll_tree(LayerImpl*
layer_impl) {
On 2016/03/07 19:10:19, ajuma wrote:
> Is this called anywhere?

Done.

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:78: ScrollTree*
scroll_tree(LayerImpl* layer_impl) {
On 2016/03/07 19:10:19, ajuma wrote:
> This is probably better off as a helper function defined outside this class.
> Also, it should be named something like GetScrollTree or ScrollTreeForLayer
> (since naming it ScrollTree collides with the class name).

Done.

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_scroll.cc:1411: void
SetScrollOffsetDelta(LayerImpl* layer_impl,
On 2016/03/07 19:10:19, ajuma wrote:
> static

Done.

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

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.h:569: typedef std::map<int,
scoped_refptr<SyncedScrollOffset>> ScrollOffsetMap;
On 2016/03/07 19:10:19, ajuma wrote:
> Why switch from unordered_map to map? (When does ordering matter?)

Sorry I ignored this change. It's just for gdb debugging, I'll switch it back in
the last patch.

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1766053002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:677:
data_for_children->scroll_tree->scroll_offset_map().erase(layer->id());
On 2016/03/07 19:10:19, ajuma wrote:
> Is this only needed for tests? If so, please add a comment explaining. If it's
> actually needed for non-test code, please also add a test.

Done.

[ajuma, 2016-03-09 16:05:17]

https://codereview.chromium.org/1766053002/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:7179:
->transform_tree.set_source_to_parent_updates_allowed(false);
This is really something that should always be false on the compositor thread
(not just for this test), so ideally this should happen in
LayerTreeImpl::BuildPropertyTreesForTesting. Do other tests fail if you move
this there? (If so, this could be left for a follow up.)

[sunxd, 2016-03-09 16:41:24]

https://codereview.chromium.org/1766053002/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:7179:
->transform_tree.set_source_to_parent_updates_allowed(false);
On 2016/03/09 16:05:17, ajuma wrote:
> This is really something that should always be false on the compositor thread
> (not just for this test), so ideally this should happen in
> LayerTreeImpl::BuildPropertyTreesForTesting. Do other tests fail if you move
> this there? (If so, this could be left for a follow up.)

No, only these two tests fail. This is understandable: computing the screen
space transform needs ancestor's transform in the initial property tree build,
but we prevent it from computing the source_to_parent transform by setting the
flag here.

However, I think BuildPropertyTreesForTesting is mimicking building property
trees on main thread, so I'm not sure if we want to put the flag there?

[ajuma, 2016-03-09 16:48:41]

https://codereview.chromium.org/1766053002/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:7179:
->transform_tree.set_source_to_parent_updates_allowed(false);
On 2016/03/09 16:41:24, sunxd wrote:
> On 2016/03/09 16:05:17, ajuma wrote:
> > This is really something that should always be false on the compositor
thread
> > (not just for this test), so ideally this should happen in
> > LayerTreeImpl::BuildPropertyTreesForTesting. Do other tests fail if you move
> > this there? (If so, this could be left for a follow up.)
> 
> No, only these two tests fail. This is understandable: computing the screen
> space transform needs ancestor's transform in the initial property tree build,
> but we prevent it from computing the source_to_parent transform by setting the
> flag here.
> 
> However, I think BuildPropertyTreesForTesting is mimicking building property
> trees on main thread, so I'm not sure if we want to put the flag there?

It's mimicking building property trees, but for use on the compositor thread
(where source_to_parent_updates shouldn't happen -- these updates are just a way
to handle layer positions changing on the main thread without having to rebuild
property trees; layer positions aren't supposed to change at all on the
compositor thread, so it's fine if tests that want to make such changes are
forced to rebuild property trees). Note that LayerTreeImpl::SetPropertyTrees
already sets this flag.

[ajuma, 2016-03-09 18:53:51]

Thanks, lgtm with one nit:

https://codereview.chromium.org/1766053002/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1766053002/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:7170: true);
This line shouldn't be needed (setting a scroll delta below should automatically
trigger a transform tree update).

[sunxd, 2016-03-09 19:39:42]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-03-09 19:39:42]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1766053002/#ps180001
(title: "Remove transform tree update setting")

[commit-bot: I haz the power, 2016-03-09 19:40:15]

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

[commit-bot: I haz the power, 2016-03-09 21:13:52]

Description was changed from

==========
Clean LayerImpl's scroll offset callers in unit tests.

The getter ScrollDelta is left because it is called by
property tree builder.

LayerImpl is now not aware of the internal logic of synced
property.

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

to

==========
Clean LayerImpl's scroll offset callers in unit tests.

The getter ScrollDelta is left because it is called by
property tree builder.

LayerImpl is now not aware of the internal logic of synced
property.

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

[commit-bot: I haz the power, 2016-03-09 21:13:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-09 21:15:07]

Description was changed from

==========
Clean LayerImpl's scroll offset callers in unit tests.

The getter ScrollDelta is left because it is called by
property tree builder.

LayerImpl is now not aware of the internal logic of synced
property.

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

to

==========
Clean LayerImpl's scroll offset callers in unit tests.

The getter ScrollDelta is left because it is called by
property tree builder.

LayerImpl is now not aware of the internal logic of synced
property.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-03-09 21:15:08]

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