SVGGeometryElement.getPointAtLength should clamp its argument to [0, length]
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Oct 2019 21:08:59 +0000 (21:08 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Oct 2019 21:08:59 +0000 (21:08 +0000)
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:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@251877 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/svg/types/scripted/SVGGeometryElement.getPointAtLength-01-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/rendering/svg/RenderSVGShape.cpp
Source/WebCore/rendering/svg/RenderSVGShape.h
Source/WebCore/svg/SVGGeometryElement.cpp
Source/WebCore/svg/SVGGeometryElement.h
Source/WebCore/svg/SVGGeometryElement.idl
Source/WebCore/svg/SVGPathElement.cpp
Source/WebCore/svg/SVGPathElement.h
Source/WebCore/svg/SVGPathUtilities.cpp
Source/WebCore/svg/SVGPathUtilities.h

index 297005d..b6b3d26 100644 (file)
@@ -1,3 +1,12 @@
+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>
 
         [Web Animations] Add support for AnimationEvent.pseudoElement
index 23f7357..d854351 100644 (file)
@@ -1,6 +1,6 @@
 
-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). 
 
index 6cbcd39..86ee704 100644 (file)
@@ -1,3 +1,35 @@
+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>
 
         Move ImageBuffer-related functionality from HTMLCanvasElement to CanvasBase
index 0a69c63..998fdcb 100644 (file)
@@ -353,13 +353,13 @@ float RenderSVGShape::getTotalLength() const
     return 0;
 }
 
-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);
 }
 
 bool RenderSVGShape::nodeAtFloatPoint(const HitTestRequest& request, HitTestResult& result, const FloatPoint& pointInParent, HitTestAction hitTestAction)
index 6463cfc..7f250bf 100644 (file)
@@ -66,7 +66,7 @@ public:
     bool isPointInStroke(const FloatPoint&);
 
     float getTotalLength() const;
-    void getPointAtLength(FloatPoint&, float distance) const;
+    FloatPoint getPointAtLength(float distance) const;
 
     bool hasPath() const { return m_path.get(); }
     Path& path() const
index b3d301c..d5ee0c8 100644 (file)
@@ -55,17 +55,21 @@ float SVGGeometryElement::getTotalLength() const
     return renderer->getTotalLength();
 }
 
-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));
 }
 
 bool SVGGeometryElement::isPointInFill(DOMPointInit&& pointInit)
index 555cb19..73a86d1 100644 (file)
@@ -35,7 +35,7 @@ class SVGGeometryElement : public SVGGraphicsElement {
     WTF_MAKE_ISO_ALLOCATED(SVGGeometryElement);
 public:
     virtual float getTotalLength() const;
-    virtual Ref<SVGPoint> getPointAtLength(float distance) const;
+    virtual ExceptionOr<Ref<SVGPoint>> getPointAtLength(float distance) const;
 
     bool isPointInFill(DOMPointInit&&);
     bool isPointInStroke(DOMPointInit&&);
index 56df6f1..8194786 100644 (file)
 
 // [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);
 };
index e1cff5c..ca53e5c 100644 (file)
@@ -106,23 +106,21 @@ void SVGPathElement::removedFromAncestor(RemovalType removalType, ContainerNode&
 
 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);
 }
 
 FloatRect SVGPathElement::getBBox(StyleUpdateStrategy styleUpdateStrategy)
index 917e33b..fa95e1d 100644 (file)
@@ -88,7 +88,7 @@ public:
     }
 
     float getTotalLength() const final;
-    Ref<SVGPoint> getPointAtLength(float distance) const final;
+    ExceptionOr<Ref<SVGPoint>> getPointAtLength(float distance) const final;
     unsigned getPathSegAtLength(float distance) const;
 
     FloatRect getBBox(StyleUpdateStrategy = AllowStyleUpdate) final;
index db8f929..b197d5b 100644 (file)
@@ -193,48 +193,45 @@ bool addToSVGPathByteStream(SVGPathByteStream& streamToAppendTo, const SVGPathBy
     return SVGPathBlender::addAnimatedPath(fromSource, bySource, builder, repeatCount);
 }
 
-bool getSVGPathSegAtLengthFromSVGPathByteStream(const SVGPathByteStream& stream, float length, unsigned& pathSeg)
+unsigned getSVGPathSegAtLengthFromSVGPathByteStream(const SVGPathByteStream& stream, float length)
 {
     if (stream.isEmpty())
-        return false;
+        return 0;
 
     PathTraversalState traversalState(PathTraversalState::Action::SegmentAtLength);
     SVGPathTraversalStateBuilder builder(traversalState, length);
 
     SVGPathByteStreamSource source(stream);
-    bool ok = SVGPathParser::parse(source, builder);
-    pathSeg = builder.pathSegmentIndex();
-    return ok;
+    SVGPathParser::parse(source, builder);
+    return builder.pathSegmentIndex();
 }
 
-bool getTotalLengthOfSVGPathByteStream(const SVGPathByteStream& stream, float& totalLength)
+float getTotalLengthOfSVGPathByteStream(const SVGPathByteStream& stream)
 {
     if (stream.isEmpty())
-        return false;
+        return 0;
 
     PathTraversalState traversalState(PathTraversalState::Action::TotalLength);
 
     SVGPathTraversalStateBuilder builder(traversalState);
 
     SVGPathByteStreamSource source(stream);
-    bool ok = SVGPathParser::parse(source, builder);
-    totalLength = builder.totalLength();
-    return ok;
+    SVGPathParser::parse(source, builder);
+    return builder.totalLength();
 }
 
-bool getPointAtLengthOfSVGPathByteStream(const SVGPathByteStream& stream, float length, FloatPoint& point)
+FloatPoint getPointAtLengthOfSVGPathByteStream(const SVGPathByteStream& stream, float length)
 {
     if (stream.isEmpty())
-        return false;
+        return { };
 
     PathTraversalState traversalState(PathTraversalState::Action::VectorAtLength);
 
     SVGPathTraversalStateBuilder builder(traversalState, length);
 
     SVGPathByteStreamSource source(stream);
-    bool ok = SVGPathParser::parse(source, builder);
-    point = builder.currentPoint();
-    return ok;
+    SVGPathParser::parse(source, builder);
+    return builder.currentPoint();
 }
 
 }
index 490ff7c..7a8fab6 100644 (file)
@@ -53,8 +53,8 @@ bool canBlendSVGPathByteStreams(const SVGPathByteStream& from, const SVGPathByte
 bool buildAnimatedSVGPathByteStream(const SVGPathByteStream& from, const SVGPathByteStream& to, SVGPathByteStream& result, float progress);
 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