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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

See http://samples.msdn.microsoft.com/ietestcenter/svg112/svg_harness.htm?url=./svg112/svg/chapter_08.2.svg

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

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.

Comment 3

[Robert Longson [:longsonr]]

Attachment: with ReportToConsole

You and your ReportToConsole obsession ;-)

Comment 4

[Jonathan Watt [:jwatt]]

:-P

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

Comment 5

[Robert Longson [:longsonr]]

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.

Comment 6

[Robert Longson [:longsonr]]

And ReportAttributeParseFailure is just a wrapper for ReportToConsole

Comment 7

[Jonathan Watt [:jwatt]]

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.

Comment 8

[Robert Longson [:longsonr]]

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.

Comment 9

[Jonathan Watt [: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

Comment 10

[Robert Longson [:longsonr]]

Attachment: address review comments

Comment 11

[Robert Longson [:longsonr]]

Attachment: leave NS_ERROR in pathseg alone too

Comment 12

[Robert Longson [:longsonr]]

pushed http://hg.mozilla.org/mozilla-central/rev/f6c53a95a98a