Check property tree state before direct compositing reasons. (issue 2644933006 by chrishtr@chromium.org)

[chri...@chromium.org]

Reviewers: trchen
CL: https://codereview.chromium.org/2644933006/

Description:
Check property tree state before direct compositing reasons.

Previously, we failed to merge a PaintChunk into a compatible one if the
compatible one had a direct compositing reason. That was an bug in the
way the code was implemented.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Affected files (+51, -2 lines):
M third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
M third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
M third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.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..4cd8dd570b283bae16154fc281d2b9cd56381889 100644
--- a/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
+++ b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
@@ -585,10 +585,10 @@ bool PaintArtifactCompositor::canMergeInto(
for (const PropertyTreeState* currentState =
&newChunk.properties.propertyTreeState;
currentState; currentState = iterator.next()) {
- if (currentState->hasDirectCompositingReasons())
- return false;
if (*currentState == candidatePendingLayer.propertyTreeState)
return true;
+ if (currentState->hasDirectCompositingReasons())
+ return false;
}
return false;
}
Index: third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
diff --git a/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
index 8ba32a7dfe4e1b1c862e3d8423a84afa2b84c06c..2e8fab41415215a513c9459907b460db62ca9a9d 100644
--- a/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
+++ b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
@@ -170,6 +170,8 @@ class PLATFORM_EXPORT PaintArtifactCompositor {
FRIEND_TEST_ALL_PREFIXES(PaintArtifactCompositorTestWithPropertyTrees,
Merge2DTransform);
FRIEND_TEST_ALL_PREFIXES(PaintArtifactCompositorTestWithPropertyTrees,
+ Merge2DTransformDirectAncestor);
+ FRIEND_TEST_ALL_PREFIXES(PaintArtifactCompositorTestWithPropertyTrees,
MergeTransformOrigin);
FRIEND_TEST_ALL_PREFIXES(PaintArtifactCompositorTestWithPropertyTrees,
MergeClip);
Index: third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
index da7f176c635bffc077ea634a1f69752921de5cea..720be04eb033865bc27b3bd59f30eba3624b3642 100644
--- a/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
+++ b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
@@ -879,6 +879,53 @@ TEST_F(PaintArtifactCompositorTestWithPropertyTrees, Merge2DTransform) {
}
}

+TEST_F(PaintArtifactCompositorTestWithPropertyTrees,
+ Merge2DTransformDirectAncestor) {
+ RefPtr<TransformPaintPropertyNode> transform =
+ TransformPaintPropertyNode::create(
+ TransformPaintPropertyNode::root(), TransformationMatrix(),
+ FloatPoint3D(), false, 0, CompositingReason3DTransform);
+
+ RefPtr<TransformPaintPropertyNode> transform2 =
+ TransformPaintPropertyNode::create(
+ transform.get(), TransformationMatrix().translate(50, 50),
+ FloatPoint3D(100, 100, 0), false, 0);
+
+ TestPaintArtifact testArtifact;
+ testArtifact
+ .chunk(transform.get(), ClipPaintPropertyNode::root(),
+ EffectPaintPropertyNode::root())
+ .rectDrawing(FloatRect(0, 0, 100, 100), Color::white);
+ // The second chunk can merge into the first because it has a descendant
+ // state of the first's transform and no direct compositing reason.
+ testArtifact
+ .chunk(transform2.get(), ClipPaintPropertyNode::root(),
+ EffectPaintPropertyNode::root())
+ .rectDrawing(FloatRect(0, 0, 100, 100), Color::black);
+
+ const PaintArtifact& artifact = testArtifact.build();
+
+ ASSERT_EQ(2u, artifact.paintChunks().size());
+ PaintArtifactCompositor::PendingLayer pendingLayer(artifact.paintChunks()[0]);
+ EXPECT_TRUE(PaintArtifactCompositor::canMergeInto(
+ artifact, artifact.paintChunks()[1], pendingLayer));
+
+ update(artifact);
+
+ ASSERT_EQ(1u, contentLayerCount());
+ {
+ Vector<RectWithColor> rectsWithColor;
+ rectsWithColor.push_back(
+ RectWithColor(FloatRect(0, 0, 100, 100), Color::white));
+ // Transform is applied to this PaintChunk.
+ rectsWithColor.push_back(
+ RectWithColor(FloatRect(50, 50, 100, 100), Color::black));
+
+ const cc::Layer* layer = contentLayerAt(0);
+ EXPECT_THAT(layer->GetPicture(), Pointee(drawsRectangles(rectsWithColor)));
+ }
+}
+
TEST_F(PaintArtifactCompositorTestWithPropertyTrees, MergeTransformOrigin) {
RefPtr<TransformPaintPropertyNode> transform =
TransformPaintPropertyNode::create(TransformPaintPropertyNode::root(),

[trc...@chromium.org]

lgtm



https://codereview.chromium.org/2644933006/

[chri...@chromium.org]

Updated to fix three tests. One had to make some adjustments to visual rects in
order to
not be (correctly) merged into fewer layers, one had different (correct again)
layerization,
and a third I added a direct reason to preserve the intent of the test.

https://codereview.chromium.org/2644933006/

[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/2644933006/20001


https://codereview.chromium.org/2644933006/

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

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_tests_slimming_paint_v2/builds/2513)

https://codereview.chromium.org/2644933006/

[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/2644933006/40001


https://codereview.chromium.org/2644933006/

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/209094f6afac678695e7061b0e02e3dc21eb7ebe

https://codereview.chromium.org/2644933006/