Add map from element id to scroll node index.

Add map from element id to scroll node index.

Essentially a scroll node specific version of
http://crrev.com/2612093002 which was only omitted there
in the interest of incrementality.

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

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

[wkorman, 2017-01-11 23:54:14]

Description was changed from

==========
Add map from element id to scroll node index.

BUG=674258
==========

to

==========
Add map from element id to scroll node index.

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

[wkorman, 2017-01-11 23:55:36]

Description was changed from

==========
Add map from element id to scroll node index.

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

to

==========
Add map from element id to scroll node index.

Essentially a scroll node specific version of
http://crrev.com/2612093002 which was only omitted there
in the interest of incrementality.

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

[wkorman, 2017-01-12 00:09:31]

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

[wkorman, 2017-01-12 00:09:31]

https://codereview.chromium.org/2623313003/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2623313003/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:3945: const int layer_id =
tree->LayerIdByElementId(element_id);
Even after this change this method still looks up layer id by element id for
purposes of passing along to ScrollTree::OnScrollOffsetAnimated, which ends up:

(1) in ScrollTree::synced_scroll_offset referencing
ScrollTree::layer_id_to_scroll_offset_map_, and

(2) in LayerTreeImpl::DidUpdateScrollOffset, using the layer id to get the
transform tree index (which has a TODO noting we can replace with
layer-id-to-index maps when available, which they are now, but now I think we
can actually use element-id-to-transform-index instead), and looking up the
current scroll offset via ScrollTree::current_scroll_offset, which uses
ScrollTree::synced_scroll_offset, which is (1).

However, I currently believe that we want to keep this part of the scrolling
code layer-id-driven since there are many other scrolling related callsites that
use ScrollTree::synced_scroll_offset in ScrollTree, and really the layer is the
thing being operated on, and our main goal is to extricate the property nodes
from a layer id dependency.

But I wanted to raise to hear any ajuma@ feedback.

[wkorman, 2017-01-12 00:09:45]

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

[commit-bot: I haz the power, 2017-01-12 00:10:39]

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-01-12 00:13:54]

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

[commit-bot: I haz the power, 2017-01-12 00:13:55]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[wkorman, 2017-01-12 00:30:45]

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

[commit-bot: I haz the power, 2017-01-12 00:33:34]

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-01-12 02:05:03]

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

[commit-bot: I haz the power, 2017-01-12 02:05:03]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-01-12 15:58:51]

lgtm

https://codereview.chromium.org/2623313003/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2623313003/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:3945: const int layer_id =
tree->LayerIdByElementId(element_id);
On 2017/01/12 00:09:31, wkorman wrote:
> Even after this change this method still looks up layer id by element id for
> purposes of passing along to ScrollTree::OnScrollOffsetAnimated, which ends
up:
> 
> (1) in ScrollTree::synced_scroll_offset referencing
> ScrollTree::layer_id_to_scroll_offset_map_, and
> 
> (2) in LayerTreeImpl::DidUpdateScrollOffset, using the layer id to get the
> transform tree index (which has a TODO noting we can replace with
> layer-id-to-index maps when available, which they are now, but now I think we
> can actually use element-id-to-transform-index instead), and looking up the
> current scroll offset via ScrollTree::current_scroll_offset, which uses
> ScrollTree::synced_scroll_offset, which is (1).
> 
> However, I currently believe that we want to keep this part of the scrolling
> code layer-id-driven since there are many other scrolling related callsites
that
> use ScrollTree::synced_scroll_offset in ScrollTree, and really the layer is
the
> thing being operated on, and our main goal is to extricate the property nodes
> from a layer id dependency.

I think scrolling should ultimately look the same as animation, driven by
element ids. Blink already sets an element id for each scrollable layer (in the
SPv1 codepath at least), so the corresponding scroll nodes should all have
element ids. I think most of the work to make this switch will be updating all
the unit tests that currently create scrollable layers without setting an
element id, and sorting out the scrollbar update logic
(LayerTreeImpl::UpdateScrollbars) that happens when DidUpdateScrollOffset gets
called.

[wkorman, 2017-01-12 19:10:03]

On 2017/01/12 at 15:58:51, ajuma wrote:
> I think scrolling should ultimately look the same as animation, driven by
element ids. Blink already sets an element id for each scrollable layer (in the
SPv1 codepath at least), so the corresponding scroll nodes should all have
element ids. I think most of the work to make this switch will be updating all
the unit tests that currently create scrollable layers without setting an
element id, and sorting out the scrollbar update logic
(LayerTreeImpl::UpdateScrollbars) that happens when DidUpdateScrollOffset gets
called.

OK, well, then, good news! It doesn't look like we're too far off. It looked
like mostly mechanical work when I surveyed yesterday. We can investigate
further as follow-on.

[wkorman, 2017-01-12 19:34:46]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-01-12 19:34:46]

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/2623313003/#ps40001
(title: "Sync to head.")

[commit-bot: I haz the power, 2017-01-12 19:35:35]

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-01-12 20:43:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484249686496850,
"parent_rev": "aec7d45bff4c7a8a697034754600f264988714ec", "commit_rev":
"d8a99a8083a992e00dc0316cc621ad9f0d9b6699"}

[commit-bot: I haz the power, 2017-01-12 20:43:36]

Description was changed from

==========
Add map from element id to scroll node index.

Essentially a scroll node specific version of
http://crrev.com/2612093002 which was only omitted there
in the interest of incrementality.

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

to

==========
Add map from element id to scroll node index.

Essentially a scroll node specific version of
http://crrev.com/2612093002 which was only omitted there
in the interest of incrementality.

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

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

[commit-bot: I haz the power, 2017-01-12 20:43:37]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d8a99a8083a992e00dc0316cc621...