Use transform paint property nodes in SVG [spv2]

third_party/WebKit/Source/core/paint/SVGPaintContext.h

Index: third_party/WebKit/Source/core/paint/SVGPaintContext.h
diff --git a/third_party/WebKit/Source/core/paint/SVGPaintContext.h b/third_party/WebKit/Source/core/paint/SVGPaintContext.h
index d8f1a18e34726f37b63c6934cc496a714e4ae099..44ce1afd8c204518dc1acaa8fea573a6ee46943a 100644
--- a/third_party/WebKit/Source/core/paint/SVGPaintContext.h
+++ b/third_party/WebKit/Source/core/paint/SVGPaintContext.h
@@ -27,11 +27,14 @@
 
 #include "core/layout/svg/LayoutSVGResourceClipper.h"
 #include "core/layout/svg/LayoutSVGResourcePaintServer.h"
+#include "core/paint/ObjectPaintProperties.h"
 #include "core/paint/PaintInfo.h"
 #include "core/paint/SVGClipPainter.h"
 #include "core/paint/SVGFilterPainter.h"
+#include "core/paint/TransformRecorder.h"
 #include "platform/graphics/paint/ClipPathRecorder.h"
 #include "platform/graphics/paint/CompositingRecorder.h"
+#include "platform/graphics/paint/ScopedPaintChunkProperties.h"
 #include "platform/transforms/AffineTransform.h"
 
 namespace blink {
@@ -41,6 +44,45 @@ class LayoutSVGResourceFilter;
 class LayoutSVGResourceMasker;
 class SVGResources;
 
+// This class hooks up the correct paint property transform node when spv2 is enabled, and otherwise
+// works like a TransformRecorder which emits Transform display items for spv1.
+class SVGTransformContext : public TransformRecorder {
+    STACK_ALLOCATED();
+public:
+    SVGTransformContext(GraphicsContext& context, const LayoutObject& object, const AffineTransform& transform)
+        : TransformRecorder(context, object, transform)
+    {
+        if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
+            const auto* objectProperties = object.objectPaintProperties();
+            if (!objectProperties)
+                return;
+            if (object.isSVGRoot()) {
+                // If a transform exists, we can rely on a layer existing to apply it.
+                DCHECK(!objectProperties || !objectProperties->transform() || object.hasLayer());
+                if (objectProperties->svgLocalToBorderBoxTransform()) {

[trchen, 2016/06/13 22:45:18]

Is it possible to add a DCHECK here to verify the transform passed-in being
equivalent to the one we apply by property node? e.g.
DCHECK_EQ(objectProperties->svgLocalToBorderBoxTransform()->transform(),
transform);

[pdr., 2016/06/13 23:03:19]

These will actually be different because the svg root needs to support two
independent transform spaces (see SVGRootLocalToBorderBoxTransformNode in
https://codereview.chromium.org/2045253005).

[trchen, 2016/06/13 23:26:52]

At this point we are in the border box space of the SVG root. I believe it is
also safe to assume currentTransformNode ==
objectProperties->svgLocalToBorderBoxTransform()->parent() . Whether it's SPv1
or SPv2, the matrix to go from the border box to the SVG viewport are supposed
to be the same. If they don't match, it could be a signal that there is a
mismatch in our implementation.

I believe the problem lies in PaintPropertyTreeBuilder::updateSvgLocalTransform
only baked in the paint offset without doing LayoutUnit snapping. I think it
should match SVGRootPainter::paint, line 24 and 56~62. More layout tests will
pass with that. :)

[pdr., 2016/06/21 22:26:21]

Ahh, I misunderstood what you meant. Excellent points.

In the latest patch I've added a FIXME to include pixel snapping in the
svgLocalToBorderBoxTransform. Mind if I fix that in a followup? We discussed
this a bit and it will be a little involved in PaintPropertyTreeBuilder.

+                    auto& paintController = context.getPaintController();
+                    PaintChunkProperties properties(paintController.currentPaintChunkProperties());
+                    properties.transform = objectProperties->svgLocalToBorderBoxTransform();
+                    m_transformPropertyScope.emplace(paintController, properties);
+                }
+            } else {
+                DCHECK(object.isSVG());
+                // Should only be used by LayoutSVGRoot.
+                DCHECK(!objectProperties->svgLocalToBorderBoxTransform());
+
+                if (objectProperties->transform()) {

[trchen, 2016/06/13 22:45:19]

Ditto.

[pdr., 2016/06/13 23:03:19]

svgLocalToBorderBoxTransform will only be set on the root svg object and this is
dcheck'd on line 71.

[trchen, 2016/06/13 23:26:52]

I mean something similar like
DCHECK_EQ(objectProperties->transform()->transform(), transform);
Is there a case that they don't match?

[pdr., 2016/06/21 22:26:21]

Done. This actually caught a bug in PaintPropertyTreeBuilder where we need to
use a different (localSVGTransform) for foreignObject instead of the regular
localToSVGParentTransform. FIXME added.

+                    auto& paintController = context.getPaintController();
+                    PaintChunkProperties properties(paintController.currentPaintChunkProperties());
+                    properties.transform = objectProperties->transform();
+                    m_transformPropertyScope.emplace(paintController, properties);
+                }
+            }
+        }
+    }
+private:
+    Optional<ScopedPaintChunkProperties> m_transformPropertyScope;
+};
+
 class SVGPaintContext {
     STACK_ALLOCATED();
 public: