SVG: Promote d to a property

third_party/WebKit/Source/core/svg/SVGPathElement.cpp

Index: third_party/WebKit/Source/core/svg/SVGPathElement.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGPathElement.cpp b/third_party/WebKit/Source/core/svg/SVGPathElement.cpp
index 676146dab788cb9592c3eaf4fcaf2b641047589f..ee8c347ad6894b7d9a0f3c8b959dfdcc94997d54 100644
--- a/third_party/WebKit/Source/core/svg/SVGPathElement.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGPathElement.cpp
@@ -21,10 +21,12 @@
 #include "config.h"
 #include "core/svg/SVGPathElement.h"
 
+#include "core/dom/NodeComputedStyle.h"
 #include "core/layout/svg/LayoutSVGPath.h"
 #include "core/svg/SVGDocumentExtensions.h"
 #include "core/svg/SVGMPathElement.h"
 #include "core/svg/SVGPathQuery.h"
+#include "core/svg/SVGPathUtilities.h"
 #include "core/svg/SVGPointTearOff.h"
 
 namespace blink {
@@ -72,52 +74,93 @@ DEFINE_NODE_FACTORY(SVGPathElement)
 
 Path SVGPathElement::asPath() const
 {
-    // If this is a <use> instance, return the referenced path to maximize geometry sharing.
-    if (const SVGElement* element = correspondingElement())
-        return toSVGPathElement(element)->asPath();
+    Path path;
+    buildPathFromByteStream(pathByteStream(), path);
+    return path;
+}
 
-    return m_path->currentValue()->path();
+const SVGPathByteStream& SVGPathElement::pathByteStream() const
+{
+    if (const ComputedStyle* style = static_cast<const Element*>(this)->computedStyle()) {

[fs, 2015/11/26 12:51:40]

Per below, this code should probably move to asPath (and just using
layoutObject()->style() and similar would probably make sense - I don't think
computedStyle buys us anything here. We assume that layoutObject() is non-null
here.) Also reinstate the correspondingElement() bit - that bit might require
more care about things being null.

[Eric Willigers, 2015/12/10 23:52:52]

A complication with the old correspondingElement() approach is that we should
use the CSS properties of the use element. For example, the new test
svg/css/inherit-d.svg fails when I reinstate that code.

[fs, 2015/12/11 13:01:58]

I think it would still be beneficial (and equivalent to the existing code) to
share the value for the presentation attribute style. With a
Style<something?>Path and a cached Path somewhere in the chain (maybe even a
cached Style<something?>Path in CSSPathValue...). The cached (or rather: shared)
Path is what will help us down-stream in Skia since it'll enable re-use of
tesselations for GPU rasterization - in case you're wondering why I'm assuming
such an adamant stance on this issue =). Well, that, memory use and avoiding
re-calculation of the path really... (but they are somewhat connected too)

+        const SVGComputedStyle& svgStyle = style->svgStyle();
+        if (svgStyle.d())
+            return *svgStyle.d();
+    }
+
+    return m_path->currentValue()->byteStream();

[fs, 2015/11/26 12:51:40]

Should only be using SVGComputedStyle here. No access to attributes.

 }
 
 float SVGPathElement::getTotalLength()
 {
+    static_cast<Element*>(this)->ensureComputedStyle();

[fs, 2015/11/26 12:51:40]

I don't think these methods should need to care about the computed style, and
just operate on the path from the attribute. This is not specified (AFAIK), but
it's consistent with other presentation attributes , like for instance
'transform'.

[pdr., 2015/12/02 05:36:50]

I'm not quite sure what you mean about the computed style--do you just mean we
should be using asPath?

I made a test of this: http://output.jsbin.com/nuqutejebu showing how
getTotalLength()/d compares to getBBox()/font-size. (This test also shows we
have a bug but I think switching to asPath will fix that right up.)

     return SVGPathQuery(pathByteStream()).getTotalLength();
 }
 
 PassRefPtrWillBeRawPtr<SVGPointTearOff> SVGPathElement::getPointAtLength(float length)
 {
+    static_cast<Element*>(this)->ensureComputedStyle();
     FloatPoint point = SVGPathQuery(pathByteStream()).getPointAtLength(length);
     return SVGPointTearOff::create(SVGPoint::create(point), 0, PropertyIsNotAnimVal);
 }
 
 unsigned SVGPathElement::getPathSegAtLength(float length)
 {
+    static_cast<Element*>(this)->ensureComputedStyle();
     return SVGPathQuery(pathByteStream()).getPathSegIndexAtLength(length);
 }
 
+bool SVGPathElement::isPresentationAttribute(const QualifiedName& attrName) const
+{
+    if (attrName == SVGNames::dAttr)
+        return true;
+    return SVGGeometryElement::isPresentationAttribute(attrName);
+}
+
+bool SVGPathElement::isPresentationAttributeWithSVGDOM(const QualifiedName& attrName) const
+{
+    if (attrName == SVGNames::dAttr)
+        return true;
+    return SVGGeometryElement::isPresentationAttributeWithSVGDOM(attrName);
+}
+
 void SVGPathElement::svgAttributeChanged(const QualifiedName& attrName)
 {
-    if (attrName == SVGNames::dAttr || attrName == SVGNames::pathLengthAttr) {
+    if (attrName == SVGNames::dAttr) {
         SVGElement::InvalidationGuard invalidationGuard(this);
+        invalidateSVGPresentationAttributeStyle();
+        setNeedsStyleRecalc(LocalStyleChange,
+            StyleChangeReasonForTracing::fromAttribute(attrName));
 
-        LayoutSVGShape* layoutObject = toLayoutSVGShape(this->layoutObject());
+        if (LayoutSVGShape* layoutPath = toLayoutSVGShape(this->layoutObject()))
+            layoutPath->setNeedsShapeUpdate();
 
-        if (attrName == SVGNames::dAttr) {
-            if (layoutObject)
-                layoutObject->setNeedsShapeUpdate();
+        invalidateMPathDependencies();
 
-            invalidateMPathDependencies();
-        }
+        if (layoutObject())
+            markForLayoutAndParentResourceInvalidation(layoutObject());

[fs, 2015/11/26 12:51:40]

Missing InvalidationGuard - or drop this and don't return.

[Eric Willigers, 2015/12/10 23:52:52]

InvalidationGuard is at line 129.

[fs, 2015/12/11 13:01:58]

*goes to check eyesight*

 
-        if (layoutObject)
-            markForLayoutAndParentResourceInvalidation(layoutObject);
+        return;
+    }
 
+    if (attrName == SVGNames::dAttr || attrName == SVGNames::pathLengthAttr) {

[fs, 2015/11/26 12:51:40]

Checking for dAttr here is "dead", but depends on how the above is resolved.

[Eric Willigers, 2015/12/10 23:52:52]

Acknowledged.

+        SVGElement::InvalidationGuard invalidationGuard(this);
+        if (layoutObject())
+            markForLayoutAndParentResourceInvalidation(layoutObject());
         return;
     }
 
     SVGGeometryElement::svgAttributeChanged(attrName);
 }
 
+void SVGPathElement::collectStyleForPresentationAttribute(const QualifiedName& name, const AtomicString& value, MutableStylePropertySet* style)
+{
+    RefPtrWillBeRawPtr<SVGAnimatedPropertyBase> property = propertyFromAttribute(name);
+    if (property == m_path)
+        addPropertyToPresentationAttributeStyle(style, CSSPropertyD, m_path->currentValue()->valueAsString());

[fs, 2015/11/26 12:51:40]

This will make for a fairly inefficient round-trip:

m_path (SVGPathByteStream) -> String -> SVGPathByteStream -> ...

but more on that elsewhere...

[Eric Willigers, 2015/12/10 23:52:52]

Changed to use CSSPathValue

+    else
+        SVGGeometryElement::collectStyleForPresentationAttribute(name, value, style);
+}
+
 void SVGPathElement::invalidateMPathDependencies()
 {
     // <mpath> can only reference <path> but this dependency is not handled in