Manage registering composited elements in PaintArtifactCompositor.

Manage registering composited elements in PaintArtifactCompositor.

Circumvent three references to element id via the Layer when
SPv2 is enabled. PaintArtifactCompositor instead directly
handles layer registration/unregistration with the layer tree
host, and setting the host to commit in presence of animations.

BUG=709137
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/2888483002
Cr-Commit-Position: refs/heads/master@{#472703}
Committed: https://chromium.googlesource.com/chromium/src/+/e006aee237d27eb7dbefae03551121dbad59dcae

[wkorman, 2017-05-16 01:46:49]

Description was changed from

==========
Manage registering composited elements in PaintArtifactCompositor.

BUG=709137
==========

to

==========
Manage registering composited elements in PaintArtifactCompositor.

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

[wkorman, 2017-05-16 01:46:50]

Description was changed from

==========
Manage registering composited elements in PaintArtifactCompositor.

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

to

==========
Manage registering composited elements in PaintArtifactCompositor.

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

[wkorman, 2017-05-16 01:49:26]

Description was changed from

==========
Manage registering composited elements in PaintArtifactCompositor.

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

to

==========
Manage registering composited elements in PaintArtifactCompositor.

Removes several references to Layer::inputs_.element_id in SPv2.

In SPv2, PaintArtifactCompositor has full knowledge of
compositor element ids. We can therefore handle registration
directly with the id as retrieved from paint property trees.

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

[wkorman, 2017-05-16 01:50:09]

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

[commit-bot: I haz the power, 2017-05-16 01:51:15]

Dry run: CQ is trying da patch.

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

[wkorman, 2017-05-16 01:52:25]

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

[wkorman, 2017-05-16 01:52:26]

Thinking something like this. Will write tests after
initial feedback and CQ results.

We can't skip the calls to RegisterLayer/UnregisterLayer in
Layer::SetLayerTreeHost until pdr@ finishes with a few remaining scroll
references to LayerById:

https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host.cc?l=1051

[commit-bot: I haz the power, 2017-05-16 01:56:06]

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

[commit-bot: I haz the power, 2017-05-16 01:56:08]

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...)

[pdr., 2017-05-16 02:46:35]

This code looks good to me, just a couple questions about the overall design.

https://codereview.chromium.org/2888483002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2888483002/diff/1/cc/layers/layer.cc#newcode157
cc/layers/layer.cc:157: if (!use_layer_lists) {
can this be simplified like:
if (host && host->GetSettings().use_layer_lists) {
  if (GetMutatorHost()->HasAnyAnimation(element_id()))
    host->SetNeedsCommit();
}

https://codereview.chromium.org/2888483002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2888483002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:660:
auto previous_host = layer->layer_tree_host();
What does it mean to change layer tree hosts? In my mental model the host is
sort of the controller... would we change it in SPV2 (or even SPV1?)

https://codereview.chromium.org/2888483002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:669:
if (host->mutator_host()->HasAnyAnimation(element_id))
Do we set animations on the animation host yet?

[wkorman, 2017-05-16 21:42:07]

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

[wkorman, 2017-05-16 21:42:42]

Still working on tests but sending replies to current comment set.

https://codereview.chromium.org/2888483002/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2888483002/diff/1/cc/layers/layer.cc#newcode157
cc/layers/layer.cc:157: if (!use_layer_lists) {
On 2017/05/16 02:46:35, pdr. wrote:
> can this be simplified like:
> if (host && host->GetSettings().use_layer_lists) {
>   if (GetMutatorHost()->HasAnyAnimation(element_id()))
>     host->SetNeedsCommit();
> }

Done and combined the two if's into one with an add'l &.

https://codereview.chromium.org/2888483002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2888483002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:660:
auto previous_host = layer->layer_tree_host();
On 2017/05/16 02:46:35, pdr. wrote:
> What does it mean to change layer tree hosts? In my mental model the host is
> sort of the controller... would we change it in SPV2 (or even SPV1?)

It appears that the LayerTreeHost of a layer is updated as layers move between
trees at least in SPv1 according to comment here:

https://cs.chromium.org/chromium/src/cc/layers/layer.h?gsn=Create&l=600

SetLayerTreeHost is critical path for maintaining a LayerTreeHost's knowledge of
the layers it's controlling. It's called by Layer::{Add,Insert}Child when they
call SetParent on the Layer.

In SPv2 we create a single root layer and reuse, and that layer appears to have
a stable LayerTreeHost for the duration of a single page.

We remove all layers from the root layer (see RemoveAllChildren() call above),
and re-add, each time PaintArtifactCompositor::Update() is called.
RemoveAllChildren ends up calling SetParent(nullptr) on the layer, which will
un-register it from its LayerTreeHost.

So, we're un-registering and then re-registering all child layers every time
with the same LayerTreeHost which does seem a little silly. I need to understand
LayerTreeHost and LayerTree lifecycle better to consider optimization
opportunities. Maybe let's look at in person whenever works for you?

https://codereview.chromium.org/2888483002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:669:
if (host->mutator_host()->HasAnyAnimation(element_id))
On 2017/05/16 02:46:35, pdr. wrote:
> Do we set animations on the animation host yet?

If I understand your question correctly, given that most animation test cases
are operational, this must be the case. Active animations end up being added as
"ticking players":

https://cs.chromium.org/chromium/src/cc/animation/animation_host.h?q=animatio...

And are ticked as part of LayerTreeHost{Impl}::Animate().

Adding of compositor animations happens through this code path from Blink into
cc:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/anima...

[commit-bot: I haz the power, 2017-05-16 21:43:22]

Dry run: CQ is trying da patch.

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

[wkorman, 2017-05-16 22:04:34]

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

[commit-bot: I haz the power, 2017-05-16 22:06:00]

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-16 23:45:40]

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

[commit-bot: I haz the power, 2017-05-16 23:45:41]

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

[chrishtr, 2017-05-17 00:39:02]

Re "Removes several references to Layer::inputs_.elmeent_id in SPv2", which ones
are they?
Add a DCHECK?

[wkorman, 2017-05-17 01:33:55]

On 2017/05/17 00:39:02, chrishtr wrote:
> Re "Removes several references to Layer::inputs_.elmeent_id in SPv2", which
ones
> are they?
> Add a DCHECK?

Well, maybe "circumvents" is a better word than "removes", will edit
description.

In the layer.cc change here, there are two refs to inputs_.element_id and one to
element_id() that are skipped if settings.use_layer_lists is true. We still
execute similar logic, we just call it from PAC directly, so I am not sure what
would be useful to DCHECK but open to thoughts.

[wkorman, 2017-05-17 01:38:36]

Description was changed from

==========
Manage registering composited elements in PaintArtifactCompositor.

Removes several references to Layer::inputs_.element_id in SPv2.

In SPv2, PaintArtifactCompositor has full knowledge of
compositor element ids. We can therefore handle registration
directly with the id as retrieved from paint property trees.

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

to

==========
Manage registering composited elements in PaintArtifactCompositor.

Circumvent three references to element id via the Layer when
SPv2 is enabled. PaintArtifactCompositor instead directly
handles layer registration/unregistration with the layer tree
host, and setting the host to commit in presence of animations.

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

[chrishtr, 2017-05-17 03:05:45]

On 2017/05/17 at 01:33:55, wkorman wrote:
> On 2017/05/17 00:39:02, chrishtr wrote:
> > Re "Removes several references to Layer::inputs_.elmeent_id in SPv2", which
ones
> > are they?
> > Add a DCHECK?
> 
> Well, maybe "circumvents" is a better word than "removes", will edit
description.
> 
> In the layer.cc change here, there are two refs to inputs_.element_id and one
to element_id() that are skipped if settings.use_layer_lists is true. We still
execute similar logic, we just call it from PAC directly, so I am not sure what
would be useful to DCHECK but open to thoughts.

Oh, ok. I missed the changes in layer.cc somehow.

[chrishtr, 2017-05-17 03:05:50]

On 2017/05/17 at 01:33:55, wkorman wrote:
> On 2017/05/17 00:39:02, chrishtr wrote:
> > Re "Removes several references to Layer::inputs_.elmeent_id in SPv2", which
ones
> > are they?
> > Add a DCHECK?
> 
> Well, maybe "circumvents" is a better word than "removes", will edit
description.
> 
> In the layer.cc change here, there are two refs to inputs_.element_id and one
to element_id() that are skipped if settings.use_layer_lists is true. We still
execute similar logic, we just call it from PAC directly, so I am not sure what
would be useful to DCHECK but open to thoughts.

Oh, ok. I missed the changes in layer.cc somehow.

[wkorman, 2017-05-17 20:07:50]

PTAL, added tests.

https://codereview.chromium.org/2888483002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2888483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:668:
if (host->mutator_host()->HasAnyAnimation(element_id))
Believe we actually don't need to do this in SPv2, removed in latest patch set.
We appear to SetNeedsCommit on the LTH every time Update is called.

[wkorman, 2017-05-17 20:07:55]

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

[commit-bot: I haz the power, 2017-05-17 20:08:57]

Dry run: CQ is trying da patch.

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

[chrishtr, 2017-05-17 20:15:35]

lgtm

[pdr., 2017-05-17 20:17:33]

OK LGTM 2

[commit-bot: I haz the power, 2017-05-17 21:00:44]

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

[commit-bot: I haz the power, 2017-05-17 21:00:45]

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

[wkorman, 2017-05-17 21:53:31]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-05-17 21:53:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2888483002/#ps100001
(title: "Sync to head and disable flaky animation test.")

[commit-bot: I haz the power, 2017-05-17 21:54:55]

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 02:59:12]

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

[commit-bot: I haz the power, 2017-05-18 02:59:14]

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_...)

[wkorman, 2017-05-18 04:36:35]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-05-18 04:38:22]

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 06:19:01]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495082195708050,
"parent_rev": "2d3f81e4a7d9839d5183dbabf17655fbbf0e5cb7", "commit_rev":
"e006aee237d27eb7dbefae03551121dbad59dcae"}

[commit-bot: I haz the power, 2017-05-18 06:19:08]

Description was changed from

==========
Manage registering composited elements in PaintArtifactCompositor.

Circumvent three references to element id via the Layer when
SPv2 is enabled. PaintArtifactCompositor instead directly
handles layer registration/unregistration with the layer tree
host, and setting the host to commit in presence of animations.

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

to

==========
Manage registering composited elements in PaintArtifactCompositor.

Circumvent three references to element id via the Layer when
SPv2 is enabled. PaintArtifactCompositor instead directly
handles layer registration/unregistration with the layer tree
host, and setting the host to commit in presence of animations.

BUG=709137
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/2888483002
Cr-Commit-Position: refs/heads/master@{#472703}
Committed:
https://chromium.googlesource.com/chromium/src/+/e006aee237d27eb7dbefae035511...
==========

[commit-bot: I haz the power, 2017-05-18 06:19:09]

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