Replace layer id with Element id for tracking scrollbar animation controllers

Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id. A future patch
will remove scrolling_layer_id from ScrollbarLayerInterface.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 3 callers of owning_layer_id (in LayerTreeHostImpl) and 2 callers
of LayerIdByElementId (in LayerTreeImpl).

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[pdr., 2017-04-13 21:46:05]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

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

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

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

[pdr., 2017-04-13 21:51:38]

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

[commit-bot: I haz the power, 2017-04-13 21:52:32]

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

[pdr., 2017-04-13 22:01:04]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

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

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-04-13 22:01:13]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel,master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-04-13 22:02:56]

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

[commit-bot: I haz the power, 2017-04-13 22:02:57]

Dry run: Your CL can not be processed by CQ because of:
 * Failed to parse additional trybots

[pdr., 2017-04-13 22:12:52]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel,master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-04-13 22:21:00]

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

[commit-bot: I haz the power, 2017-04-13 22:21:40]

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-04-13 23:54:45]

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

[commit-bot: I haz the power, 2017-04-13 23:54:46]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[pdr., 2017-04-14 01:17:14]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This required tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id (a TODO has
been added to clean this up further).

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-04-14 01:18:28]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-04-14 01:20:39]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id. ScrollbarLayerInterface
now tracks both the scrolling layer id and the scrolling element id, but
a future patch will be able to just track element ids.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-04-14 01:20:43]

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

[commit-bot: I haz the power, 2017-04-14 01:21:14]

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

[pdr., 2017-04-14 01:39:33]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id. ScrollbarLayerInterface
now tracks both the scrolling layer id and the scrolling element id, but
a future patch will be able to just track element ids.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 5 users of owning_layer_id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id. A future patch
will remove scrolling_layer_id from ScrollbarLayerInterface.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 3 callers of owning_layer_id (in LayerTreeHostImpl) and 2 callers
of LayerIdByElementId (in LayerTreeImpl).

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-04-14 02:16:47]

pdr@chromium.org changed reviewers:
+ weiliangc@chromium.org, wkorman@chromium.org

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

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2017-04-14 18:17:30]

lgtm

https://codereview.chromium.org/2816063003/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2816063003/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:108:
v_scrollbar_layer_->SetScrollInfo(scroll_layer_ptr->id(),
Wondered whether we would be best off naming the method SetScrollElementId(),
since we'll be removing the layer id soon. Info is better during interim but
seems we'll want to end up with just the element id. Just mentioning while
reading, ok to keep as is.

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/painted_scrol...
File cc/layers/painted_scrollbar_layer.h (right):

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/painted_scrol...
cc/layers/painted_scrollbar_layer.h:26: ElementId element_id);
We could add a default '= ElementId()' here and then tests and other places that
are just passing default instance wouldn't need to be changed. I see some
precedent in cc code, see for example cc/paint/paint_flags.h getFillPath.

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_interface.h (right):

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_interface.h:17: virtual ElementId ScrollElementId()
const = 0;
I think cc style is to name straight accessors like this that don't have any
logic as scroll_element_id(). Same for this method name throughout change.

There's a method rename script I hear but here's perl as well in case helpful,
something like:

% find ./cc -name "*.h" | xargs perl -p -i -e
's/ScrollElementId/scroll_element_id/'

Repeat for .cc, etc. Add "\(\)" after Id if you want more specificity but you
don't want to unless you have to so you'll get the comment mentions as well.

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

https://codereview.chromium.org/2816063003/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:741:
layer_tree_host_impl_->ScrollbarAnimationControllerForId(old_element_id);
Future change -- suggest renaming this method to qualify what kind of Id it is.
So, ScrollbarAnimationControllerForElementId.

[pdr., 2017-04-15 03:33:01]

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

[commit-bot: I haz the power, 2017-04-15 03:33:10]

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-04-15 05:26:18]

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

[commit-bot: I haz the power, 2017-04-15 05:26:19]

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

[pdr., 2017-04-15 06:02:00]

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/painted_scrol...
File cc/layers/painted_scrollbar_layer.h (right):

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/painted_scrol...
cc/layers/painted_scrollbar_layer.h:26: ElementId element_id);
On 2017/04/14 at 18:17:30, wkorman wrote:
> We could add a default '= ElementId()' here and then tests and other places
that are just passing default instance wouldn't need to be changed. I see some
precedent in cc code, see for example cc/paint/paint_flags.h getFillPath.

This saves many lines in the diff. Done

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_interface.h (right):

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_interface.h:17: virtual ElementId ScrollElementId()
const = 0;
On 2017/04/14 at 18:17:30, wkorman wrote:
> I think cc style is to name straight accessors like this that don't have any
logic as scroll_element_id(). Same for this method name throughout change.
> 
> There's a method rename script I hear but here's perl as well in case helpful,
something like:
> 
> % find ./cc -name "*.h" | xargs perl -p -i -e
's/ScrollElementId/scroll_element_id/'
> 
> Repeat for .cc, etc. Add "\(\)" after Id if you want more specificity but you
don't want to unless you have to so you'll get the comment mentions as well.

Thanks for the script, it made this super easy. Done.

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

https://codereview.chromium.org/2816063003/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:741:
layer_tree_host_impl_->ScrollbarAnimationControllerForId(old_element_id);
On 2017/04/14 at 18:17:30, wkorman wrote:
> Future change -- suggest renaming this method to qualify what kind of Id it
is. So, ScrollbarAnimationControllerForElementId.

+1, going to include that in this change since we both wanted it. I had done it
in a previous patch and took it out.

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1132: layer->SetElementId(inputs_.element_id);
LayerTreeHostImplTest.ScrollbarVisibilityChangeCausesRedrawAndCommit failed
because of this. The issue was that layer->SetScrollClipLayer depends on the
element id being set, but SetElementId was after SetScrollClipLayer. This was
flaky but would pass on retry because of sync timing, but luckily
linux_chromium_tsan_rel_ng failed reliably.

[wkorman, 2017-04-17 17:53:36]

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/scrollbar_lay...
File cc/layers/scrollbar_layer_interface.h (right):

https://codereview.chromium.org/2816063003/diff/40001/cc/layers/scrollbar_lay...
cc/layers/scrollbar_layer_interface.h:17: virtual ElementId ScrollElementId()
const = 0;
On 2017/04/15 06:02:00, pdr. wrote:
> On 2017/04/14 at 18:17:30, wkorman wrote:
> > I think cc style is to name straight accessors like this that don't have any
> logic as scroll_element_id(). Same for this method name throughout change.
> > 
> > There's a method rename script I hear but here's perl as well in case
helpful,
> something like:
> > 
> > % find ./cc -name "*.h" | xargs perl -p -i -e
> 's/ScrollElementId/scroll_element_id/'
> > 
> > Repeat for .cc, etc. Add "\(\)" after Id if you want more specificity but
you
> don't want to unless you have to so you'll get the comment mentions as well.
> 
> Thanks for the script, it made this super easy. Done.

Glad you got use out of this, learning the Perl in-place edit command line
switch and reading this book are among highest value things ever:

https://www.amazon.com/Mastering-Regular-Expressions-Jeffrey-Friedl/dp/059652...

[pdr., 2017-04-17 19:02:52]

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

[pdr., 2017-04-17 19:05:05]

+enne for review.

Here's a sneak peak at two followups that are based on this patch:
Refactor LayerTreeImpl's scrollbar map to be keyed on element ids
https://codereview.chromium.org/2824693002
Refactor FindScrollLayerForDeviceViewportPoint to use scroll nodes over layer
ids
https://codereview.chromium.org/2824683002

[enne (OOO), 2017-04-18 00:47:57]

This seems pretty straightforward, even if it's a ton of plumbing.

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1132: layer->SetElementId(inputs_.element_id);
On 2017/04/15 at 06:02:00, pdr. wrote:
> LayerTreeHostImplTest.ScrollbarVisibilityChangeCausesRedrawAndCommit failed
because of this. The issue was that layer->SetScrollClipLayer depends on the
element id being set, but SetElementId was after SetScrollClipLayer. This was
flaky but would pass on retry because of sync timing, but luckily
linux_chromium_tsan_rel_ng failed reliably.

Good catch! That sounds like a terrible thing to have to track down!


Maybe I'm just unfamiliar with this code but "element id" doesn't mean very much
to me.  I'm honestly surprised a layer could have a different element id over
time.  Is there some documentation for what this means?  Sadly,
Layer::SetElementId has no comments either.

If ElementId is related to scrolling, should SetScrollClipLayer become
SetScrollInfo(element id, clip layer id?) so they can't be called in the
incorrect order?  Or should SetElementId do more work to fix up whatever
SetScrollClipLayer did so that they can be called in either order?

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/painted_overl...
File cc/layers/painted_overlay_scrollbar_layer.h (right):

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/painted_overl...
cc/layers/painted_overlay_scrollbar_layer.h:35: void SetScrollInfo(int layer_id,
ElementId element_id) override;
Can you leave a TODO that layer id is deprecated here?

Is there a reason to not just remove it in this patch?

[pdr., 2017-04-18 04:34:11]

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1132: layer->SetElementId(inputs_.element_id);
On 2017/04/18 at 00:47:57, enne wrote:
> On 2017/04/15 at 06:02:00, pdr. wrote:
> > LayerTreeHostImplTest.ScrollbarVisibilityChangeCausesRedrawAndCommit failed
because of this. The issue was that layer->SetScrollClipLayer depends on the
element id being set, but SetElementId was after SetScrollClipLayer. This was
flaky but would pass on retry because of sync timing, but luckily
linux_chromium_tsan_rel_ng failed reliably.
> 
> Good catch! That sounds like a terrible thing to have to track down!
> 
> 
> Maybe I'm just unfamiliar with this code but "element id" doesn't mean very
much to me.  I'm honestly surprised a layer could have a different element id
over time.  Is there some documentation for what this means?  Sadly,
Layer::SetElementId has no comments either.

We do have a big comment in element_id.h that is... very out of date. In the
latest patch I've updated it to describe the two usecases for element ids
(stable ids for scrolling and animations) and a little more about the overall
design. Walter, could you please take a look at the new comment too?

> 
> If ElementId is related to scrolling, should SetScrollClipLayer become
SetScrollInfo(element id, clip layer id?) so they can't be called in the
incorrect order?  Or should SetElementId do more work to fix up whatever
SetScrollClipLayer did so that they can be called in either order?

Good catch.

I looked around and I don't think element ids need to change (now, or in the
future). Refactoring layers to take an element id parameter does not look
trivial though. This patch does introduce a new ordering dependency, so we
should enforce that better. I need to think of a better way to handle this..

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/painted_overl...
File cc/layers/painted_overlay_scrollbar_layer.h (right):

https://codereview.chromium.org/2816063003/diff/60001/cc/layers/painted_overl...
cc/layers/painted_overlay_scrollbar_layer.h:35: void SetScrollInfo(int layer_id,
ElementId element_id) override;
On 2017/04/18 at 00:47:57, enne wrote:
> Can you leave a TODO that layer id is deprecated here?

Done, I added a comment here and to all other ScrollbarLayerInterface clients
that scrollbars should be refactored to just rely on element ids.

> 
> Is there a reason to not just remove it in this patch?

The scroll layer -> scrollbar layer map still depends on these layer ids (e.g.,
LayerTreeImpl::RegisterScrollbar). In a followup
(https://codereview.chromium.org/2824693002), I will refactor the scrollbar
mapping to use element ids and then we'll be able to switch SetScrollInfo to
just be SetScrollElementId.

In writing this response, I realized we can remove the ScrollLayerId accessor
from ScrollbarLayerInterface but we still need to pass it in to SetScrollInfo
for now.

[enne (OOO), 2017-04-18 17:42:11]

On 2017/04/18 at 04:34:11, pdr wrote:

> I looked around and I don't think element ids need to change (now, or in the
future). Refactoring layers to take an element id parameter does not look
trivial though. This patch does introduce a new ordering dependency, so we
should enforce that better. I need to think of a better way to handle this..

Agreed that changing the Layer ctor is not something to be done lightly or
possibly at all.  <_<

Do all Layers have ElementIds?  If so, one approach might be to make
SetElementId DCHECK that the element id has never been set before.  Then maybe
make the SetScrollClipLayer DCHECK that the element id has been set?  Then maybe
also leave a comment in Layer/LayerImppl push properties that SetElementId needs
to be set first before anything else.  Is that plausible?

[pdr., 2017-04-18 18:26:49]

https://codereview.chromium.org/2816063003/diff/80001/cc/trees/element_id.h
File cc/trees/element_id.h (right):

https://codereview.chromium.org/2816063003/diff/80001/cc/trees/element_id.h#n...
cc/trees/element_id.h:26: 
I've split out the update to this comment in
https://codereview.chromium.org/2826703003 and will remove it from this review.

[pdr., 2017-04-18 22:28:42]

I've added a small comment to the SetElementId functions pointing to the new
class comment being added in https://codereview.chromium.org/2826703003. I also
added a comment in PushPropertiesTo noting that the element id needs to be set
first. I don't love this approach but it seems to be practical.

PTAL

[pdr., 2017-04-18 22:29:01]

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

[enne (OOO), 2017-04-18 22:29:46]

lgtm

[pdr., 2017-04-19 01:36:41]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-04-19 01:36:42]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2816063003/#ps100001
(title: "Address reviewer comments, pull element_id.h change to another patch")

[commit-bot: I haz the power, 2017-04-19 01:36:58]

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-04-19 03:11:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492565801737770,
"parent_rev": "7b57ef309c97d1fd37c5eb83377b7b5e511fb292", "commit_rev":
"faf2ee225610f256279fe77bf1413bb512dec3c2"}

[commit-bot: I haz the power, 2017-04-19 03:11:19]

Description was changed from

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id. A future patch
will remove scrolling_layer_id from ScrollbarLayerInterface.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 3 callers of owning_layer_id (in LayerTreeHostImpl) and 2 callers
of LayerIdByElementId (in LayerTreeImpl).

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace layer id with Element id for tracking scrollbar animation controllers

This patch tracks scrollbar animation controllers with ElementIds instead
of layer ids. This requires tracking the scrolling element id in
scrollbar layers in addition to the scrolling layer id. A future patch
will remove scrolling_layer_id from ScrollbarLayerInterface.

This patch get us closer to removing scroll_node's owning_layer_id by
removing 3 callers of owning_layer_id (in LayerTreeHostImpl) and 2 callers
of LayerIdByElementId (in LayerTreeImpl).

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-04-19 03:11:20]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/faf2ee225610f256279fe77bf141...

[pdr., 2017-04-20 02:53:42]

https://codereview.chromium.org/2816063003/diff/100001/cc/layers/scrollbar_la...
File cc/layers/scrollbar_layer_impl_base.cc (right):

https://codereview.chromium.org/2816063003/diff/100001/cc/layers/scrollbar_la...
cc/layers/scrollbar_layer_impl_base.cc:50: scroll_element_id ==
scroll_element_id)
Oops, "scroll_element_id == scroll_element_id" instead of "scroll_element_id_ ==
scroll_element_id". Fixing in followup:
https://codereview.chromium.org/2827163005