Bug 10362 – SVG needs to support SVGError events and some form of "error state"

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

SVG needs to support SVGError events and some form of "error state"

The spec calls for implementing an "error state":
http://www.w3.org/TR/SVG11/implnote.html#ErrorProcessing

"A highly perceivable indication of error shall occur. For visual rendering situations, an example of an indication of error would be to render a translucent colored pattern such as a checkerboard on top of the area where the SVG content is rendered."

Most desktop browser authors would consider such error behavior complete lunacy, given the current expectation of web content authors for browsers to silently render any erroneous HTML page.

Regardless, we should have some way to display errors to the content author, even if we silently render erroneous content nicely for the end user.  One possible solution would be to leverage an error console, such as is described by bug 10361.

[Rob Buis]

Created attachment 12145 [details]
First attempt

Note that this is just a start. I am not adding svg error reporting everywhere yet (so nothing is in place for reporting path syntax errors) since I'd like to know whether this is the right approach :)
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 12145 [details]
First attempt

I don't like the idea of stopping parsing.  Also fprintfs are a bad idea for the layout tests, we should just remove those.

I think reporting errors is great.  I don't think we should stop parsing however.  Long term we could consider offering some sort of "debug mode" in Safari which would stop parsing, etc.  But I still think that's probably a bad idea.

[Rob Buis]

Created attachment 12193 [details]
Improved patch

This patch should be much better. It reports a lot of errors that occur during parsing and DOM changes.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 12193 [details]
Improved patch

we discussed a bunch of improvements over irc, including making lineNumber(), etc pure virtual, using contextElement() instead of passing document() and cleaning up the long ifs which repeatedly called the same error message.

[Rob Buis]

Created attachment 12244 [details]
Improved patch

This should fix most of MacDome's issues. It doesnt seem worth it to make lineNumber() pure virtual, since three Tokenizer-derived classes in loader/ would need the same base implementation.
It is hard to get the js line numbers. Maybe that needs to be done later.
I also split out Tokenizer itself. That is not really required for this patch to work, but I thought it may make sense.
Cheers,

Rob.

[Sam Weinig]

Comment on attachment 12244 [details]
Improved patch

r- for a couple style issues

In Tokenizer.h (I do realize that this is just a copy and paste but we might as well get it right):

There should not be a trailing underscore.

+#ifndef Tokenizer_h_
+#define Tokenizer_h_

Put the brace on the same line as the class name.

+class Tokenizer
+{

Indent the initialization list and put the braces on two lines.

+    Tokenizer(bool viewSourceMode = false) 
+    : m_parserStopped(false)
+    , m_inViewSourceMode(viewSourceMode)
+    {}

You should also indent the whole class declaration.

Add comments to closing brace and #endif.

+}
+
+#endif


In SVGDocumentExtensions.cpp:

The lines (seen in both reportWarning() and reportError())

+        int lineNumber = 1;
+        if (m_doc->tokenizer())
+            lineNumber =  m_doc->tokenizer()->lineNumber();

could be clearer if written in one line, like:

    int lineNumber = m_doc->tokenizer() ? m_doc->tokenizer()->lineNumber() : 1;

(note that I got rid of the double space :) )


The changes in SVGFitToViewBox.h don't seem to have anything todo with the patch.


In SVGParserUtilities.h:

As long as you are changing the return value, please fix the &.

+        bool parsePoints(const String &points) const;

[Rob Buis]

Created attachment 12278 [details]
Improved patch

Deciding to not touch Tokenizer too much since it is unrelated to the patch. This patch should address
Sam's issues.
Cheers,

Rob.

[Rob Buis]

Landed in r18652.