[webkit-reviews] review denied: [Bug 37515] getPathSegAtLength() gives incorrect value : [Attachment 55096] Fixed this bug and added a test case.

Fri May 7 06:47:35 PDT 2010

Nikolas Zimmermann <zimmermann at kde.org> has denied Robin Qiu
<robin.qiu at torchmobile.com.cn>'s request for review:
Bug 37515: getPathSegAtLength() gives incorrect value
https://bugs.webkit.org/show_bug.cgi?id=37515

Attachment 55096: Fixed this bug and added a test case.
https://bugs.webkit.org/attachment.cgi?id=55096&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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 at 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 at 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