[SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer id.

[SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer id.

This means we will always have an element id in cc property trees, which means
we can rely on it as a key for node lookup instead of layer id.

BUG=718564, 723099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2890953002
Cr-Commit-Position: refs/heads/master@{#473085}
Committed: https://chromium.googlesource.com/chromium/src/+/18fc5e473594ff6bb062183835a60257def9a627

[chrishtr, 2017-05-17 21:39:05]

Description was changed from

==========
none


none


none

BUG=
==========

to

==========
none


none


none

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

[chrishtr, 2017-05-17 21:42:25]

Description was changed from

==========
none


none


none

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

to

==========
[SPv1] Always set a CompositorElementId on main graphics layers; use fragment
id.

This means we will always have an element id in cc property trees, which means
we can rely on it as a key for node lookup instead of layer id.

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

[chrishtr, 2017-05-17 23:10:33]

Description was changed from

==========
[SPv1] Always set a CompositorElementId on main graphics layers; use fragment
id.

This means we will always have an element id in cc property trees, which means
we can rely on it as a key for node lookup instead of layer id.

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

to

==========
[SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer
id.

This means we will always have an element id in cc property trees, which means
we can rely on it as a key for node lookup instead of layer id.

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

[chrishtr, 2017-05-17 23:50:44]

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

[chrishtr, 2017-05-17 23:50:49]

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

[commit-bot: I haz the power, 2017-05-17 23:51:55]

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-18 00:45:59]

lgtm

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:756: if
(target_element->GetLayoutObject() &&
nit: pulling target_element->GetLayoutObject() into local var above can shorten
4 refs here and below

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/Animation.cpp:766: } else {
nit: maybe more readable if we add

if (!composited_element_ids.has_value())
  return false;

at start of block and rm else.

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (left):

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:448: if
(!element.GetLayoutObject() ||
nit: consider local var for element.GetLayoutObject() to shorten refs

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:1028: // A id for this
PaintLayer that is unique for the lifetime of the WebView.
A -> An

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:75: static
CompositorElementId CreateDomNodeBasedCompositorElementId(
Suggest rename to CreatePaintLayerBasedCompositorElementId() or something, to
match logic change.

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:77: //
TODO(wkorman): Centralize this implementation with similar across
Maybe we can remove this now. It's not looking to me like we'd want to share
more than we do currently, but open to thoughts.

I mean, maybe there are a few other places where we use
ToLayoutBoxModelObject(object).Layer()->UniqueId(), and we could move this
method somewhere in CompositorElementId.h and have all of those use it, but not
sure it's worth it.

When I wrote the TODO I think we were using DOMNodeIds everywhere when creating
a CompositorElementId.

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1199: if
(RuntimeEnabledFeatures::slimmingPaintV2Enabled() && object.HasLayer()) {
This means we will stop setting compositor element ids on transform/effect nodes
in SPv2 when the involved layout object has no PaintLayer, right?

I think it also means that the majority of element ids we see in SPv2 will now
be paint layer ids rather than dom node ids. I only mention this because it
means we'll stop pounding the WeakIdentifierMap for DOM node ids.

Both seem fine, just noting while reading. Suggest noting both in CL
description. It currently only mentions SPv1 logic changes. Changes here still
have no ramification for SPv1 despite spinvalidation, right?

Do currently-passing animation layout tests in SPv2 still pass with the change?

[commit-bot: I haz the power, 2017-05-18 01:12:22]

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

[commit-bot: I haz the power, 2017-05-18 01:12:23]

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-18 01:25:47]

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:1028: // A id for this
PaintLayer that is unique for the lifetime of the WebView.
On 2017/05/18 at 00:45:58, wkorman wrote:
> A -> An

Done.

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (left):

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:75: static
CompositorElementId CreateDomNodeBasedCompositorElementId(
On 2017/05/18 at 00:45:58, wkorman wrote:
> Suggest rename to CreatePaintLayerBasedCompositorElementId() or something, to
match logic change.

Done.

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:77: //
TODO(wkorman): Centralize this implementation with similar across
On 2017/05/18 at 00:45:58, wkorman wrote:
> Maybe we can remove this now. It's not looking to me like we'd want to share
more than we do currently, but open to thoughts.
> 
> I mean, maybe there are a few other places where we use
ToLayoutBoxModelObject(object).Layer()->UniqueId(), and we could move this
method somewhere in CompositorElementId.h and have all of those use it, but not
sure it's worth it.
> 
> When I wrote the TODO I think we were using DOMNodeIds everywhere when
creating a CompositorElementId.

Done.

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:1199: if
(RuntimeEnabledFeatures::slimmingPaintV2Enabled() && object.HasLayer()) {
On 2017/05/18 at 00:45:58, wkorman wrote:
> This means we will stop setting compositor element ids on transform/effect
nodes in SPv2 when the involved layout object has no PaintLayer, right?

No, because transforms and effects all induce PaintLayers. See:

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

> 
> I think it also means that the majority of element ids we see in SPv2 will now
be paint layer ids rather than dom node ids. I only mention this because it
means we'll stop pounding the WeakIdentifierMap for DOM node ids.

Yes. This is the reason I made the WeakIdentifierMap at the same time as
setting it unconditionally in SPv1 mode - to avoid a perf regression.

> 
> Both seem fine, just noting while reading. Suggest noting both in CL
description. It currently only mentions SPv1 logic changes. Changes here still
have no ramification for SPv1 despite spinvalidation, right?
> 
> Do currently-passing animation layout tests in SPv2 still pass with the
change?

Haven't checked. Let's see what CQ dry run says.

[chrishtr, 2017-05-18 01:45:01]

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

[commit-bot: I haz the power, 2017-05-18 01:46:44]

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-18 03:01:39]

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

[commit-bot: I haz the power, 2017-05-18 03:01:40]

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

[pdr., 2017-05-18 03:32:25]

Looks reasonable to me too.

LGTM

[chrishtr, 2017-05-18 21:22:04]

Updated to still use DOM node ids for layers that are controlled by compsitor
workers,
because CompositorProxy objects can be kept alive by script and need to
reference ids
that are stable across changes to layout, which may delete and add PaintLayers.

[chrishtr, 2017-05-18 21:22:07]

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

[commit-bot: I haz the power, 2017-05-18 21:23:13]

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

lgtm

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2203:
Persistent<Node> owning_node = nullptr;
add STACK_ALLOCATED()? seems to be MO.

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2269:
element_id = CompositorElementIdFromPaintLayerId(
Perhaps worth adding a comment here re: why we prefer using paint layer ids.
Could mention (1) fragmentation, (2) desire to avoid dom id map overhead.

Or could add the comment to where we define CompositorElementIdFromPaintLayerId
since we have another callsite below. You have the inverse on the dom node id
method definition (noting it should be used only in exceptional situations).

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h
(right):

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h:10:
#include "platform/PlatformExport.h"
Do we need this and next line? Looks like maybe just need something in .cpp
file.

[pdr., 2017-05-18 21:47:52]

On 2017/05/18 at 21:22:04, chrishtr wrote:
> Updated to still use DOM node ids for layers that are controlled by compsitor
workers,
> because CompositorProxy objects can be kept alive by script and need to
reference ids
> that are stable across changes to layout, which may delete and add
PaintLayers.

LGTM

[chrishtr, 2017-05-18 22:15:31]

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2203:
Persistent<Node> owning_node = nullptr;
On 2017/05/18 at 21:31:57, wkorman wrote:
> add STACK_ALLOCATED()? seems to be MO.

Done.

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2269:
element_id = CompositorElementIdFromPaintLayerId(
On 2017/05/18 at 21:31:57, wkorman wrote:
> Perhaps worth adding a comment here re: why we prefer using paint layer ids.
Could mention (1) fragmentation, (2) desire to avoid dom id map overhead.
> 
> Or could add the comment to where we define
CompositorElementIdFromPaintLayerId since we have another callsite below. You
have the inverse on the dom node id method definition (noting it should be used
only in exceptional situations).

Done.

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h
(right):

https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.h:10:
#include "platform/PlatformExport.h"
On 2017/05/18 at 21:31:57, wkorman wrote:
> Do we need this and next line? Looks like maybe just need something in .cpp
file.

Fixed. We do need line 10, but moved it to its old location.

[chrishtr, 2017-05-18 22:21:22]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-18 22:21:22]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2890953002/#ps200001
(title: "none")

[commit-bot: I haz the power, 2017-05-18 22:23:15]

CQ is trying da patch.

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

[chrishtr, 2017-05-18 22:24:17]

Description was changed from

==========
[SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer
id.

This means we will always have an element id in cc property trees, which means
we can rely on it as a key for node lookup instead of layer id.

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

to

==========
[SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer
id.

This means we will always have an element id in cc property trees, which means
we can rely on it as a key for node lookup instead of layer id.

BUG=718564,723099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-05-19 00:05:50]

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

[commit-bot: I haz the power, 2017-05-19 00:05:52]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrishtr, 2017-05-19 03:26:37]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-05-19 03:27: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-19 04:21:04]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495164397717870,
"parent_rev": "76d8ea110fdf384fcc37c37faab9a0a08f76160c", "commit_rev":
"18fc5e473594ff6bb062183835a60257def9a627"}

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

Description was changed from

==========
[SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer
id.

This means we will always have an element id in cc property trees, which means
we can rely on it as a key for node lookup instead of layer id.

BUG=718564,723099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer
id.

This means we will always have an element id in cc property trees, which means
we can rely on it as a key for node lookup instead of layer id.

BUG=718564,723099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2890953002
Cr-Commit-Position: refs/heads/master@{#473085}
Committed:
https://chromium.googlesource.com/chromium/src/+/18fc5e473594ff6bb062183835a6...
==========

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

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/18fc5e473594ff6bb062183835a6...