Bug 84966 – Improve SVG error reporting

• Component: SVG
• Status: NEW

[Stephen Chenney]

SVG currently does a terrible job of reporting errors, particularly problems parsing content and invalid attribute values. While an significant project, it is very suitable for an intern or someone wishing to ramp up on the code.

In the context of bug 84363, Nikolas Zimmerman suggested the following to make it cleaner and easier to report errors in attribute parsing.

------------

We should revisit the warning generation itself, I'm not sure if its a good idea to sprinkle actual error message strings all over the codebase in future if we move more elements to generate parsing warnings on failures.
I'm sure this makes localization of these strings harder - is localization actually desired for warnings? I think so. (Not sure how localization is handled at all in WebKit or Inspector though.)

I wish the could would just look like:

if (attr->name() == SVGNames::orderAttr) {
    float x, y;
    if (parseNumberOptionalNumber(value, x, y) && x >= 1 && y >= 1) {
        setOrderXBaseValue(x);
        setOrderYBaseValue(y);
    } else
        reportSVGAttributeParsingError(this, attr);
    return;
}

-- To reach this simplicity, I propose to add reportSVGAttributeParsingError() as a free-function, placed in eg. SVGParsingErrors.h.

typedef void (*WarningStringGenerator)(const AtomicString& tagName, const AtomicString& elementName, const AtomicString& parsedValue, String& warningString);
void reportSVGAttributeParsingError(const SVGElement* element, Attribute* attribute)
{
    if (!element || !element->document() || !attribute)
        return;
    // Set up map from (tag name | attribute name) pairs to a function that returns the error String when given some context (here: the string value that we've tried to parse and failed).
    DEFINE_STATIC_LOCAL(HashMap<pair<AtomicStringImpl* tagName, AtomicStringImpl* attributeName>, WarningStringGenerator> >, s_tagNameAndAttributeToStringMap; ());
    if (s_tagNameAndAttributeToStringMap.isEmpty()) {
....
        s_tagNameAndAttributeToStringMap.set(make_pair(SVGNames::feConvolveMatrixTag.localName().impl(), SVGNames::orderAttr.localName().impl()), &feConvolveMatrixOrderWarning);
        s_tagNameAndAttributeToStringMap.set(make_pair(SVGNames::feConvolveMatrixTag.localName().impl(), SVGNames::edgeModeAttr.localName().impl()), &feConvolveMatrixEdgeModeWarning);
....
    }

    WarningStringGenerator generator = s_tagNameAndAttributeToStringMap.get(make_pair(element->localName().impl(), attribute->name().localName().impl()));
    ASSERT(generator); // If someone calls reportSVGAttributeParsingError, the element/attribute must be present in the map.

    String warningString;
    (*generator)(element->localName(), attribute->name().localName(), attribute->value(), warningString);
    element->document()->accessSVGExtensions()->reportWarning(warningString);
}
....
inline String parsingWarning(const String& tagName, const String& attributeName, const AtomicString& parsedValue)
{
    return tagName + ": problem parsing " + attributeName + "=\"" + parsedValue + "\". ";
}

void feConvolveMatrixOrderWarning(const AtomicString& tagName, const AtomicString& attributeName, AtomicString& parsedValue, String& warning)
{
    warning = parsingWarning(tagName, attributeName) + " Filtered element will not be displayed.";
}

void feConvolveMatrixEdgeModeWarning(const AtomicString& tagName, const AtomicString& attributeName, AtomicString& parsedValue, String& warning)
{
    warning = parsingWarning(tagName, attributeName) + " Filtered element will not be displayed.";
}
....

The benefits of this concept is a single place where strings passed around as warnings are contained. Also retrieving the warning message is much easier. Just add "reportSVGAttributeParsingError(localName(), attr)" in the failures branches in parseAttribute() for a specific attribute / element, and register a new warning in the SVGParsingErrors.h/cpp file.
What do you think?

[Philip Rogers]

I agree that our error reporting is horrendous and I think this is a good proposal. Two small points:
1) Should we change this to be more general, instead of just parse errors? Most of our properties go through strings, but do they all? E.g., animating a number from -1 to 1 when zero is not allowed.
2) I'm not sure we should localize errors, since authors presumably know English because all the tag names / properties / etc. are in English.

(In reply to comment #0)

[Blaze Burg]

This bug seems too broad to lead to specific patches. I recommend filing more targeted bugs and relating to this one.