Unify content transformation calculations for SVG clip paths

Source/core/layout/svg/LayoutSVGResourceClipper.cpp

Index: Source/core/layout/svg/LayoutSVGResourceClipper.cpp
diff --git a/Source/core/layout/svg/LayoutSVGResourceClipper.cpp b/Source/core/layout/svg/LayoutSVGResourceClipper.cpp
index 71d7acbedb40d7d782eb02f157048e53e621af5b..f278c86b814e40b07b252b956f719281d3457896 100644
--- a/Source/core/layout/svg/LayoutSVGResourceClipper.cpp
+++ b/Source/core/layout/svg/LayoutSVGResourceClipper.cpp
@@ -66,8 +66,26 @@ void LayoutSVGResourceClipper::removeClientFromCache(LayoutObject* client, bool
     markClientForInvalidation(client, markForInvalidation ? BoundariesInvalidation : ParentOnlyInvalidation);
 }
 
+AffineTransform LayoutSVGResourceClipper::calculateContentTransformation(const LayoutObject* target, const FloatRect& targetBoundingBox)

[fs, 2015/06/16 09:30:15]

const?

+{
+    AffineTransform transform = toSVGClipPathElement(element())->calculateAnimatedLocalTransform();
+
+    // When drawing a clip for non-SVG elements, the CTM does not include the zoom factor.
+    // In this case, we need to apply the zoom scale explicitly - but only for clips with
+    // userSpaceOnUse units (the zoom is accounted for objectBoundingBox-resolved lengths).
+    if (target && !target->isSVG() && clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_USERSPACEONUSE)
+        transform.scale(style()->effectiveZoom());

[fs, 2015/06/16 09:30:15]

Shouldn't this apply before 'transform'?

+
+    if (clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
+        transform.translate(targetBoundingBox.x(), targetBoundingBox.y());
+        transform.scaleNonUniform(targetBoundingBox.width(), targetBoundingBox.height());
+    }
+
+    return transform;
+}
+
 bool LayoutSVGResourceClipper::tryPathOnlyClipping(const LayoutObject& layoutObject, GraphicsContext* context,
-    const AffineTransform& animatedLocalTransform, const FloatRect& objectBoundingBox) {
+    const AffineTransform& contentTransformation) {
     // If the current clip-path gets clipped itself, we have to fallback to masking.
     if (!style()->svgStyle().clipperResource().isEmpty())
         return false;
@@ -111,15 +129,7 @@ bool LayoutSVGResourceClipper::tryPathOnlyClipping(const LayoutObject& layoutObj
 
     // We are able to represent the clip as a path. Continue with direct clipping,
     // and transform the content to userspace if necessary.
-    if (clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
-        AffineTransform transform;
-        transform.translate(objectBoundingBox.x(), objectBoundingBox.y());
-        transform.scaleNonUniform(objectBoundingBox.width(), objectBoundingBox.height());
-        clipPath.transform(transform);
-    }
-
-    // Transform path by animatedLocalTransform.
-    clipPath.transform(animatedLocalTransform);
+    clipPath.transform(contentTransformation);
 
     if (RuntimeEnabledFeatures::slimmingPaintEnabled()) {
         if (!context->displayItemList()->displayItemConstructionIsDisabled())
@@ -132,16 +142,10 @@ bool LayoutSVGResourceClipper::tryPathOnlyClipping(const LayoutObject& layoutObj
     return true;
 }
 
-PassRefPtr<const SkPicture> LayoutSVGResourceClipper::createContentPicture(AffineTransform& contentTransformation, const FloatRect& targetBoundingBox,
-    GraphicsContext* context)
+PassRefPtr<const SkPicture> LayoutSVGResourceClipper::createContentPicture(const AffineTransform& contentTransformation, GraphicsContext* context)
 {
     ASSERT(frame());
 
-    if (clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
-        contentTransformation.translate(targetBoundingBox.x(), targetBoundingBox.y());
-        contentTransformation.scaleNonUniform(targetBoundingBox.width(), targetBoundingBox.height());
-    }
-
     if (m_clipContentPicture)
         return m_clipContentPicture;
 
@@ -190,7 +194,7 @@ PassRefPtr<const SkPicture> LayoutSVGResourceClipper::createContentPicture(Affin
     return m_clipContentPicture;
 }
 
-void LayoutSVGResourceClipper::calculateClipContentPaintInvalidationRect()
+void LayoutSVGResourceClipper::calculateClipContentPaintInvalidationRect(const LayoutObject* object)
 {
     // This is a rough heuristic to appraise the clip size and doesn't consider clip on clip.
     for (SVGElement* childElement = Traversal<SVGElement>::firstChild(*element()); childElement; childElement = Traversal<SVGElement>::nextSibling(*childElement)) {
@@ -204,7 +208,9 @@ void LayoutSVGResourceClipper::calculateClipContentPaintInvalidationRect()
             continue;
         m_clipBoundaries.unite(layoutObject->localToParentTransform().mapRect(layoutObject->paintInvalidationRectInLocalCoordinates()));
     }
-    m_clipBoundaries = toSVGClipPathElement(element())->calculateAnimatedLocalTransform().mapRect(m_clipBoundaries);
+
+    const AffineTransform& contentTransformation = calculateContentTransformation(object, object->objectBoundingBox());
+    m_clipBoundaries = contentTransformation.mapRect(m_clipBoundaries);

[fs, 2015/06/16 09:30:15]

This makes m_clipBoundaries depend on a certain client - the previous
formulation was only dependent on the clip path itself. Maybe just sink this
part out of the function, and let m_clipBoundaries be in the local coordinate
space of the clipPath instead?

 }
 
 bool LayoutSVGResourceClipper::hitTestClipContent(const FloatRect& objectBoundingBox, const FloatPoint& nodeAtPoint)
@@ -213,18 +219,8 @@ bool LayoutSVGResourceClipper::hitTestClipContent(const FloatRect& objectBoundin
     if (!SVGLayoutSupport::pointInClippingArea(this, point))
         return false;
 
-    if (clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
-        AffineTransform transform;
-        transform.translate(objectBoundingBox.x(), objectBoundingBox.y());
-        transform.scaleNonUniform(objectBoundingBox.width(), objectBoundingBox.height());
-        point = transform.inverse().mapPoint(point);
-    }
-
-    AffineTransform animatedLocalTransform = toSVGClipPathElement(element())->calculateAnimatedLocalTransform();
-    if (!animatedLocalTransform.isInvertible())

[fs, 2015/06/16 09:30:15]

This got dropped.

-        return false;
-
-    point = animatedLocalTransform.inverse().mapPoint(point);
+    AffineTransform contentTransformation = calculateContentTransformation(nullptr, objectBoundingBox);
+    contentTransformation.inverse().mapPoint(point);

[fs, 2015/06/16 09:30:15]

point = ...

 
     for (SVGElement* childElement = Traversal<SVGElement>::firstChild(*element()); childElement; childElement = Traversal<SVGElement>::nextSibling(*childElement)) {
         LayoutObject* layoutObject = childElement->layoutObject();
@@ -248,15 +244,7 @@ FloatRect LayoutSVGResourceClipper::resourceBoundingBox(const LayoutObject* obje
         return object->objectBoundingBox();
 
     if (m_clipBoundaries.isEmpty())
-        calculateClipContentPaintInvalidationRect();
-
-    if (clipPathUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX) {
-        FloatRect objectBoundingBox = object->objectBoundingBox();
-        AffineTransform transform;
-        transform.translate(objectBoundingBox.x(), objectBoundingBox.y());
-        transform.scaleNonUniform(objectBoundingBox.width(), objectBoundingBox.height());
-        return transform.mapRect(m_clipBoundaries);
-    }
+        calculateClipContentPaintInvalidationRect(object);
 
     return m_clipBoundaries;
 }