344378 - Implement nsSVGPathElement::GetPointAtLength

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alex Menzies]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

Upcoming patch implements GetPointAtLength

Reproducible: Always

Comment 1

[Alex Menzies]

Attachment: Patch

Comment 2

[Robert Longson [:longsonr]]

> +  nsresult rv = NS_NewSVGPoint(_retval, x, y);
> +  if (NS_FAILED(rv))
> +    return rv;
> +
> +  return NS_OK;

better as 

return NS_NewSVGPoint(_retval, x, y);

Comment 3

[tor]

Comment on attachment 228968 [details] [diff] [review]
Patch

>? .cdtproject
>? .project
>? content/svg/content/src/.nsSVGFilters.cpp.marks
>? content/svg/content/src/.nsSVGPathElement.cpp.marks
>? layout/svg/base/src/.nsSVGTextFrame.cpp.marks
>? security/nss/cmd/shlibsign/NONE
>? security/nss/cmd/shlibsign/mangle/NONE
>? security/nss/lib/ckfw/builtins/NONE
>? security/nss/lib/freebl/NONE
>? security/nss/lib/nss/NONE
>? security/nss/lib/smime/NONE
>? security/nss/lib/softoken/NONE
>? security/nss/lib/ssl/NONE

You know you can diff just a particular directory, and not the whole tree?  Ex: "cvs diff content/svg".

> /* nsIDOMSVGPoint getPointAtLength (in float distance); */
> NS_IMETHODIMP
> nsSVGPathElement::GetPointAtLength(float distance, nsIDOMSVGPoint **_retval)
> {
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  nsAutoPtr<nsSVGFlattenedPath> flat(GetFlattenedPath());
>+  if (!flat)
>+    return NS_ERROR_FAILURE;
>+
>+  if (HasAttr(kNameSpaceID_None, nsGkAtoms::pathLength))
>+  {
>+    float pLen, tLen;

Rename these variable to be clearer what you're storing in them.

>+    mPathLength->GetAnimVal(&pLen);
>+    tLen = flat->GetLength();
>+    distance *= tLen / pLen;
>+  }
>+
>+  float x, y, ang;

"ang" -> "angle" likewise.

>+  flat->FindPoint(0, distance, 0, &x, &y, &ang);
>+  nsresult rv = NS_NewSVGPoint(_retval, x, y);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  return NS_OK;

You can just do "return NS_NewSVGPoint(...)".

Out of range distance values (less than zero, greater than the length of the path) need to be handled.  While at first glance it seems that returning NS_ERROR_DOM_SVG_INVALID_VALUE_ERR, the specification states that this method doesn't throw exceptions.  If we follow that, pinning to the endpoints might be the appropriate action.  Whichever approach you take, mail www-svg to make sure they'll address this in an errata or 1.2.

Comment 4

[Alex Menzies]

Attachment: Updated Patch

Updated patch implements comments above and modifies FindPoint to handle the case when aXOffset is equal to totalLength.

Comment 5

[tor]

Comment on attachment 229017 [details] [diff] [review]
Updated Patch

>+  if (HasAttr(kNameSpaceID_None, nsGkAtoms::pathLength))
>+  {

Nit - open brace on the same line as the "if".  With that, r=tor.

Comment 6

[Alex Menzies]

Attachment: Updated Patch, Nit Fix

Comment 7

[Alex Menzies]

Attachment: Test Case

In this test case two paths are created.  Both are of length 200, but the bottom line has its pathLength attribute set to 100.  A circle appears on each line at a point determined by a call to  getPointAtLength(distance) where distance is a variable that changes with time.  Distance is Initially set to -10 and is incremented by 10 every second.  This tests the cases when distance is negative, zero, greater than zero but less than the length of the path, equal to the length of the path, and greater than the length of the path.

Comment 8

[tor]

Attachment: branch version, won't work at onload time

Comment 9

[tor]

Checked in on trunk for Alex.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 229168 [details] [diff] [review]
branch version, won't work at onload time

+  *_retval = 0;

nsnull

Comment 11

[tor]

Comment on attachment 229168 [details] [diff] [review]
branch version, won't work at onload time

Low risk, useful DOM feature for content developers.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229168 [details] [diff] [review]
branch version, won't work at onload time

a=drivers, early landing approved since we're not expecting bake time to get us much testing :)

Comment 13

[tor]

Attachment: branch patch for checkin

Comment 14

[tor]

Checked in on branch.