[webkit-reviews] review denied: [Bug 62599] Errors encountered within SVG documents should be reported to the console : [Attachment 97028] [style] Preliminary test (don't cq+), reports SVGLength parse errors on <circle> elements

[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 97028: [style] Preliminary test (don't cq+), reports SVGLength parse
errors on <circle> elements
https://bugs.webkit.org/attachment.cgi?id=97028&action=review

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

Good first step, r- because of some comments:

> Source/WebCore/svg/SVGCircleElement.cpp:84
>	   if (rBaseValue().value(this) < 0.0)
>	       document()->accessSVGExtensions()->reportError("A negative value
for circle <r> is not allowed");

Do we have any lengths that are allowed to be negative? If not, we could extend
SVGLength to bail if it sees negative values, and reuse your new comon error
reporting code path.
We should check the spec closely.

> Source/WebCore/svg/SVGCircleElement.cpp:96
> +    if (ec) {
> +	   document()->accessSVGExtensions()->reportError("Invalid value for
circle attribute " +
> +							 
attr->name().toString() + "=\"" +
> +							  attr->value() +
"\"");
> +    }

This could go into SVGElement as protected method
"reportAttributeParsingError(const QualifiedName& attrName, const String&
errorMessage)".

> Source/WebCore/svg/SVGLength.h:79
> +    static SVGLength create(SVGLengthMode, const String&, ExceptionCode&);

I wouldn't name it create, as it's misleading. We only use static create
methods whenever we return a PassOwnPtr or PassRefPtr.
"construct" is a better name, if you create an object on the stack and return
it.