Closed Bug 1024860 Opened 7 years ago Closed 7 years ago

Empty SVG Path getTotalLength() throws exception in JS

Categories

(Core :: SVG, defect)

31 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: alancutter, Assigned: longsonr)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.49 Safari/537.36

Steps to reproduce:

Call getTotalLength() on an SVG Path element with an empty data field.
http://jsfiddle.net/PGPqM/12/



Actual results:

[Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://fiddle.jshell.net/PGPqM/12/show/ :: window.onload :: line 23"  data: no]



Expected results:

Total path length: 0
This test passes.
Component: Untriaged → SVG
Product: Firefox → Core
Attached patch length.txtSplinter Review
Assignee: nobody → longsonr
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8441186 - Flags: review?(jwatt)
I'll adapt the mochitest from bug 1024926 to cover this case before landing i.e. start from a d="" measure and check that and then set the path in the bug 1024926 mochitest using setAttribute.
Comment on attachment 8441186 [details] [diff] [review]
length.txt

Review of attachment 8441186 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/SVGPathElement.cpp
@@ +347,5 @@
>      float authorsPathLengthEstimate = mPathLength.GetAnimValue();
>      if (authorsPathLengthEstimate > 0) {
>        RefPtr<Path> path = GetPathForLengthOrPositionMeasuring();
> +      if (!path) {
> +        return 0.0;       

Return 1.0 as we do at the end of this function. And remove the trailing whitespace from this line.
Attachment #8441186 - Flags: review?(jwatt) → review+
> Return 1.0 as we do at the end of this function. And remove the trailing
> whitespace from this line.

Returning 1.0 would be wrong at that point. RefPtr<Path> path = GetPathForLengthOrPositionMeasuring(); returns null if the path is empty i.e. its a zero length path. 

We want to return  path->ComputeLength() / authorsPathLengthEstimate; and we've tested that authorsPathLengthEstimate > 0 so in this inner bit of code that would come out as 0 (it used to do that before moz2d came along and made empty paths return nullptr)

You want some comments to help future readers?
Flags: needinfo?(jwatt)
Okay. And yes, a comment would be helpful. Thanks!
Flags: needinfo?(jwatt)
Comment on attachment 8441186 [details] [diff] [review]
length.txt

Apparently the webidl change needs a DOM peer review
Attachment #8441186 - Flags: review?(bzbarsky)
Comment on attachment 8441186 [details] [diff] [review]
length.txt

r=me
Attachment #8441186 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/82210155467c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.