Remove indirection: setup scrollbar scroll layers in the scrollbar constructor

Remove indirection: setup scrollbar scroll layers in the scrollbar constructor

Scrollbar layers keep track of the layer id (soon: element id) that they
actually scroll. Before this patch, scrollbar layers were constructed
with no scroll layer id, then later updated to set the scroll layer id.
This patch removes this indirection and directly sets the scroll layer
in the constructor.

Removed functions:
  VisualViewport::setScrollLayerOnScrollbars
  WebScrollbarLayer::setScrollLayer
  WebScrollbarLayerImpl::setScrollLayer
  PaintedOverlayScrollbarLayer::SetScrollLayer
  PaintedScrollbarLayer::SetScrollLayer
  SolidColorScrollbarLayer::SetScrollLayer

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

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

[pdr., 2017-03-03 23:56:30]

Description was changed from

==========
Remove indirection: setup scrollbar scroll layers in the scrollbar constructor

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

to

==========
Remove indirection: setup scrollbar scroll layers in the scrollbar constructor

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

[pdr., 2017-03-03 23:57:11]

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

[commit-bot: I haz the power, 2017-03-03 23:57:39]

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

[pdr., 2017-03-04 00:07:06]

Description was changed from

==========
Remove indirection: setup scrollbar scroll layers in the scrollbar constructor

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

to

==========
Remove indirection: setup scrollbar scroll layers in the scrollbar constructor

Scrollbar layers keep track of the layer id (soon: element id) that they
actually scroll. Before this patch, scrollbar layers were constructed
with no scroll layer id, then later updated to set the scroll layer id.
This patch removes this indirection and directly sets the scroll layer
in the constructor.

Removed functions:
  VisualViewport::setScrollLayerOnScrollbars
  WebScrollbarLayer::setScrollLayer
  WebScrollbarLayerImpl::setScrollLayer
  PaintedOverlayScrollbarLayer::SetScrollLayer
  PaintedScrollbarLayer::SetScrollLayer
  SolidColorScrollbarLayer::SetScrollLayer

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

[commit-bot: I haz the power, 2017-03-04 00:57:37]

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

[commit-bot: I haz the power, 2017-03-04 00:57:38]

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

[pdr., 2017-03-04 01:45:58]

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

[commit-bot: I haz the power, 2017-03-04 01:46:09]

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

[pdr., 2017-03-04 01:53:12]

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

[commit-bot: I haz the power, 2017-03-04 01:53:27]

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

[pdr., 2017-03-04 02:37:53]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[bokan, 2017-03-06 14:36:35]

https://codereview.chromium.org/2728253002/diff/40001/cc/blink/web_scrollbar_...
File cc/blink/web_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/2728253002/diff/40001/cc/blink/web_scrollbar_...
cc/blink/web_scrollbar_layer_impl.cc:49: bool,
Whoops, this bool argument is used solely to pick this overload of the
constructor but isn't used. I added this by accident. Could you name this
argument |is_overlay| and merge it with the above constructor, picking which
ScrollbarLayer type to create based on this bool? (alternatively I can fix this
myself after you land this patch).

https://codereview.chromium.org/2728253002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2728253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:454:
detachScrollbarLayer(scrollbarGraphicsLayer);
Why do we need to do this here now? setupScrollbarLayer will do this, do we need
to avoid the code below in this case?

[pdr., 2017-03-06 19:00:24]

https://codereview.chromium.org/2728253002/diff/40001/cc/blink/web_scrollbar_...
File cc/blink/web_scrollbar_layer_impl.cc (right):

https://codereview.chromium.org/2728253002/diff/40001/cc/blink/web_scrollbar_...
cc/blink/web_scrollbar_layer_impl.cc:49: bool,
On 2017/03/06 at 14:36:34, bokan wrote:
> Whoops, this bool argument is used solely to pick this overload of the
constructor but isn't used. I added this by accident. Could you name this
argument |is_overlay| and merge it with the above constructor, picking which
ScrollbarLayer type to create based on this bool? (alternatively I can fix this
myself after you land this patch).

Sure, removed.

https://codereview.chromium.org/2728253002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2728253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:454:
detachScrollbarLayer(scrollbarGraphicsLayer);
On 2017/03/06 at 14:36:34, bokan wrote:
> Why do we need to do this here now? setupScrollbarLayer will do this, do we
need to avoid the code below in this case?

Before this patch, this function called detachScrollbarLayer in both of
"important" return codepaths: the early-out a few lines up for custom
scrollbars, and in setupScrollbarLayer. This patch can skip even creating the
scrollbar layers but I didn't want to change the detachScrollbarLayer logic.

We could remove the DCHECK in setupScrollbarLayer that checks for a scrollbar
layer and call setupScrollbarLayer. I think it's a little cleaner to just make
the early-out behavior more obvious. I don't really think we even need to call
detachScrollbarLayer but I didn't want to tackle that change in this patch.

[pdr., 2017-03-06 19:00:29]

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

[commit-bot: I haz the power, 2017-03-06 19:00:48]

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

[pdr., 2017-03-06 19:08:08]

On 2017/03/06 at 14:36:35, bokan wrote:
>
https://codereview.chromium.org/2728253002/diff/40001/cc/blink/web_scrollbar_...
> File cc/blink/web_scrollbar_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/2728253002/diff/40001/cc/blink/web_scrollbar_...
> cc/blink/web_scrollbar_layer_impl.cc:49: bool,
> Whoops, this bool argument is used solely to pick this overload of the
constructor but isn't used. I added this by accident. Could you name this
argument |is_overlay| and merge it with the above constructor, picking which
ScrollbarLayer type to create based on this bool? (alternatively I can fix this
myself after you land this patch).

Oops, I did this wrong. Please wait for review.

[commit-bot: I haz the power, 2017-03-06 19:16:23]

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

[commit-bot: I haz the power, 2017-03-06 19:16:25]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2017-03-06 19:54:26]

On 2017/03/06 at 19:08:08, pdr. wrote:
> On 2017/03/06 at 14:36:35, bokan wrote:
> >
https://codereview.chromium.org/2728253002/diff/40001/cc/blink/web_scrollbar_...
> > File cc/blink/web_scrollbar_layer_impl.cc (right):
> > 
> >
https://codereview.chromium.org/2728253002/diff/40001/cc/blink/web_scrollbar_...
> > cc/blink/web_scrollbar_layer_impl.cc:49: bool,
> > Whoops, this bool argument is used solely to pick this overload of the
constructor but isn't used. I added this by accident. Could you name this
argument |is_overlay| and merge it with the above constructor, picking which
ScrollbarLayer type to create based on this bool? (alternatively I can fix this
myself after you land this patch).
> 
> Oops, I did this wrong. Please wait for review.

Ok, should be good to go now. Please take a look

[pdr., 2017-03-06 20:33:40]

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

[commit-bot: I haz the power, 2017-03-06 20:34:34]

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

[bokan, 2017-03-06 21:49:29]

lgtm

https://codereview.chromium.org/2728253002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2728253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:454:
detachScrollbarLayer(scrollbarGraphicsLayer);
On 2017/03/06 19:00:24, pdr. wrote:
> On 2017/03/06 at 14:36:34, bokan wrote:
> > Why do we need to do this here now? setupScrollbarLayer will do this, do we
> need to avoid the code below in this case?
> 
> Before this patch, this function called detachScrollbarLayer in both of
> "important" return codepaths: the early-out a few lines up for custom
> scrollbars, and in setupScrollbarLayer. This patch can skip even creating the
> scrollbar layers but I didn't want to change the detachScrollbarLayer logic.
> 
> We could remove the DCHECK in setupScrollbarLayer that checks for a scrollbar
> layer and call setupScrollbarLayer. I think it's a little cleaner to just make
> the early-out behavior more obvious. I don't really think we even need to call
> detachScrollbarLayer but I didn't want to tackle that change in this patch.

Ok, thanks for the explanation.

[ajuma, 2017-03-06 22:06:30]

lgtm

[pdr., 2017-03-06 22:51:40]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2017-03-06 22:51:42]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-03-06 22:52:18]

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-07 02:34:30]

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

[commit-bot: I haz the power, 2017-03-07 02:34:31]

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-03-07 08:06:13]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-03-07 08:06:40]

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-07 11:14:34]

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

[commit-bot: I haz the power, 2017-03-07 11:14:35]

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-03-07 15:02:26]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-03-07 15:03:05]

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

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488898946178740,
"parent_rev": "2e97347bcd54763ed7e4b3a692537bd8304975d7", "commit_rev":
"a20ad732333dc9db2b545886acd16a11abdc5842"}

[commit-bot: I haz the power, 2017-03-07 17:12:02]

Description was changed from

==========
Remove indirection: setup scrollbar scroll layers in the scrollbar constructor

Scrollbar layers keep track of the layer id (soon: element id) that they
actually scroll. Before this patch, scrollbar layers were constructed
with no scroll layer id, then later updated to set the scroll layer id.
This patch removes this indirection and directly sets the scroll layer
in the constructor.

Removed functions:
  VisualViewport::setScrollLayerOnScrollbars
  WebScrollbarLayer::setScrollLayer
  WebScrollbarLayerImpl::setScrollLayer
  PaintedOverlayScrollbarLayer::SetScrollLayer
  PaintedScrollbarLayer::SetScrollLayer
  SolidColorScrollbarLayer::SetScrollLayer

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

to

==========
Remove indirection: setup scrollbar scroll layers in the scrollbar constructor

Scrollbar layers keep track of the layer id (soon: element id) that they
actually scroll. Before this patch, scrollbar layers were constructed
with no scroll layer id, then later updated to set the scroll layer id.
This patch removes this indirection and directly sets the scroll layer
in the constructor.

Removed functions:
  VisualViewport::setScrollLayerOnScrollbars
  WebScrollbarLayer::setScrollLayer
  WebScrollbarLayerImpl::setScrollLayer
  PaintedOverlayScrollbarLayer::SetScrollLayer
  PaintedScrollbarLayer::SetScrollLayer
  SolidColorScrollbarLayer::SetScrollLayer

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

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

[commit-bot: I haz the power, 2017-03-07 17:12:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a20ad732333dc9db2b545886acd1...

[pdr., 2017-03-09 17:52:05]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2743653003/ by pdr@chromium.org.

The reason for reverting is: Regressed scrolling performance:
https://crbug.com/699705.