Track the currently scrolling ScrollNode instead of the scrolling layer

Track the currently scrolling ScrollNode instead of the scrolling layer

This patch removes CurrentlyScrollingLayer functions from LayerTreeImpl
and LayerTreeHostImpl, replacing them with calls to CurrentlyScrollingNode.
This removes two more uses of ScrollNode's owning_layer_id and sets us
up to remove the ScrollbarAnimationController dependency on layer ids
as well (left out of this patch to keep it simple).

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

Review-Url: https://codereview.chromium.org/2720183003
Cr-Commit-Position: refs/heads/master@{#454006}
Committed: https://chromium.googlesource.com/chromium/src/+/b2d4d4ba555e4658e5b2ea2ba4cbd9b95ac84442

[pdr., 2017-02-28 04:54:58]

Description was changed from

==========
Track the currently scrolling ScrollNode instead of the scrolling layer

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

to

==========
Track the currently scrolling ScrollNode instead of the scrolling layer

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

[pdr., 2017-02-28 04:57:18]

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

[commit-bot: I haz the power, 2017-02-28 04:57:54]

Dry run: 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-28 05:00:05]

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

[commit-bot: I haz the power, 2017-02-28 05:00:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pdr., 2017-02-28 05:52:31]

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

[commit-bot: I haz the power, 2017-02-28 05:52:49]

Dry run: 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-28 06:52:56]

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

[commit-bot: I haz the power, 2017-02-28 06:52:57]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-02-28 19:18:25]

Description was changed from

==========
Track the currently scrolling ScrollNode instead of the scrolling layer

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

to

==========
Track the currently scrolling ScrollNode instead of the scrolling layer

This patch removes CurrentlyScrollingLayer functions from LayerTreeImpl
and LayerTreeHostImpl, replacing them with calls to CurrentlyScrollingNode.

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

[pdr., 2017-02-28 19:18:25]

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

[pdr., 2017-02-28 19:20:10]

Description was changed from

==========
Track the currently scrolling ScrollNode instead of the scrolling layer

This patch removes CurrentlyScrollingLayer functions from LayerTreeImpl
and LayerTreeHostImpl, replacing them with calls to CurrentlyScrollingNode.

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

to

==========
Track the currently scrolling ScrollNode instead of the scrolling layer

This patch removes CurrentlyScrollingLayer functions from LayerTreeImpl
and LayerTreeHostImpl, replacing them with calls to CurrentlyScrollingNode.
This removes two more uses of ScrollNode's owning_layer_id and sets us
up to remove the ScrollbarAnimationController dependency on layer ids
as well (left out of this patch to keep it simple).

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

[pdr., 2017-02-28 19:20:22]

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/tree_synchroni...
File cc/trees/tree_synchronizer_unittest.cc (right):

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/tree_synchroni...
cc/trees/tree_synchronizer_unittest.cc:608:
host_impl->active_tree()->SetCurrentlyScrollingNode(
Reviewer note: This has to be after property trees are built to get the right
index. This makes this part of the test less real than it was before.

[ajuma, 2017-02-28 22:59:37]

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:609: if (last_scrolled_node_index) {
This needs to be compared to ScrollTree::kInvalidNodeId

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2677: viewport()->MainScrollLayer()
Can MainScrollLayer() be null here? The code added above to
IsCurrentlyScrollingViewport assumes it can't be but here and in
PinchGestureBegin we check for null, so just wondering if that's an intentional
difference.

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:439: auto* currently_scrolling_node =
target_tree->CurrentlyScrollingNode();
This gets a pointer to a node in the target tree's old screen. It looks like
that could get destroyed or overwritten on the next line when the new trees get
set (say if property tree topology changes). It seems safer to use a stable id
for this step (which, for now, would be owning_layer_id).

[pdr., 2017-03-01 07:21:03]

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:609: if (last_scrolled_node_index) {
On 2017/02/28 at 22:59:37, ajuma wrote:
> This needs to be compared to ScrollTree::kInvalidNodeId

Done

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2677: viewport()->MainScrollLayer()
On 2017/02/28 at 22:59:37, ajuma wrote:
> Can MainScrollLayer() be null here? The code added above to
IsCurrentlyScrollingViewport assumes it can't be but here and in
PinchGestureBegin we check for null, so just wondering if that's an intentional
difference.

I confirmed that MainScrollLayer can be null in tests so this isn't guaranteed
to be non-null.

I've added a null check to IsCurrentlyScrollingViewport. There are 4 places
where we lookup the MainScrollLayer's scroll node so I've combined them into
"LayerTreeHostImpl::OuterViewportScrollNode" which checks for a null
MainScrollLayer. It's a little confusing to mix "OuterViewportScrollNode" and
"MainScrollLayer" but I think the former is a little easier to understand since
viewport()->MainScrollLayer() actually returns OuterViewportScrollLayer().

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:439: auto* currently_scrolling_node =
target_tree->CurrentlyScrollingNode();
On 2017/02/28 at 22:59:37, ajuma wrote:
> This gets a pointer to a node in the target tree's old screen. It looks like
that could get destroyed or overwritten on the next line when the new trees get
set (say if property tree topology changes). It seems safer to use a stable id
for this step (which, for now, would be owning_layer_id).

You're right, and fixing this makes the TreeSynch test much saner.

Longer-term, is there any existing mapping between property tree nodes across
active->pending? It shouldn't be too hard to make one for the current scroll
node, but it'll be a little awkward doing that using just stable layers.

[pdr., 2017-03-01 08:09:35]

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

[commit-bot: I haz the power, 2017-03-01 08:09:56]

Dry run: 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-03-01 09:10:59]

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

[commit-bot: I haz the power, 2017-03-01 09:11:00]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-03-01 15:29:20]

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:439: auto* currently_scrolling_node =
target_tree->CurrentlyScrollingNode();
On 2017/03/01 07:21:03, pdr. wrote:
> On 2017/02/28 at 22:59:37, ajuma wrote:
> > This gets a pointer to a node in the target tree's old screen. It looks like
> that could get destroyed or overwritten on the next line when the new trees
get
> set (say if property tree topology changes). It seems safer to use a stable id
> for this step (which, for now, would be owning_layer_id).
> 
> You're right, and fixing this makes the TreeSynch test much saner.
> 
> Longer-term, is there any existing mapping between property tree nodes across
> active->pending? It shouldn't be too hard to make one for the current scroll
> node, but it'll be a little awkward doing that using just stable layers.

I think the longer term approach would be to use the mapping from element id to
index (element_id_to_scroll_node_index). In fact that might even work now, since
it looks like all scrollable layers and nodes are already assigned an ElementId.

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

https://codereview.chromium.org/2720183003/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:556: return node->id ==
viewport()->MainScrollLayer()->scroll_tree_index();
I think the logic above is equivalent to just doing
CurrentlyScrollingNode() == OuterViewportScrollNode()

[ajuma, 2017-03-01 15:29:46]

lgtm

[pdr., 2017-03-01 19:31:27]

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

https://codereview.chromium.org/2720183003/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:556: return node->id ==
viewport()->MainScrollLayer()->scroll_tree_index();
On 2017/03/01 at 15:29:20, ajuma wrote:
> I think the logic above is equivalent to just doing
> CurrentlyScrollingNode() == OuterViewportScrollNode()

Yeah it's the same (as long as we check that the currently scrolling node is
non-null) but OuterViewportScrollNode does a lookup of the ScrollNode and I
thought we could avoid it. There are a couple other places where I use the
scroll_tree_index for this performance reason. It's just a vector lookup so I
could go either way--do you think the code cleanliness of using
OuterViewportScrollNode and pointer equality outweighs the redundant ScrollNode
lookup?

[ajuma, 2017-03-01 19:34:42]

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

https://codereview.chromium.org/2720183003/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:556: return node->id ==
viewport()->MainScrollLayer()->scroll_tree_index();
On 2017/03/01 19:31:26, pdr. wrote:
> On 2017/03/01 at 15:29:20, ajuma wrote:
> > I think the logic above is equivalent to just doing
> > CurrentlyScrollingNode() == OuterViewportScrollNode()
> 
> Yeah it's the same (as long as we check that the currently scrolling node is
> non-null) but OuterViewportScrollNode does a lookup of the ScrollNode and I
> thought we could avoid it. There are a couple other places where I use the
> scroll_tree_index for this performance reason. It's just a vector lookup so I
> could go either way--do you think the code cleanliness of using
> OuterViewportScrollNode and pointer equality outweighs the redundant
ScrollNode
> lookup?

Ah, I missed that difference. Leaving this as-is using scroll_tree_index sgtm.

[pdr., 2017-03-01 19:50:21]

Thanks for the review! You caught some nasty bugs once again.

On 2017/03/01 at 15:29:20, ajuma wrote:
>
https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_imp...
> File cc/trees/layer_tree_impl.cc (right):
> 
>
https://codereview.chromium.org/2720183003/diff/20001/cc/trees/layer_tree_imp...
> cc/trees/layer_tree_impl.cc:439: auto* currently_scrolling_node =
target_tree->CurrentlyScrollingNode();
> On 2017/03/01 07:21:03, pdr. wrote:
> > On 2017/02/28 at 22:59:37, ajuma wrote:
> > > This gets a pointer to a node in the target tree's old screen. It looks
like
> > that could get destroyed or overwritten on the next line when the new trees
get
> > set (say if property tree topology changes). It seems safer to use a stable
id
> > for this step (which, for now, would be owning_layer_id).
> > 
> > You're right, and fixing this makes the TreeSynch test much saner.
> > 
> > Longer-term, is there any existing mapping between property tree nodes
across
> > active->pending? It shouldn't be too hard to make one for the current scroll
> > node, but it'll be a little awkward doing that using just stable layers.
> 
> I think the longer term approach would be to use the mapping from element id
to index (element_id_to_scroll_node_index). In fact that might even work now,
since it looks like all scrollable layers and nodes are already assigned an
ElementId.

I'm going to try this as a followup today.

[pdr., 2017-03-01 19:50:31]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-03-01 19:51:22]

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-03-01 19:56:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488397831147020,
"parent_rev": "3f1441642c8943c3c42799d529e6788e11ddc510", "commit_rev":
"b2d4d4ba555e4658e5b2ea2ba4cbd9b95ac84442"}

[commit-bot: I haz the power, 2017-03-01 19:57:26]

Description was changed from

==========
Track the currently scrolling ScrollNode instead of the scrolling layer

This patch removes CurrentlyScrollingLayer functions from LayerTreeImpl
and LayerTreeHostImpl, replacing them with calls to CurrentlyScrollingNode.
This removes two more uses of ScrollNode's owning_layer_id and sets us
up to remove the ScrollbarAnimationController dependency on layer ids
as well (left out of this patch to keep it simple).

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

to

==========
Track the currently scrolling ScrollNode instead of the scrolling layer

This patch removes CurrentlyScrollingLayer functions from LayerTreeImpl
and LayerTreeHostImpl, replacing them with calls to CurrentlyScrollingNode.
This removes two more uses of ScrollNode's owning_layer_id and sets us
up to remove the ScrollbarAnimationController dependency on layer ids
as well (left out of this patch to keep it simple).

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

Review-Url: https://codereview.chromium.org/2720183003
Cr-Commit-Position: refs/heads/master@{#454006}
Committed:
https://chromium.googlesource.com/chromium/src/+/b2d4d4ba555e4658e5b2ea2ba4cb...
==========

[commit-bot: I haz the power, 2017-03-01 19:57:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b2d4d4ba555e4658e5b2ea2ba4cb...