SVG: Promote d to a property

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

 /*
  * Copyright (C) 2004, 2005, 2006, 2008 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007 Rob Buis <buis@kde.org>
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  */
 
 #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 {
 
 class SVGAnimatedPathLength final : public SVGAnimatedNumber {
 public:
     static PassRefPtrWillBeRawPtr<SVGAnimatedPathLength> create(SVGPathElement* contextElement)
     {
         return adoptRefWillBeNoop(new SVGAnimatedPathLength(contextElement));
     }
@@ skipping 27 matching lines @@
 {
     visitor->trace(m_pathLength);
     visitor->trace(m_path);
     SVGGeometryElement::trace(visitor);
 }
 
 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) {
+        SVGElement::InvalidationGuard invalidationGuard(this);
+        invalidateSVGPresentationAttributeStyle();
+        setNeedsStyleRecalc(LocalStyleChange,
+            StyleChangeReasonForTracing::fromAttribute(attrName));
+
+        if (LayoutSVGShape* layoutPath = toLayoutSVGShape(this->layoutObject()))
+            layoutPath->setNeedsShapeUpdate();
+
+        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*

+
+        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);
-
-        LayoutSVGShape* layoutObject = toLayoutSVGShape(this->layoutObject());
-
-        if (attrName == SVGNames::dAttr) {
-            if (layoutObject)
-                layoutObject->setNeedsShapeUpdate();
-
-            invalidateMPathDependencies();
-        }
-
-        if (layoutObject)
-            markForLayoutAndParentResourceInvalidation(layoutObject);
-
+        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
     // markForLayoutAndParentResourceInvalidation so we update any mpath dependencies manually.
     if (SVGElementSet* dependencies = setOfIncomingReferences()) {
         for (SVGElement* element : *dependencies) {
             if (isSVGMPathElement(*element))
                 toSVGMPathElement(element)->targetPathChanged();
         }
     }
 }
 
 Node::InsertionNotificationRequest SVGPathElement::insertedInto(ContainerNode* rootParent)
 {
     SVGGeometryElement::insertedInto(rootParent);
     invalidateMPathDependencies();
     return InsertionDone;
 }
 
 void SVGPathElement::removedFrom(ContainerNode* rootParent)
 {
     SVGGeometryElement::removedFrom(rootParent);
     invalidateMPathDependencies();
 }
 
 } // namespace blink