Bug 37515

Summary:

getPathSegAtLength() gives incorrect value

Product:

WebKit

Reporter:

Alexis Deveria <adeveria>

Component:

SVG

Assignee:

Robin Qiu <robin.qiu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

charles.wei, commit-queue, eric, jeffschiller, phrogz, robin.qiu, staikos, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

URL:

http://svgtorture.googlecode.com/svn/trunk/automated/test.html?SVGPathElement%20module%3A%20Supports%20getPathSegAtLength%28%29

Attachments:

Description	Flags
Testcase that alerts the index returned by getPathSegAtLength()	none
Fixed this bug and added a test case.	none
Modified according to Niko's comment.	none
Forgot to set "review ?". Modified according to Niko's comment.	zimmermann: review-
3rd patch	zimmermann: review-
4th edition	zimmermann: review+, commit-queue: commit-queue-
5th patch, fixed that warning.	zimmermann: review-
7 th patch	none

Alexis Deveria

Reported 2010-04-13 12:10:45 PDT

Created attachment 53270 [details] Testcase that alerts the index returned by getPathSegAtLength() On a simple path using this data: "M0,0 L0,5 L0,10", using getPathSegAtLength(7) should return the number 2. Instead, it returns the number 1. Currently Opera gets this wrong too, while Gecko gets it right.

Attachments
Testcase that alerts the index returned by getPathSegAtLength() (484 bytes, image/svg+xml) 2010-04-13 12:10 PDT, Alexis Deveria	no flags	Details
Fixed this bug and added a test case. (4.24 KB, patch) 2010-05-05 01:11 PDT, Robin Qiu	no flags	Details Formatted Diff Diff
Modified according to Niko's comment. (5.98 KB, patch) 2010-05-07 19:51 PDT, Robin Qiu	no flags	Details Formatted Diff Diff
Forgot to set "review ?". Modified according to Niko's comment. (5.98 KB, patch) 2010-05-07 19:54 PDT, Robin Qiu	zimmermann: review-	Details Formatted Diff Diff
3rd patch (6.46 KB, patch) 2010-05-08 00:44 PDT, Robin Qiu	zimmermann: review-	Details Formatted Diff Diff
4th edition (6.46 KB, patch) 2010-05-08 05:51 PDT, Robin Qiu	zimmermann: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
5th patch, fixed that warning. (6.57 KB, patch) 2010-05-09 18:47 PDT, Robin Qiu	zimmermann: review-	Details Formatted Diff Diff
7 th patch (6.78 KB, patch) 2010-05-10 22:05 PDT, Robin Qiu	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Robin Qiu

Comment 1 2010-05-04 21:16:32 PDT

According to the spec: "The index of the path segment, where the first path segment is number 0." I think the return value should be 1. However, there is indeed a bug in this function, because getPathSegAtLength(2) and getPathSegAtLength(7) both return 1. By the way, assuming the index of the path segment starts from 1, Firefox is correct.

Robin Qiu

Comment 2 2010-05-05 00:12:38 PDT

There is a tiny misktake in SVGPathSegList::getPathSegAtLength(). I'm going to fix it.

Robin Qiu

Comment 3 2010-05-05 01:11:24 PDT

Created attachment 55096 [details] Fixed this bug and added a test case. Fixed this bug and added a test case.

Nikolas Zimmermann

Comment 4 2010-05-07 06:47:36 PDT

Comment on attachment 55096 [details] Fixed this bug and added a test case. r-, some comments: > diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog > index 6715cab..8bb4169 100644 > --- a/LayoutTests/ChangeLog > +++ b/LayoutTests/ChangeLog > @@ -1,3 +1,13 @@ > +2010-05-05 Robin Qiu <robin.qiu@torchmobile.com.cn> > + > + Reviewed by NOBODY (OOPS!). > + > + Fix a bug in SVGPathSegList::getPathSegAtLength(). > + https://bugs.webkit.org/show_bug.cgi?id=37515 The ChangeLog needs to be much more explicit :-) Please describe in detail what was broken, and what is fixed now. > +++ b/LayoutTests/svg/dom/svgpath-getPathSegAtLength.html > @@ -0,0 +1,28 @@ > +<!DOCTYPE html> > +<html> > +<head> > +<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css"> > +<script src="../../fast/js/resources/js-test-pre.js"></script> > +</head> > +<body> > +<p id="description"></p> > +<div id="console"></div> > +<script> > + description("Test SVG path.getPathSegAtLength()."); > + path = document.createElementNS("http://www.w3.org/2000/svg", "path"); > + path.setAttributeNS(null, "d", "M0 0 L0 5 L5 5 L 5 0"); > + var segIndex = [ path.getPathSegAtLength(0), > + path.getPathSegAtLength(1), > + path.getPathSegAtLength(5), > + path.getPathSegAtLength(6), > + path.getPathSegAtLength(10), > + path.getPathSegAtLength(11), > + path.getPathSegAtLength(16)]; > + shouldBe("segIndex.toString()", "'0,0,1,1,2,2,0'"); > + successfullyParsed = true; > +</script> > +<script src="../../fast/js/resources/js-test-post.js"></script> > +</body> > +</html> In general this looks fine, though the prefered style is to add a new svgpath-getPathSegAtLength.js file in svg/dom/script-tests. See svg/dom/id-reflect.html & svg/dom/script-tests/id-reflect.js as example template. Should be trivial to change. A comment within the test would also be nice, to why the segIndex should look like this. I'd advice to use "shouldBe" though, as it's much easier to read in the expected output. IIRC: shouldBe("path.getPathSegAtLength(0)", "0"); etc.. This will include the actual command that you're executing in the expected.txt file. > +2010-05-05 Robin Qiu <robin.qiu@torchmobile.com.cn> > + > + Reviewed by NOBODY (OOPS!). > + > + Fix a bug in SVGPathSegList::getPathSegAtLength(). > + https://bugs.webkit.org/show_bug.cgi?id=37515 > + > + Test: svg/dom/svgpath-getPathSegAtLength.html Same as above, needs better explaination. > -unsigned SVGPathSegList::getPathSegAtLength(double, ExceptionCode& ec) > +unsigned SVGPathSegList::getPathSegAtLength(double length, ExceptionCode& ec) > { > // FIXME : to be useful this will need to support non-normalized SVGPathSegLists > int len = numberOfItems(); > // FIXME: Eventually this will likely move to a "path applier"-like model, until then PathTraversalState is less useful as we could just use locals > PathTraversalState traversalState(PathTraversalState::TraversalSegmentAtLength); > + traversalState.m_desiredLength = length; > for (int i = 0; i < len; ++i) { > SVGPathSeg* segment = getItem(i, ec).get(); > if (ec) > @@ -90,6 +91,8 @@ unsigned SVGPathSegList::getPathSegAtLength(double, ExceptionCode& ec) > ASSERT(false); // FIXME: This only works with normalized/processed path data. > break; > } > + if (!segmentLength) > + continue; > traversalState.m_totalLength += segmentLength; > if ((traversalState.m_action == PathTraversalState::TraversalSegmentAtLength) > && (traversalState.m_totalLength > traversalState.m_desiredLength)) { Wow, I wouldn't have expected it's so easy to implement. I wonder that this doesn't have any side effects :-) Can you elaborate a bit more here to why this is correct? Maybe my question is resolved with a better ChangeLog! Thanks a lot for tackling this, Niko

Robin Qiu

Comment 5 2010-05-07 19:51:23 PDT

Created attachment 55450 [details] Modified according to Niko's comment. This bug is just a misktake: almost all of the code is OK, but the parameter is not used at all, therefore, this function always returns "1". Another tiny problem is that the SVG spec says the index of the first path segment is number 0, but we start from number 1. (By the way, Firefox starts from 1 as well), So I added this code to let it start from 0. + if (!segmentLength) + continue Here is a question, does "M0 0 L5 5 L5 5 L10 10" equal to "M0 0 L5 5 L10 10" ? If not, those two line code I added is incorrect, I will put it right.

Robin Qiu

Comment 6 2010-05-07 19:54:22 PDT

Created attachment 55452 [details] Forgot to set "review ?". Modified according to Niko's comment.

Nikolas Zimmermann

Comment 7 2010-05-07 22:15:40 PDT

Comment on attachment 55452 [details] Forgot to set "review ?". Modified according to Niko's comment. WebCore/svg/SVGPathSegList.cpp:96 + if (!segmentLength) Almost r+, the test is perfect. Just this line bugs me, from a SVG DOM perspective, "M 5 5" and "M 5 5 M 5 5" are two different path segments, no matter how it's rendered. Also how can you be sure this is just a "move"? And why is the first move the one ignored? I guess this is not correct.

Nikolas Zimmermann

Comment 8 2010-05-07 23:32:02 PDT

Okay, I've tried the testcase across the SVG implementations: Safari 4 (latest release) FAIL path.getPathSegAtLength(0) should be 0. Was 1. FAIL path.getPathSegAtLength(1) should be 0. Was 1. PASS path.getPathSegAtLength(5) is 1 PASS path.getPathSegAtLength(6) is 1 FAIL path.getPathSegAtLength(10) should be 2. Was 1. FAIL path.getPathSegAtLength(11) should be 2. Was 1. FAIL path.getPathSegAtLength(16) should be 0. Was 1. Opera 10.10 FAIL path.getPathSegAtLength(0) should be 0. Was 2. FAIL path.getPathSegAtLength(1) should be 0. Was 2. FAIL path.getPathSegAtLength(5) should be 1. Was 2. FAIL path.getPathSegAtLength(6) should be 1. Was 2. PASS path.getPathSegAtLength(10) is 2 PASS path.getPathSegAtLength(11) is 2 FAIL path.getPathSegAtLength(16) should be 0. Was 3. FireFox trunk (Minefield) PASS path.getPathSegAtLength(0) is 0 FAIL path.getPathSegAtLength(1) should be 0. Was 1. PASS path.getPathSegAtLength(5) is 1 FAIL path.getPathSegAtLength(6) should be 1. Was 2. PASS path.getPathSegAtLength(10) is 2 FAIL path.getPathSegAtLength(11) should be 2. Was 3. FAIL path.getPathSegAtLength(16) should be 0. Was 3. I think FireFox does it right. The path is auto-closed because of the last segment, so the distance=16 should actually say 3 IMHO.

Nikolas Zimmermann

Comment 9 2010-05-07 23:59:10 PDT

Some more results on distances that exceed the actual path length, Opera/FF always return the last segment, we should mimic that. PASS path.getPathSegAtLength(20) is 3 PASS path.getPathSegAtLength(24) is 3 PASS path.getPathSegAtLength(25) is 3 PASS path.getPathSegAtLength(100) is 3

Robin Qiu

Comment 10 2010-05-08 00:44:27 PDT

Created attachment 55468 [details] 3rd patch Thanks for Niko's comment! According to his comment, I changed some code, now, WebKit/Opera/FF all return the last path segment if the distance exceeds the actual path length, and also added some code in test case to verify this.

Nikolas Zimmermann

Comment 11 2010-05-08 00:53:20 PDT

Comment on attachment 55468 [details] 3rd patch WebCore/svg/SVGPathSegList.cpp:103 + return --traversalState.m_segmentIndex; Hm, why is that? Just return "traversalState.m_segmentIndex - 1". That saves one assignment operation :-) Sorry for that, but r- :( Other than that it looks very good, If you change that last thing, I'm going to set r+/cq+

Robin Qiu

Comment 12 2010-05-08 05:51:19 PDT

Created attachment 55473 [details] 4th edition Changed the last line. I admit that " return --traversalState.m_segmentIndex; " really looks a little strange. :P

Nikolas Zimmermann

Comment 13 2010-05-09 01:32:31 PDT

Comment on attachment 55473 [details] 4th edition Good patch, r=me! Should land soon...

WebKit Commit Bot

Comment 14 2010-05-09 01:57:45 PDT

Comment on attachment 55473 [details] 4th edition Rejecting patch 55473 from commit-queue. Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1 Last 500 characters of output: setenv YACC /Developer/usr/bin/yacc /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh ** BUILD FAILED ** The following build commands failed: WebCore: Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/SVGPathSegList.o /Users/eseidel/Projects/CommitQueue/WebCore/svg/SVGPathSegList.cpp normal i386 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://webkit-commit-queue.appspot.com/results/2199083

Eric Seidel (no email)

Comment 15 2010-05-09 14:05:59 PDT

Comment on attachment 55473 [details] 4th edition I kinda dislike the behavior of returning the last path segment when you go past the path end, but I guess that makes the API more user friendly. Glad we're finally adding testing! That's why the method didn't work before was there was no testing. :) Beautiful patch, btw. Very well done!

Eric Seidel (no email)

Comment 16 2010-05-09 14:06:56 PDT

/Users/eseidel/Projects/CommitQueue/WebCore/svg/SVGPathSegList.cpp: In member function ‘unsigned int WebCore::SVGPathSegList::getPathSegAtLength(double, WebCore::ExceptionCode&)’: /Users/eseidel/Projects/CommitQueue/WebCore/svg/SVGPathSegList.cpp:60: warning: implicit conversion shortens 64-bit value into a 32-bit value was the error on the commit queue, btw.

Robin Qiu

Comment 17 2010-05-09 18:47:32 PDT

Created attachment 55515 [details] 5th patch, fixed that warning.

Eric Seidel (no email)

Comment 18 2010-05-09 18:55:19 PDT

Comment on attachment 55515 [details] 5th patch, fixed that warning. Should we just have changed m_desiredLength to be a double? I'm not sure what type Path exposes for its length,

Nikolas Zimmermann

Comment 19 2010-05-10 01:28:43 PDT

Comment on attachment 55515 [details] 5th patch, fixed that warning. Hey Eric, I just saw that you have already set r+, I'm taking it back to r-, because of two issues: WebCore/svg/SVGPathSegList.cpp:60 + traversalState.m_desiredLength = static_cast<float>length; How can this compile? Missing paranthesis, it should be: static_cast<float>(length). Second issue: We should not just cast double to float, but instead use "narrowPrecisionToFloat". Please upload a revised version.

Robin Qiu

Comment 20 2010-05-10 03:04:51 PDT

(In reply to comment #19) > (From update of attachment 55515 [details]) I'm really sorry for this fault. I should always check the code in any case. I'm re-compiling all code and will do all layout tests. After that, I will upload a new one.

Robin Qiu

Comment 21 2010-05-10 22:05:08 PDT

Created attachment 55663 [details] 7 th patch Changes in this patch: 1. use narrowPrecisionToFloat() to converst double to float. 2. add a check before return "traversalState.m_segmentIndex - 1", otherwise, it maybe return 4294967295(-1) in some occasions.

Dirk Schulze

Comment 22 2010-05-10 22:56:59 PDT

Comment on attachment 55663 [details] 7 th patch Great! Hopefully it lands this time.

WebKit Commit Bot

Comment 23 2010-05-12 01:19:31 PDT

Comment on attachment 55663 [details] 7 th patch Clearing flags on attachment: 55663 Committed r59213: <http://trac.webkit.org/changeset/59213>

WebKit Commit Bot

Comment 24 2010-05-12 01:19:40 PDT

All reviewed patches have been landed. Closing bug.

Alexis Deveria

Comment 25 2010-05-18 08:06:43 PDT

Just wanted to say thanks for fixing this so fast! Great work! :)

Gavin Kistner

Comment 26 2011-03-09 13:21:51 PST

This bug appears to be correctly fixed in WebKit nightly builds, but is still present in the just-released Safari v5.0.4. How can I a) find out why it has not been accepted there, and/or b) encourage it to be applied to Safari?

Note You need to log in before you can comment on or make changes to this bug.