Refactor LayerTreeImpl's scrollbar map to be keyed on element ids

Refactor LayerTreeImpl's scrollbar map to be keyed on element ids

An element can have up to two scrollbars and we can switch LayerTreeImpl's
scrollbar tracking map from layer_id->scrollbar_layer_id to
element_id->scrollbar_layer_id. This patch gets us a little closer to
making the cc scrollbar code just depend on element ids instead of layer
ids.

With this patch we can remove the layer_id param from
LayerTreeHostImpl::RegisterScrollbarAnimationController which will make
it easy to remove layer ids from the scrollbar layers in a followup (i.e.,
track just scroll_element_id instead of scroll_layer_id and scroll_element_id).

This patch is primarily a switch from layer_id to element_id. Some tests
needed updates to ensure the scrolling layer's element id is set.

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/2824693002
Cr-Commit-Position: refs/heads/master@{#465754}
Committed: https://chromium.googlesource.com/chromium/src/+/7b8ed8a28833335aebe45f6e2c4193c7ee058515

[pdr., 2017-04-16 01:48:51]

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

[wkorman, 2017-04-17 17:44:35]

lgtm

May need to hit 'Publish' to send email to wei for this and the other change, I
happened to see them in code review ui but don't think I saw an email.

https://codereview.chromium.org/2824693002/diff/1/cc/layers/scrollbar_layer_u...
File cc/layers/scrollbar_layer_unittest.cc (left):

https://codereview.chromium.org/2824693002/diff/1/cc/layers/scrollbar_layer_u...
cc/layers/scrollbar_layer_unittest.cc:743:
scroll_layer->SetScrollClipLayerId(layer_tree_root->id());
Don't need any more?

https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h#...
cc/trees/layer_tree_impl.h:539: std::multimap<ElementId, int> scrollbar_map_;
Suggestion, consider rename to something like element_id_to_scroll_layer_ids,
which makes code referencing more self documenting. We've been using this for
newer named maps in cc.

https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h#...
cc/trees/layer_tree_impl.h:539: std::multimap<ElementId, int> scrollbar_map_;
Future optimization potential -- similar to recent brettw@ change, maybe we
should look at internal impl of multimap, since we know there will never be more
than two, I expect this has inefficient memory overhead.

Could switch to analogue of flat_map for multimap, or a map whose value is a
tuple/vector/array-of-two.

See https://codereview.chromium.org/2801603010

[weiliangc, 2017-04-18 17:03:43]

LGTM. Agree w/ Walter's suggestions about flatmap.

[pdr., 2017-04-19 18:02:18]

PTAL

https://codereview.chromium.org/2824693002/diff/1/cc/layers/scrollbar_layer_u...
File cc/layers/scrollbar_layer_unittest.cc (left):

https://codereview.chromium.org/2824693002/diff/1/cc/layers/scrollbar_layer_u...
cc/layers/scrollbar_layer_unittest.cc:743:
scroll_layer->SetScrollClipLayerId(layer_tree_root->id());
On 2017/04/17 at 17:44:35, wkorman wrote:
> Don't need any more?

There were actually two of these (the first is on line 739) so I'm just removing
a duplicated line here.

https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h#...
cc/trees/layer_tree_impl.h:539: std::multimap<ElementId, int> scrollbar_map_;
On 2017/04/17 at 17:44:35, wkorman wrote:
> Future optimization potential -- similar to recent brettw@ change, maybe we
should look at internal impl of multimap, since we know there will never be more
than two, I expect this has inefficient memory overhead.
> 
> Could switch to analogue of flat_map for multimap, or a map whose value is a
tuple/vector/array-of-two.
> 
> See https://codereview.chromium.org/2801603010

Switched name from scrollbar_map_ to element_id_to_scrollbar_layer_ids_.

Yeah I think a flat_map and/or map whose value is a pair would be ideal here.
Mind if I pursue this in a followup? I'd like to keep this patch as just the
layer id -> element id change. I've added a TODO above
element_id_to_scrollbar_layer_ids_ to update to a better datastructure.

[wkorman, 2017-04-19 18:07:09]

lgtm

https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h#...
cc/trees/layer_tree_impl.h:539: std::multimap<ElementId, int> scrollbar_map_;
On 2017/04/19 18:02:18, pdr. wrote:
> On 2017/04/17 at 17:44:35, wkorman wrote:
> > Future optimization potential -- similar to recent brettw@ change, maybe we
> should look at internal impl of multimap, since we know there will never be
more
> than two, I expect this has inefficient memory overhead.
> > 
> > Could switch to analogue of flat_map for multimap, or a map whose value is a
> tuple/vector/array-of-two.
> > 
> > See https://codereview.chromium.org/2801603010
> 
> Switched name from scrollbar_map_ to element_id_to_scrollbar_layer_ids_.
> 
> Yeah I think a flat_map and/or map whose value is a pair would be ideal here.
> Mind if I pursue this in a followup? I'd like to keep this patch as just the
> layer id -> element id change. I've added a TODO above
> element_id_to_scrollbar_layer_ids_ to update to a better datastructure.

Yeah, definitely better as separate change.

https://codereview.chromium.org/2824693002/diff/20001/cc/input/single_scrollb...
File cc/input/single_scrollbar_animation_controller_thinning_unittest.cc
(right):

https://codereview.chromium.org/2824693002/diff/20001/cc/input/single_scrollb...
cc/input/single_scrollbar_animation_controller_thinning_unittest.cc:99:
kThinningDuration);
I think I dreamed that I knew what a thinning scroll bar was the other night.

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

https://codereview.chromium.org/2824693002/diff/20001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:539: // TODO(pdr): Refactor this to be more
efficient--likely a map where the key
s/key/value/ i believe

[pdr., 2017-04-19 18:12:31]

On 2017/04/19 at 18:07:09, wkorman wrote:
> lgtm
> 
> https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h
> File cc/trees/layer_tree_impl.h (right):
> 
>
https://codereview.chromium.org/2824693002/diff/1/cc/trees/layer_tree_impl.h#...
> cc/trees/layer_tree_impl.h:539: std::multimap<ElementId, int> scrollbar_map_;
> On 2017/04/19 18:02:18, pdr. wrote:
> > On 2017/04/17 at 17:44:35, wkorman wrote:
> > > Future optimization potential -- similar to recent brettw@ change, maybe
we
> > should look at internal impl of multimap, since we know there will never be
more
> > than two, I expect this has inefficient memory overhead.
> > > 
> > > Could switch to analogue of flat_map for multimap, or a map whose value is
a
> > tuple/vector/array-of-two.
> > > 
> > > See https://codereview.chromium.org/2801603010
> > 
> > Switched name from scrollbar_map_ to element_id_to_scrollbar_layer_ids_.
> > 
> > Yeah I think a flat_map and/or map whose value is a pair would be ideal
here.
> > Mind if I pursue this in a followup? I'd like to keep this patch as just the
> > layer id -> element id change. I've added a TODO above
> > element_id_to_scrollbar_layer_ids_ to update to a better datastructure.
> 
> Yeah, definitely better as separate change.
> 
>
https://codereview.chromium.org/2824693002/diff/20001/cc/input/single_scrollb...
> File cc/input/single_scrollbar_animation_controller_thinning_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2824693002/diff/20001/cc/input/single_scrollb...
> cc/input/single_scrollbar_animation_controller_thinning_unittest.cc:99:
kThinningDuration);
> I think I dreamed that I knew what a thinning scroll bar was the other night.
> 
>
https://codereview.chromium.org/2824693002/diff/20001/cc/trees/layer_tree_impl.h
> File cc/trees/layer_tree_impl.h (right):
> 
>
https://codereview.chromium.org/2824693002/diff/20001/cc/trees/layer_tree_imp...
> cc/trees/layer_tree_impl.h:539: // TODO(pdr): Refactor this to be more
efficient--likely a map where the key
> s/key/value/ i believe

Oopsies, you are correct.

[pdr., 2017-04-19 18:12:43]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-04-19 18:12:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
weiliangc@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2824693002/#ps40001
(title: "wrong:right :: key:value")

[commit-bot: I haz the power, 2017-04-19 18:13:24]

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 19:16:00]

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

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

Failed to apply patch for cc/trees/layer_tree_host_impl.cc:
While running git apply --index -3 -p1;
  error: patch failed: cc/trees/layer_tree_host_impl.cc:3453
  Falling back to three-way merge...
  Applied patch to 'cc/trees/layer_tree_host_impl.cc' with conflicts.
  U cc/trees/layer_tree_host_impl.cc

Patch:       cc/trees/layer_tree_host_impl.cc
Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index
9d04554dcc57cf6d22f6eb4c4bfcac65ad194d65..219d02d1e182841eadcd61d33684a8ec361d683a
100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -3453,9 +3453,7 @@ void LayerTreeHostImpl::MouseMoveAt(const gfx::Point&
viewport_point) {
   if (!new_animation_controller)
     return;
 
-  int new_layer_id = active_tree_->LayerIdByElementId(new_element_id);
-  for (ScrollbarLayerImplBase* scrollbar :
-       active_tree_->ScrollbarsFor(new_layer_id)) {
+  for (auto* scrollbar : active_tree_->ScrollbarsFor(new_element_id)) {
     new_animation_controller->DidMouseMoveNear(
         scrollbar->orientation(),
         DeviceSpaceDistanceToLayer(device_viewport_point, scrollbar) /
@@ -3681,14 +3679,13 @@ std::string LayerTreeHostImpl::LayerTreeAsJson() const {
 }
 
 void LayerTreeHostImpl::RegisterScrollbarAnimationController(
-    int scroll_layer_id,
     ElementId scroll_element_id) {
   if (settings().scrollbar_animator == LayerTreeSettings::NO_ANIMATOR)
     return;
   if (ScrollbarAnimationControllerForElementId(scroll_element_id))
     return;
   scrollbar_animation_controllers_[scroll_element_id] =
-      active_tree_->CreateScrollbarAnimationController(scroll_layer_id);
+      active_tree_->CreateScrollbarAnimationController(scroll_element_id);
 }
 
 void LayerTreeHostImpl::UnregisterScrollbarAnimationController(
@@ -3729,8 +3726,8 @@ void
LayerTreeHostImpl::SetNeedsRedrawForScrollbarAnimation() {
   SetNeedsRedraw();
 }
 
-ScrollbarSet LayerTreeHostImpl::ScrollbarsFor(int scroll_layer_id) const {
-  return active_tree_->ScrollbarsFor(scroll_layer_id);
+ScrollbarSet LayerTreeHostImpl::ScrollbarsFor(ElementId id) const {
+  return active_tree_->ScrollbarsFor(id);
 }
 
 void LayerTreeHostImpl::AddVideoFrameController(

[pdr., 2017-04-19 20:09:42]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-04-19 20:09:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
weiliangc@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2824693002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-19 20:10:24]

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 21:24:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492632582452760,
"parent_rev": "9cddc0b9cd174242d365e9d64615687384b73c89", "commit_rev":
"7b8ed8a28833335aebe45f6e2c4193c7ee058515"}

[commit-bot: I haz the power, 2017-04-19 21:24:41]

Description was changed from

==========
Refactor LayerTreeImpl's scrollbar map to be keyed on element ids

An element can have up to two scrollbars and we can switch LayerTreeImpl's
scrollbar tracking map from layer_id->scrollbar_layer_id to
element_id->scrollbar_layer_id. This patch gets us a little closer to
making the cc scrollbar code just depend on element ids instead of layer
ids.

With this patch we can remove the layer_id param from
LayerTreeHostImpl::RegisterScrollbarAnimationController which will make
it easy to remove layer ids from the scrollbar layers in a followup (i.e.,
track just scroll_element_id instead of scroll_layer_id and scroll_element_id).

This patch is primarily a switch from layer_id to element_id. Some tests
needed updates to ensure the scrolling layer's element id is set.

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

to

==========
Refactor LayerTreeImpl's scrollbar map to be keyed on element ids

An element can have up to two scrollbars and we can switch LayerTreeImpl's
scrollbar tracking map from layer_id->scrollbar_layer_id to
element_id->scrollbar_layer_id. This patch gets us a little closer to
making the cc scrollbar code just depend on element ids instead of layer
ids.

With this patch we can remove the layer_id param from
LayerTreeHostImpl::RegisterScrollbarAnimationController which will make
it easy to remove layer ids from the scrollbar layers in a followup (i.e.,
track just scroll_element_id instead of scroll_layer_id and scroll_element_id).

This patch is primarily a switch from layer_id to element_id. Some tests
needed updates to ensure the scrolling layer's element id is set.

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/2824693002
Cr-Commit-Position: refs/heads/master@{#465754}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b8ed8a28833335aebe45f6e2c41...
==========

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

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