[Webkit-unassigned] [Bug 84966] New: Improve SVG error reporting

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=84966

           Summary: Improve SVG error reporting
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: Unspecified
        OS/Version: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: SVG
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: schenney at chromium.org
                CC: zimmermann at kde.org, krit at webkit.org, pdr at google.com


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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.