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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 97466: 0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded
https://bugs.webkit.org/attachment.cgi?id=97466&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=97466&action=review

r=me. This would be OK to land as is, but I think there is room for
improvement, so I said commit-queue-.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:202
> +	   frame->domWindow()->console()->addMessage(JSMessageSource,
LogMessageType, level, message,
> +						    
parserLineNumber(document), document->documentURI());

This line breaking is not our style; for one thing the WebKit style guide tells
you not to line up subsequent lines with the open parenthesis at the start of
the arguments. I suggest just leaving this all on one line.

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

I don’t think this function needs “if needed” in its name.

> Source/WebCore/svg/SVGElement.cpp:108
> +    if (error == NoError || !document())

There’s no such thing as an element with 0 for its document(), so that check is
both unnecessary and untestable.

> Source/WebCore/svg/SVGElement.cpp:111
> +    String errorString = "<" + tagName() + "> attribute " +
attribute->name().toString() + "=\"" + attribute->value() + "\"";

For single characters and String concatenation you should be able to just use a
character, not a string, and this is slightly more efficient. While I haven't
tested it, you should be able to use '<' and '"' instead of "<" and "\"".

> Source/WebCore/svg/SVGElement.h:127
> +    void reportAttributeParsingErrorIfNeeded(const SVGParsingError&,
Attribute*);

To pass an enum you would just use SVGParsingError, not const SVGParsingError&.
The latter is less efficient. If an error was an structure or some other kind
of non-scalar, then const& would probably be OK.

> Source/WebCore/svg/SVGParsingError.h:34
> +enum SVGParsingError {

If this enum is included in only one header, I see no value in putting it into
a separate header file. If there was some file that needed to use the enum
without sing SVGElement, then the separate file would be worthwhile.

Also, there is no reason to put a RIM copyright on this, unless perhaps someone
from RIM wrote this enum?

> Source/WebCore/svg/SVGParsingError.h:35
> +    NoError = 0,

I don’t think there’s a lot of value in specifying "= 0" here. The first
element is always zero, and we don’t depend on the fact it’s zero in any code.