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.
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.
There is a tiny misktake in SVGPathSegList::getPathSegAtLength(). I'm going to fix it.
Created attachment 55096 [details] Fixed this bug and added a test case. Fixed this bug and added a test case.
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
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.
Created attachment 55452 [details] Forgot to set "review ?". Modified according to Niko's comment.
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.
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.
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
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.
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+
Created attachment 55473 [details] 4th edition Changed the last line. I admit that " return --traversalState.m_segmentIndex; " really looks a little strange. :P
Comment on attachment 55473 [details] 4th edition Good patch, r=me! Should land soon...
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
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!
/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.
Created attachment 55515 [details] 5th patch, fixed that warning.
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,
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.
(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.
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.
Comment on attachment 55663 [details] 7 th patch Great! Hopefully it lands this time.
Comment on attachment 55663 [details] 7 th patch Clearing flags on attachment: 55663 Committed r59213: <http://trac.webkit.org/changeset/59213>
All reviewed patches have been landed. Closing bug.
Just wanted to say thanks for fixing this so fast! Great work! :)
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?