[webkit-reviews] review denied: [Bug 62599] Errors encountered within SVG documents should be reported to the console : [Attachment 97397] 0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 62599: Errors encountered within SVG documents should be reported to the
console
https://bugs.webkit.org/show_bug.cgi?id=62599

Attachment 97397: 0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded
https://bugs.webkit.org/attachment.cgi?id=97397&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97397&action=review

Still r-, because of a copyright issue and it doesn't apply on mac/cr-mac at
the moment.

> Source/WebCore/ChangeLog:14
> +	   Include the SVG document's URI when writing to the Web Inspector
> +	   console, so that the UI displays both the filename and the line
number.

Great! This needs new tests though....
I guess you want to move all SVG*Element files to the new error reporting
framework in a follow-up patch? That would be acceptable IMHO.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:19926
> +				2D3A0E3613A7D76100E85AF0 /* SVGParsingError.h
in Headers */,

You might want to use "sort-XCode-project-file
Source/WebCore.xcodejproj/project.pxbproj" before submitting. This is not
needed, but if everyone does it, the file stays clean.

> Source/WebCore/svg/SVGElement.cpp:107
> +void SVGElement::reportAttributeParsingErrorIfNeeded(const SVGParsingError&
error,
> +							Attribute* attribute)

No need to wrap lines here, we don't use an 80 or 100 char limit at all.

> Source/WebCore/svg/SVGElement.cpp:123
> +    if (error & NegativeValueForbiddenError) {
> +	   extensions->reportError("Invalid negative value for " +
errorString);
> +	   return;
> +    }
> +
> +    if (error & ParsingAttributeFailedError) {
> +	   extensions->reportError("Invalid value for " + errorString);
> +	   return;
> +    }

I'm just realizing this is mutally exclusive. And I think we never want to
report more than one error for a single attribute that failed to parse.
So just use error == here.

> Source/WebCore/svg/SVGParsingError.h:2
> + * Copyright (C) 2011 Nikolas Zimmermann <zimmermann at kde.org>

Oh please use the Research in Motion copyright here.
I'm not sure whether you're allowed to submit this under your name, don't you
want to use Apple copyright?

> Source/WebCore/svg/SVGParsingError.h:31
> +    ParsingAttributeFailedError = 1 << 0,
> +    NegativeValueForbiddenError = 1 << 1

As explained above the flags probably don't make much sense. If we want
multiple flags, we have to pass "unsigned" as error parameter to the report..()
function otherwhise we couldn't use combinations like "Foo | Bar" as error
codes.
But I think we never want that, so just remove the = 0, = 1 << .. lines here
and leave the rest as-is.