data parser should parse up to a failure and not scrap the whole path

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Updated

9 years ago
Assignee: nobody → longsonr
(Assignee)

Comment 1

9 years ago
Created attachment 433811 [details] [diff] [review]
patch
Attachment #433811 - Flags: review?(jwatt)
(Assignee)

Updated

9 years ago
Blocks: 554013
I think we should definitely send a warning to the console about problems in the path data. Can you add:

  // ReportToConsole

comments at appropriate places so that when someone comes to implement that function properly they notice to use it there. It probably means changing some of the return values from void back to nsresult.
(Assignee)

Comment 3

9 years ago
Created attachment 435403 [details] [diff] [review]
with ReportToConsole


You and your ReportToConsole obsession ;-)
Attachment #433811 - Attachment is obsolete: true
Attachment #435403 - Flags: review?(jwatt)
Attachment #433811 - Flags: review?(jwatt)
:-P

So I may be missing something, but where are the ReportToConsole comments in the patch?
(Assignee)

Comment 5

9 years ago
In one place it's

+      nsresult rv = parser.Parse(*aValue);
+      if (NS_FAILED(rv)) {
+        ReportAttributeParseFailure(GetOwnerDoc(), aName, *aValue);
+      }

In the other cases the call stack traces back to here...

http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGElement.cpp#299

So when they fail you get the effect you want.
(Assignee)

Comment 6

9 years ago
And ReportAttributeParseFailure is just a wrapper for ReportToConsole
Ah, sorry, when I did a quick search for 'ReportToConsole' and got nothing I thought you must have lost the comments from your diff.

One other question: why the changes to nsSVGTransformList::SetValueString? I know we're changing the handling for path data because of Appendix F.5, but I'm not aware of a similar requirement for the 'transform' attribute.

Other than that it looks good I think.
(Assignee)

Comment 8

9 years ago
We're compatible with Opera if we change everything, but I can't find the transform requirement either. Oh for consistency in the specification.

I'll do another patch that just has the path change in it and we can raise the other parsing with w3c.
(Assignee)

Updated

9 years ago
Attachment #435403 - Attachment is obsolete: true
Attachment #435403 - Flags: review?(jwatt)
Comment on attachment 435403 [details] [diff] [review]
with ReportToConsole

r=jwatt with the transform changes dropped, and could you add a comment like the following for the OOM return in PathFini:

// mArguments, mNumCommands and mNumArguments are left in a consistent state
Attachment #435403 - Attachment is obsolete: false
Attachment #435403 - Flags: review+
(Assignee)

Comment 10

9 years ago
Created attachment 436281 [details] [diff] [review]
address review comments
Attachment #435403 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #436281 - Attachment is patch: true
(Assignee)

Comment 11

9 years ago
Created attachment 436284 [details] [diff] [review]
leave NS_ERROR in pathseg alone too
Attachment #436281 - Attachment is obsolete: true
(Assignee)

Comment 12

9 years ago
pushed http://hg.mozilla.org/mozilla-central/rev/f6c53a95a98a
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.