Closed Bug 553905 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file, 3 obsolete files)

Assignee: nobody → longsonr
Attached patch patch (obsolete) — Splinter Review
Attachment #433811 - Flags: review?(jwatt)
Blocks: ietestcenter
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.
Attached patch with ReportToConsole (obsolete) — Splinter Review
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?
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.
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.
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.
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+
Attached patch address review comments (obsolete) — Splinter Review
Attachment #435403 - Attachment is obsolete: true
Attachment #436281 - Attachment is patch: true
Attachment #436281 - Attachment is obsolete: true
pushed http://hg.mozilla.org/mozilla-central/rev/f6c53a95a98a
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.