Changeset 251877 in webkit

Timestamp:

Oct 31, 2019 2:08:59 PM (4 years ago)

Author:

commit-queue@webkit.org

Message:

SVGGeometryElement.getPointAtLength should clamp its argument to [0, length]
​https://bugs.webkit.org/show_bug.cgi?id=203687

Patch by Said Abou-Hallawa <​sabouhallawa@apple.com> on 2019-10-31
Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

• web-platform-tests/svg/types/scripted/SVGGeometryElement.getPointAtLength-01-expected.txt:

Source/WebCore:

The spec link is:

​https://svgwg.org/svg2-draft/types.html#__svg__SVGGeometryElement__getPointAtLength

-- Fix the SVGGeometryElement.idl to match the specs.
-- Fix the path utility functions to return zeros in case of an error.

The callers never used the return values.

• rendering/svg/RenderSVGShape.cpp:

(WebCore::RenderSVGShape::getPointAtLength const):

• rendering/svg/RenderSVGShape.h:
• svg/SVGGeometryElement.cpp:

(WebCore::SVGGeometryElement::getPointAtLength const):

• svg/SVGGeometryElement.h:
• svg/SVGGeometryElement.idl:
• svg/SVGPathElement.cpp:

(WebCore::SVGPathElement::getTotalLength const):
(WebCore::SVGPathElement::getPointAtLength const):
(WebCore::SVGPathElement::getPathSegAtLength const):

• svg/SVGPathElement.h:
• svg/SVGPathUtilities.cpp:

(WebCore::getSVGPathSegAtLengthFromSVGPathByteStream):
(WebCore::getTotalLengthOfSVGPathByteStream):
(WebCore::getPointAtLengthOfSVGPathByteStream):

• svg/SVGPathUtilities.h:

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/svg/types/scripted/SVGGeometryElement.getPointAtLength-01-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/svg/RenderSVGShape.cpp (modified) (1 diff)
• Source/WebCore/rendering/svg/RenderSVGShape.h (modified) (1 diff)
• Source/WebCore/svg/SVGGeometryElement.cpp (modified) (1 diff)
• Source/WebCore/svg/SVGGeometryElement.h (modified) (1 diff)
• Source/WebCore/svg/SVGGeometryElement.idl (modified) (1 diff)
• Source/WebCore/svg/SVGPathElement.cpp (modified) (1 diff)
• Source/WebCore/svg/SVGPathElement.h (modified) (1 diff)
• Source/WebCore/svg/SVGPathUtilities.cpp (modified) (4 diffs)
• Source/WebCore/svg/SVGPathUtilities.h (modified) (1 diff)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2019-10-31  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        SVGGeometryElement.getPointAtLength should clamp its argument to [0, length]
+        https://bugs.webkit.org/show_bug.cgi?id=203687
+
+        Reviewed by Simon Fraser.
+
+        * web-platform-tests/svg/types/scripted/SVGGeometryElement.getPointAtLength-01-expected.txt:
+
 2019-10-31  Antoine Quint  <graouts@apple.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/svg/types/scripted/SVGGeometryElement.getPointAtLength-01-expected.txt

@@ -1 +1 @@
 
-FAIL SVGGeometryElement.prototype.getPointAtLength clamps its argument to [0, length], less than zero (SVGLineElement). assert_equals: starting x expected 50 but got 40
-FAIL SVGGeometryElement.prototype.getPointAtLength clamps its argument to [0, length], less than zero (SVGPathElement). assert_equals: starting x expected 40 but got 30
+PASS SVGGeometryElement.prototype.getPointAtLength clamps its argument to [0, length], less than zero (SVGLineElement). 
+PASS SVGGeometryElement.prototype.getPointAtLength clamps its argument to [0, length], less than zero (SVGPathElement). 
 PASS SVGGeometryElement.prototype.getPointAtLength clamps its argument to [0, length], greater than 'length' (SVGLineElement). 
 PASS SVGGeometryElement.prototype.getPointAtLength clamps its argument to [0, length], greater than 'length' (SVGPathElement). 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-10-31  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        SVGGeometryElement.getPointAtLength should clamp its argument to [0, length]
+        https://bugs.webkit.org/show_bug.cgi?id=203687
+
+        Reviewed by Simon Fraser.
+
+        The spec link is:
+            https://svgwg.org/svg2-draft/types.html#__svg__SVGGeometryElement__getPointAtLength
+
+        -- Fix the SVGGeometryElement.idl to match the specs.
+        -- Fix the path utility functions to return zeros in case of an error.
+           The callers never used the return values.
+
+        * rendering/svg/RenderSVGShape.cpp:
+        (WebCore::RenderSVGShape::getPointAtLength const):
+        * rendering/svg/RenderSVGShape.h:
+        * svg/SVGGeometryElement.cpp:
+        (WebCore::SVGGeometryElement::getPointAtLength const):
+        * svg/SVGGeometryElement.h:
+        * svg/SVGGeometryElement.idl:
+        * svg/SVGPathElement.cpp:
+        (WebCore::SVGPathElement::getTotalLength const):
+        (WebCore::SVGPathElement::getPointAtLength const):
+        (WebCore::SVGPathElement::getPathSegAtLength const):
+        * svg/SVGPathElement.h:
+        * svg/SVGPathUtilities.cpp:
+        (WebCore::getSVGPathSegAtLengthFromSVGPathByteStream):
+        (WebCore::getTotalLengthOfSVGPathByteStream):
+        (WebCore::getPointAtLengthOfSVGPathByteStream):
+        * svg/SVGPathUtilities.h:
+
 2019-10-31  Zan Dobersek  <zdobersek@igalia.com>  and  Chris Lord  <clord@igalia.com>
 

trunk/Source/WebCore/rendering/svg/RenderSVGShape.cpp

@@ -354 +354 @@
 }
 
-void RenderSVGShape::getPointAtLength(FloatPoint& point, float distance) const
+FloatPoint RenderSVGShape::getPointAtLength(float distance) const
 {
     if (!m_path)
-        return;
+        return { };
 
     bool isValid;
-    point = m_path->pointAtLength(distance, isValid);
+    return m_path->pointAtLength(distance, isValid);
 }
 

trunk/Source/WebCore/rendering/svg/RenderSVGShape.h

@@ -67 +67 @@
 
     float getTotalLength() const;
-    void getPointAtLength(FloatPoint&, float distance) const;
+    FloatPoint getPointAtLength(float distance) const;
 
     bool hasPath() const { return m_path.get(); }

trunk/Source/WebCore/svg/SVGGeometryElement.cpp

@@ -56 +56 @@
 }
 
-Ref<SVGPoint> SVGGeometryElement::getPointAtLength(float distance) const
+ExceptionOr<Ref<SVGPoint>> SVGGeometryElement::getPointAtLength(float distance) const
 {
-    FloatPoint point { };
-
     document().updateLayoutIgnorePendingStylesheets();
 
     auto* renderer = downcast<RenderSVGShape>(this->renderer());
-    if (renderer)
-        renderer->getPointAtLength(point, distance);
+    
+    // Spec: If current element is a non-rendered element, throw an InvalidStateError.
+    if (!renderer)
+        return Exception { InvalidStateError };
+    
+    // Spec: Clamp distance to [0, length].
+    distance = clampTo<float>(distance, 0, getTotalLength());
 
-    return SVGPoint::create(point);
+    // Spec: Return a newly created, detached SVGPoint object.
+    return SVGPoint::create(renderer->getPointAtLength(distance));
 }
 

trunk/Source/WebCore/svg/SVGGeometryElement.h

@@ -36 +36 @@
 public:
     virtual float getTotalLength() const;
-    virtual Ref<SVGPoint> getPointAtLength(float distance) const;
+    virtual ExceptionOr<Ref<SVGPoint>> getPointAtLength(float distance) const;
 
     bool isPointInFill(DOMPointInit&&);

trunk/Source/WebCore/svg/SVGGeometryElement.idl

@@ -26 +26 @@
 // [Exposed=Window]
 interface SVGGeometryElement : SVGGraphicsElement {
-    readonly attribute SVGAnimatedNumber pathLength; // FIXME: Should be [SameObject].
+    [SameObject] readonly attribute SVGAnimatedNumber pathLength;
 
     boolean isPointInFill(optional DOMPointInit point);
     boolean isPointInStroke(optional DOMPointInit point);
     unrestricted float getTotalLength();
-    [NewObject] SVGPoint getPointAtLength(float distance);
+    [NewObject, MayThrowException] SVGPoint getPointAtLength(float distance);
 };

trunk/Source/WebCore/svg/SVGPathElement.cpp

@@ -107 +107 @@
 float SVGPathElement::getTotalLength() const
 {
-    float totalLength = 0;
-    getTotalLengthOfSVGPathByteStream(pathByteStream(), totalLength);
-    return totalLength;
+    return getTotalLengthOfSVGPathByteStream(pathByteStream());
 }
 
-Ref<SVGPoint> SVGPathElement::getPointAtLength(float length) const
+ExceptionOr<Ref<SVGPoint>> SVGPathElement::getPointAtLength(float distance) const
 {
-    FloatPoint point;
-    getPointAtLengthOfSVGPathByteStream(pathByteStream(), length, point);
-    return SVGPoint::create(point);
+    // Spec: Clamp distance to [0, length].
+    distance = clampTo<float>(distance, 0, getTotalLength());
+
+    // Spec: Return a newly created, detached SVGPoint object.
+    return SVGPoint::create(getPointAtLengthOfSVGPathByteStream(pathByteStream(), distance));
 }
 
 unsigned SVGPathElement::getPathSegAtLength(float length) const
 {
-    unsigned pathSeg = 0;
-    getSVGPathSegAtLengthFromSVGPathByteStream(pathByteStream(), length, pathSeg);
-    return pathSeg;
+    return getSVGPathSegAtLengthFromSVGPathByteStream(pathByteStream(), length);
 }
 

trunk/Source/WebCore/svg/SVGPathElement.h

@@ -89 +89 @@
 
     float getTotalLength() const final;
-    Ref<SVGPoint> getPointAtLength(float distance) const final;
+    ExceptionOr<Ref<SVGPoint>> getPointAtLength(float distance) const final;
     unsigned getPathSegAtLength(float distance) const;
 

trunk/Source/WebCore/svg/SVGPathUtilities.cpp

@@ -194 +194 @@
 }
 
-bool getSVGPathSegAtLengthFromSVGPathByteStream(const SVGPathByteStream& stream, float length, unsigned& pathSeg)
-{
-    if (stream.isEmpty())
-        return false;
+unsigned getSVGPathSegAtLengthFromSVGPathByteStream(const SVGPathByteStream& stream, float length)
+{
+    if (stream.isEmpty())
+        return 0;
 
     PathTraversalState traversalState(PathTraversalState::Action::SegmentAtLength);
@@ -203 +203 @@
 
     SVGPathByteStreamSource source(stream);
-    bool ok = SVGPathParser::parse(source, builder);
-    pathSeg = builder.pathSegmentIndex();
-    return ok;
-}
-
-bool getTotalLengthOfSVGPathByteStream(const SVGPathByteStream& stream, float& totalLength)
-{
-    if (stream.isEmpty())
-        return false;
+    SVGPathParser::parse(source, builder);
+    return builder.pathSegmentIndex();
+}
+
+float getTotalLengthOfSVGPathByteStream(const SVGPathByteStream& stream)
+{
+    if (stream.isEmpty())
+        return 0;
 
     PathTraversalState traversalState(PathTraversalState::Action::TotalLength);
@@ -218 +217 @@
 
     SVGPathByteStreamSource source(stream);
-    bool ok = SVGPathParser::parse(source, builder);
-    totalLength = builder.totalLength();
-    return ok;
-}
-
-bool getPointAtLengthOfSVGPathByteStream(const SVGPathByteStream& stream, float length, FloatPoint& point)
-{
-    if (stream.isEmpty())
-        return false;
+    SVGPathParser::parse(source, builder);
+    return builder.totalLength();
+}
+
+FloatPoint getPointAtLengthOfSVGPathByteStream(const SVGPathByteStream& stream, float length)
+{
+    if (stream.isEmpty())
+        return { };
 
     PathTraversalState traversalState(PathTraversalState::Action::VectorAtLength);
@@ -233 +231 @@
 
     SVGPathByteStreamSource source(stream);
-    bool ok = SVGPathParser::parse(source, builder);
-    point = builder.currentPoint();
-    return ok;
-}
-
-}
+    SVGPathParser::parse(source, builder);
+    return builder.currentPoint();
+}
+
+}

trunk/Source/WebCore/svg/SVGPathUtilities.h

@@ -54 +54 @@
 bool addToSVGPathByteStream(SVGPathByteStream& streamToAppendTo, const SVGPathByteStream& from, unsigned repeatCount = 1);
 
-bool getSVGPathSegAtLengthFromSVGPathByteStream(const SVGPathByteStream&, float length, unsigned& pathSeg);
-bool getTotalLengthOfSVGPathByteStream(const SVGPathByteStream&, float& totalLength);
-bool getPointAtLengthOfSVGPathByteStream(const SVGPathByteStream&, float length, FloatPoint&);
+unsigned getSVGPathSegAtLengthFromSVGPathByteStream(const SVGPathByteStream&, float length);
+float getTotalLengthOfSVGPathByteStream(const SVGPathByteStream&);
+FloatPoint getPointAtLengthOfSVGPathByteStream(const SVGPathByteStream&, float length);
 
 } // namespace WebCore