Refactor scroll chaining to use ScrollNodes without going through LayerImpl

Refactor scroll chaining to use ScrollNodes without going through LayerImpl

Before this patch, ScrollState::DistributeToScrollChainDescendant kept
a list of const ScrollNode pointers and used the node's owning_layer_id to
lookup a non-const version of the ScrollNode through LayerImpl's
DistributeScroll. This indirection is not needed and we can simply use a
non-const ScrollNode list, bypassing LayerImpl::DistributeScroll.

It can be verified that this is equivalent by adding the following DHCECK
to ScrollState::DistributeToScrollChainDescendant:
DCHECK_EQ(scroll_tree.Node(layer_tree_impl_->LayerById(next->owning_layer_id)->scroll_tree_index()), next);

This patch lets us remove one use of ScrollNode's owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2708493002
Cr-Commit-Position: refs/heads/master@{#452143}
Committed: https://chromium.googlesource.com/chromium/src/+/014861b417eb3532456a982f1d81a384358fc859

[pdr., 2017-02-19 22:51:43]

Description was changed from

==========
Refactor scroll chaining to use ScrollNodes without going through LayerImpl

Before this patch, ScrollState::DistributeToScrollChainDescendant kept
a list of const ScrollNode pointers and used the node's owning_layer_id to
lookup a non-const version of the ScrollNode through LayerImpl's
DistributeScroll. This indirection is not needed and we can simply use a
non-const ScrollNode list, bypassing LayerImpl::DistributeScroll.

It can be verified that this is equivalent by adding the following DHCECK
to ScrollState::DistributeToScrollChainDescendant:
DCHECK_EQ(scroll_tree.Node(layer_tree_impl_->LayerById(next->owning_layer_id)->scroll_tree_index()),
next);

This patch lets us remove one use of ScrollNode's owning_layer_id.

BUG=693740
==========

to

==========
Refactor scroll chaining to use ScrollNodes without going through LayerImpl

Before this patch, ScrollState::DistributeToScrollChainDescendant kept
a list of const ScrollNode pointers and used the node's owning_layer_id to
lookup a non-const version of the ScrollNode through LayerImpl's
DistributeScroll. This indirection is not needed and we can simply use a
non-const ScrollNode list, bypassing LayerImpl::DistributeScroll.

It can be verified that this is equivalent by adding the following DHCECK
to ScrollState::DistributeToScrollChainDescendant:
DCHECK_EQ(scroll_tree.Node(layer_tree_impl_->LayerById(next->owning_layer_id)->scroll_tree_index()),
next);

This patch lets us remove one use of ScrollNode's owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[pdr., 2017-02-19 22:52:29]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-19 22:52:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-02-19 22:54:17]

pdr@chromium.org changed reviewers:
+ ajuma@chromium.org, bokan@chromium.org

[commit-bot: I haz the power, 2017-02-19 23:50:50]

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

[commit-bot: I haz the power, 2017-02-19 23:50:50]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-02-21 14:36:11]

lgtm

[pdr., 2017-02-21 19:21:15]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-02-21 19:22:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-02-21 19:33:39]

lgtm

https://codereview.chromium.org/2708493002/diff/1/cc/input/scroll_state.h
File cc/input/scroll_state.h (right):

https://codereview.chromium.org/2708493002/diff/1/cc/input/scroll_state.h#new...
cc/input/scroll_state.h:64: void
set_scroll_chain_and_layer_tree(std::list<ScrollNode*>* scroll_chain,
Nit, why pass by pointer rather than const-ref?

[commit-bot: I haz the power, 2017-02-21 21:24:42]

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

[commit-bot: I haz the power, 2017-02-21 21:24:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[pdr., 2017-02-22 18:12:33]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-02-22 18:13:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 18:44:42]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487787153422670, "parent_rev":
"2f99ba4adc9c23b0e611985304dd3a9c52235ed9", "commit_rev":
"014861b417eb3532456a982f1d81a384358fc859"}

[commit-bot: I haz the power, 2017-02-22 18:45:14]

Description was changed from

==========
Refactor scroll chaining to use ScrollNodes without going through LayerImpl

Before this patch, ScrollState::DistributeToScrollChainDescendant kept
a list of const ScrollNode pointers and used the node's owning_layer_id to
lookup a non-const version of the ScrollNode through LayerImpl's
DistributeScroll. This indirection is not needed and we can simply use a
non-const ScrollNode list, bypassing LayerImpl::DistributeScroll.

It can be verified that this is equivalent by adding the following DHCECK
to ScrollState::DistributeToScrollChainDescendant:
DCHECK_EQ(scroll_tree.Node(layer_tree_impl_->LayerById(next->owning_layer_id)->scroll_tree_index()),
next);

This patch lets us remove one use of ScrollNode's owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Refactor scroll chaining to use ScrollNodes without going through LayerImpl

Before this patch, ScrollState::DistributeToScrollChainDescendant kept
a list of const ScrollNode pointers and used the node's owning_layer_id to
lookup a non-const version of the ScrollNode through LayerImpl's
DistributeScroll. This indirection is not needed and we can simply use a
non-const ScrollNode list, bypassing LayerImpl::DistributeScroll.

It can be verified that this is equivalent by adding the following DHCECK
to ScrollState::DistributeToScrollChainDescendant:
DCHECK_EQ(scroll_tree.Node(layer_tree_impl_->LayerById(next->owning_layer_id)->scroll_tree_index()),
next);

This patch lets us remove one use of ScrollNode's owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2708493002
Cr-Commit-Position: refs/heads/master@{#452143}
Committed:
https://chromium.googlesource.com/chromium/src/+/014861b417eb3532456a982f1d81...
==========

[commit-bot: I haz the power, 2017-02-22 18:45:15]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/014861b417eb3532456a982f1d81...

[pdr., 2017-02-22 21:35:59]

https://codereview.chromium.org/2708493002/diff/1/cc/input/scroll_state.h
File cc/input/scroll_state.h (right):

https://codereview.chromium.org/2708493002/diff/1/cc/input/scroll_state.h#new...
cc/input/scroll_state.h:64: void
set_scroll_chain_and_layer_tree(std::list<ScrollNode*>* scroll_chain,
On 2017/02/21 at 19:33:39, bokan wrote:
> Nit, why pass by pointer rather than const-ref?

Good catch, this was just a mistake. I started removing consts and got carried
away. Fix up: https://codereview.chromium.org/2713713002