Remove scroll_clip_layer dependency from viewport layer tracking

Remove scroll_clip_layer dependency from viewport layer tracking

This patch prepares for removing scroll_clip_layer by removing uses of
scroll_clip_layer from the viewport layers. All 4 viewport layers are now
tracked explicitly instead of just tracking the scroll layers and using
scroll_clip_layer() to lookup the container layers.

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

Review-Url: https://codereview.chromium.org/2887053002
Cr-Commit-Position: refs/heads/master@{#472630}
Committed: https://chromium.googlesource.com/chromium/src/+/9bb53af48ddd915e3b94a145722ad9e2d6650fc8

[pdr., 2017-05-17 03:50:48]

Description was changed from

==========
Remove scroll_clip_layer dependency from viewport layer tracking

This patch prepares for removing scroll_clip_layer by removing uses of
scroll_clip_layer from the viewport layers, instead tracking the clip and
container layers explicitly.

BUG=723263
==========

to

==========
Remove scroll_clip_layer dependency from viewport layer tracking

This patch prepares for removing scroll_clip_layer by removing uses of
scroll_clip_layer from the viewport layers, instead tracking the clip and
container layers explicitly.

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

[pdr., 2017-05-17 18:05:21]

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

[commit-bot: I haz the power, 2017-05-17 18:07:18]

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-05-17 18:20:36]

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

[commit-bot: I haz the power, 2017-05-17 18:20:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pdr., 2017-05-17 18:43:20]

Description was changed from

==========
Remove scroll_clip_layer dependency from viewport layer tracking

This patch prepares for removing scroll_clip_layer by removing uses of
scroll_clip_layer from the viewport layers, instead tracking the clip and
container layers explicitly.

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

to

==========
Remove scroll_clip_layer dependency from viewport layer tracking

This patch prepares for removing scroll_clip_layer by removing uses of
scroll_clip_layer from the viewport layers. All 4 viewport layers are now
tracked explicitly instead of just tracking the scroll layers and using
scroll_clip_layer() to lookup the container layers.

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

[pdr., 2017-05-17 18:48:12]

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

[pdr., 2017-05-17 18:48:24]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, enne@chromium.org, wkorman@chromium.org

[commit-bot: I haz the power, 2017-05-17 18:49:21]

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-05-17 19:06:00]

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

[commit-bot: I haz the power, 2017-05-17 19:06:01]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2017-05-17 19:16:03]

lgtm

[chrishtr, 2017-05-17 19:19:12]

Cool!

[pdr., 2017-05-17 20:28:41]

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

[commit-bot: I haz the power, 2017-05-17 20:30:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-05-17 20:48:02]

On 2017/05/17 at 19:19:12, chrishtr wrote:
> Cool!

Latest patch has a bunch of Web* updates that I missed before because I was only
running cc_unittests. PTAL

[wkorman, 2017-05-17 20:59:39]

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

https://codereview.chromium.org/2887053002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:244: void RegisterViewportLayers(
For consideration -- with 6 params it's nearing point where something like a
struct with named fields that we pass in may be worthwhile for readability.
Maybe we could even just store the struct as member data then, and save a bunch
of duplicative field definitions. Maybe would simplify the INVALID_ID we have to
fall back to everywhere. Could have one which stores layers, another which
stores layer ids, and able to get one from the other.

Anyway not needed for this change but wanted to mention while reading.

Also, if the full set of these layers is going to continue to be around for a
while, is there an ascii diagram or doc we can reference with more info for
folks who want to see what all the layers are?

I'm thinking something like what we have in:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...

I feel like there was another better one somewhere in Blink but not remembering
where.

https://codereview.chromium.org/2887053002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp
(right):

https://codereview.chromium.org/2887053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:208:

Can shorten below here to just:

return area ? area->LayerForContainer() : nullptr;

[pdr., 2017-05-17 21:36:44]

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

https://codereview.chromium.org/2887053002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:244: void RegisterViewportLayers(
On 2017/05/17 at 20:59:39, wkorman wrote:
> For consideration -- with 6 params it's nearing point where something like a
struct with named fields that we pass in may be worthwhile for readability.
Maybe we could even just store the struct as member data then, and save a bunch
of duplicative field definitions. Maybe would simplify the INVALID_ID we have to
fall back to everywhere. Could have one which stores layers, another which
stores layer ids, and able to get one from the other.
> 
> Anyway not needed for this change but wanted to mention while reading.
> 
> Also, if the full set of these layers is going to continue to be around for a
while, is there an ascii diagram or doc we can reference with more info for
folks who want to see what all the layers are?
> 
> I'm thinking something like what we have in:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> 
> I feel like there was another better one somewhere in Blink but not
remembering where.

Yeah I wish there was a diagram :( I am still learning about these layers and
will need to fully understand them when we're done. This patch just makes the
implicit layer dependencies explicit.

Mind if I file the 6 params -> struct as a followup bug?

https://codereview.chromium.org/2887053002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp
(right):

https://codereview.chromium.org/2887053002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/TopDocumentRootScrollerController.cpp:208:

On 2017/05/17 at 20:59:39, wkorman wrote:
> Can shorten below here to just:
> 
> return area ? area->LayerForContainer() : nullptr;

Done

[wkorman, 2017-05-17 21:45:50]

lgtm

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

https://codereview.chromium.org/2887053002/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:244: void RegisterViewportLayers(
On 2017/05/17 21:36:44, pdr. wrote:
> On 2017/05/17 at 20:59:39, wkorman wrote:
> > For consideration -- with 6 params it's nearing point where something like a
> struct with named fields that we pass in may be worthwhile for readability.
> Maybe we could even just store the struct as member data then, and save a
bunch
> of duplicative field definitions. Maybe would simplify the INVALID_ID we have
to
> fall back to everywhere. Could have one which stores layers, another which
> stores layer ids, and able to get one from the other.
> > 
> > Anyway not needed for this change but wanted to mention while reading.
> > 
> > Also, if the full set of these layers is going to continue to be around for
a
> while, is there an ascii diagram or doc we can reference with more info for
> folks who want to see what all the layers are?
> > 
> > I'm thinking something like what we have in:
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> > 
> > I feel like there was another better one somewhere in Blink but not
> remembering where.
> 
> Yeah I wish there was a diagram :( I am still learning about these layers and
> will need to fully understand them when we're done. This patch just makes the
> implicit layer dependencies explicit.
> 
> Mind if I file the 6 params -> struct as a followup bug?

Sure, sounds good.

[chrishtr, 2017-05-17 21:47:40]

lgtm

[pdr., 2017-05-17 21:48:02]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-05-17 21:48:03]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2887053002/#ps100001
(title: "Cleanup return stmt")

[commit-bot: I haz the power, 2017-05-17 21:49:20]

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-05-18 03:25:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495057682921860,
"parent_rev": "6400c720a346d268183f659158691adfdeeb7a7a", "commit_rev":
"e9012d228d439a489a2850f3470926a37a658199"}

[commit-bot: I haz the power, 2017-05-18 03:25:36]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495057682921860,
"parent_rev": "019e8abea199c9aeeddc38362b9b75eb1ce6fe00", "commit_rev":
"9bb53af48ddd915e3b94a145722ad9e2d6650fc8"}

[commit-bot: I haz the power, 2017-05-18 03:25:47]

Description was changed from

==========
Remove scroll_clip_layer dependency from viewport layer tracking

This patch prepares for removing scroll_clip_layer by removing uses of
scroll_clip_layer from the viewport layers. All 4 viewport layers are now
tracked explicitly instead of just tracking the scroll layers and using
scroll_clip_layer() to lookup the container layers.

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

to

==========
Remove scroll_clip_layer dependency from viewport layer tracking

This patch prepares for removing scroll_clip_layer by removing uses of
scroll_clip_layer from the viewport layers. All 4 viewport layers are now
tracked explicitly instead of just tracking the scroll layers and using
scroll_clip_layer() to lookup the container layers.

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

Review-Url: https://codereview.chromium.org/2887053002
Cr-Commit-Position: refs/heads/master@{#472630}
Committed:
https://chromium.googlesource.com/chromium/src/+/9bb53af48ddd915e3b94a145722a...
==========

[commit-bot: I haz the power, 2017-05-18 03:25:48]

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