Set layer element id when building layers in PaintArtifactCompositor. (issue 2637383006 by wkorman@chromium.org)

[wko...@chromium.org]

Reviewers: pdr., ajuma
CL: https://codereview.chromium.org/2637383006/

Message:
Note dependent patchset. Will write unit test after initial comment.


https://codereview.chromium.org/2637383006/diff/1/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
File
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
(right):

https://codereview.chromium.org/2637383006/diff/1/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h#newcode74
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:74:
const CompositorElementId compositorElementId() const;
We could put this logic in PaintArtifactCompositor as a static method
but seemed cleaner this way, open to thoughts though.

Description:
Set layer element id when building layers in PaintArtifactCompositor.

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

Affected files (+19, -0 lines):
M third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
M third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
M third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp


Index: third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
index 7996e52360c5f4861153406cdb1cd12d0f0a23cb..93aaafc46e6c99276f846c987130a86c4b4b00d1 100644
--- a/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
+++ b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
@@ -720,6 +720,7 @@ void PaintArtifactCompositor::update(
propertyTreeManager.updateScrollOffset(layer->id(), scrollId);

layer->set_offset_to_transform_parent(layerOffset);
+ layer->SetElementId(pendingLayer.propertyTreeState.compositorElementId());

m_rootLayer->AddChild(layer);
layer->set_property_tree_sequence_number(
Index: third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp b/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
index 8f2317a75fc1dba07ef262829306ce000d3fb0de..c20a0f0cfdafbb0b30d11b613b67d0758e711c92 100644
--- a/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
+++ b/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
@@ -29,6 +29,19 @@ bool isAncestorOf(const PropertyNode* ancestor, const PropertyNode* child) {
return child == ancestor;
}

+const CompositorElementId PropertyTreeState::compositorElementId() const {
+ // Zero or more of the scroll, effect or transform nodes could have a
+ // compositor element id. The order doesn't matter as the element id should be
+ // the same on all that have a non-default CompositorElementId.
+ if (effect()->compositorElementId())
+ return effect()->compositorElementId();
+ if (scroll()->compositorElementId())
+ return scroll()->compositorElementId();
+ if (transform()->compositorElementId())
+ return transform()->compositorElementId();
+ return CompositorElementId();
+}
+
PropertyTreeState::InnermostNode PropertyTreeState::innermostNode() const {
// TODO(chrishtr): this is very inefficient when innermostNode() is called
// repeatedly.
Index: third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
diff --git a/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h b/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
index 19555592996153b78883df9520f1af9cc9f6b1c2..7bcf40dcc8b13cd320a46b07f80ac56b58213137 100644
--- a/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
+++ b/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
@@ -68,6 +68,11 @@ class PLATFORM_EXPORT PropertyTreeState {
m_scroll = std::move(node);
}

+ // Returns the compositor element id, if any, for this property state. If none
+ // of the scroll, effect or transform nodes for this state have a compositor
+ // element id then a default instance is returned.
+ const CompositorElementId compositorElementId() const;
+
enum InnermostNode {
None, // None means that all nodes are their root values
Transform,

[aj...@chromium.org]

https://codereview.chromium.org/2637383006/diff/1/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
File
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
(right):

https://codereview.chromium.org/2637383006/diff/1/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp#newcode35
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:35:

// the same on all that have a non-default CompositorElementId.

Is it worth DCHECK-ing that this holds?

https://codereview.chromium.org/2637383006/

[wko...@chromium.org]

PTAL, added tests.

https://codereview.chromium.org/2637383006/diff/1/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
File
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
(right):

https://codereview.chromium.org/2637383006/diff/1/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp#newcode35
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp:35:
// the same on all that have a non-default CompositorElementId.

On 2017/01/20 at 15:07:46, ajuma wrote:
> Is it worth DCHECK-ing that this holds?

Done.

https://codereview.chromium.org/2637383006/

[aj...@chromium.org]

Thanks, lgtm

https://codereview.chromium.org/2637383006/

[p...@chromium.org]

On 2017/01/20 at 23:25:45, ajuma wrote:
> Thanks, lgtm

LGTM

https://codereview.chromium.org/2637383006/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2637383006/60001


https://codereview.chromium.org/2637383006/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4243922107810fd1644fafa33cd8329ed1616515

https://codereview.chromium.org/2637383006/