Description was changed from ========== none none none BUG= ========== to ========== none none none ...
3 years, 7 months ago
(2017-05-17 21:39:05 UTC)
#1
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
Description was changed from ========== none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPv1] Always ...
3 years, 7 months ago
(2017-05-17 21:42:25 UTC)
#2
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
Description was changed from ========== [SPv1] Always set a CompositorElementId on main graphics layers; use ...
3 years, 7 months ago
(2017-05-17 23:10:33 UTC)
#3
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
==========
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_ng/builds/456710)
3 years, 7 months ago
(2017-05-18 01:12:23 UTC)
#10
https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Source/core/paint/PaintLayer.h File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2890953002/diff/60001/third_party/WebKit/Source/core/paint/PaintLayer.h#newcode1028 third_party/WebKit/Source/core/paint/PaintLayer.h:1028: // A id for this PaintLayer that is unique ...
3 years, 7 months ago
(2017-05-18 01:25:47 UTC)
#11
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
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
3 years, 7 months ago
(2017-05-18 01:45:01 UTC)
#12
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_asan_rel_ng/builds/373632)
3 years, 7 months ago
(2017-05-18 03:01:40 UTC)
#15
3 years, 7 months ago
(2017-05-18 03:32:25 UTC)
#16
Looks reasonable to me too.
LGTM
chrishtr
Updated to still use DOM node ids for layers that are controlled by compsitor workers, ...
3 years, 7 months ago
(2017-05-18 21:22:04 UTC)
#17
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
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
3 years, 7 months ago
(2017-05-18 21:22:07 UTC)
#18
On 2017/05/18 at 21:22:04, chrishtr wrote: > Updated to still use DOM node ids for ...
3 years, 7 months ago
(2017-05-18 21:47:52 UTC)
#21
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
https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2890953002/diff/120001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode2203 third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2203: Persistent<Node> owning_node = nullptr; On 2017/05/18 at 21:31:57, wkorman ...
3 years, 7 months ago
(2017-05-18 22:15:31 UTC)
#22
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
The CQ bit was checked by chrishtr@chromium.org
3 years, 7 months ago
(2017-05-18 22:21:22 UTC)
#23
Description was changed from ========== [SPv1] Always set a CompositorElementId on main graphics layers; use ...
3 years, 7 months ago
(2017-05-18 22:24:17 UTC)
#26
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
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 7 months ago
(2017-05-19 00:05:50 UTC)
#27
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_android_rel_ng/builds/297687)
3 years, 7 months ago
(2017-05-19 00:05:52 UTC)
#28
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495164397717870, "parent_rev": "76d8ea110fdf384fcc37c37faab9a0a08f76160c", "commit_rev": "18fc5e473594ff6bb062183835a60257def9a627"}
3 years, 7 months ago
(2017-05-19 04:21:04 UTC)
#31
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
Description was changed from ========== [SPv1] Always set a CompositorElementId on main graphics layers; use ...
3 years, 7 months ago
(2017-05-19 04:21:16 UTC)
#32
Message was sent while issue was closed.
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
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/18fc5e473594ff6bb062183835a60257def9a627
3 years, 7 months ago
(2017-05-19 04:21:19 UTC)
#33
Issue 2890953002: [SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer id.
(Closed)
Created 3 years, 7 months ago by chrishtr
Modified 3 years, 7 months ago
Reviewers: pdr., wkorman
Base URL:
Comments: 17