Bug 62599 – Errors encountered within SVG documents should be reported to the console

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Tim Horton]

For example, if there is a parse error while parsing a string into an SVGLength, we should report this to the console!

Discussion started on #ksvg, can continue here (and, eventually, with prelimiary patches thrown in!)

[Tim Horton]

Created attachment 97027 [details]
Preliminary test (don't cq+), reports SVGLength parse errors on <circle> elements

Is the refactoring of parseMappedAttribute acceptable?

I changed reportMessage() so it passes the documentURI on to the console, so it shows the document (and, more importantly, the line number). This helps.

One interesting issue: the messages don't appear if you include the SVG in a HTML document with the <img> tag. I'm not sure why, I'm going to look into this next.

If this solution works for everyone, I'll forge ahead and apply it to all of the rest of the usages of SVGLength, and then eventually the other types.

[WebKit Review Bot]

Attachment 97027 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/svg/SVGCircleElement.cpp', ..." exit_code: 1

Source/WebCore/svg/SVGCircleElement.cpp:85:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/svg/SVGCircleElement.cpp:86:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/svg/SVGCircleElement.cpp:90:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebCore/svg/SVGCircleElement.cpp:91:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/svg/SVGLength.h:80:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/svg/SVGLength.h:80:  The parameter name "exceptionCode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 97028 [details]
[style] Preliminary test (don't cq+), reports SVGLength parse errors on <circle> elements

Just found the script to run the style checker locally, so I won't be surprised when I submit patches anymore...

[WebKit Review Bot]

Comment on attachment 97028 [details]
[style] Preliminary test (don't cq+), reports SVGLength parse errors on <circle> elements

Attachment 97028 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8837301

New failing tests:
svg/hixie/error/001.xml

[WebKit Review Bot]

Created attachment 97033 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 97028 [details]
[style] Preliminary test (don't cq+), reports SVGLength parse errors on <circle> elements

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.

[Robert Longson]

> 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.

In many cases, for instance x and y for rect, svg, foreignObject are lengths and may be negative.

[Nikolas Zimmermann]

(In reply to comment #7)
> > 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.

Ok, I was imprecise. I meant "Do we have any lengths that are allowed to be negative, which would go through Tims new code path?"
I looked again, and for the 'r' attribute, where now possibly raising two console errors:
    } else if (attr->name() == SVGNames::rAttr) {
        setRBaseValue(SVGLength::create(LengthModeOther, attr->value(), ec));
         if (rBaseValue().value(this) < 0.0)
             document()->accessSVGExtensions()->reportError("A negative value for circle <r> is not allowed");
...

+    if (ec) {
+        document()->accessSVGExtensions()->reportError("Invalid value for circle attribute " +
+                                                       attr->name().toString() + "=\"" +
+                                                       attr->value() + "\"");
...

The actual ExceptionCode is not used at all, so I propose adding following new enum to SVGElement.h:
enum SVGParsingError {
    NoError = 0,
    AttributeParsingFailedError = 1 << 0,
    NegativeValueForbiddenError = 1 << 1
};

Then I'd add following enum to SVGLength.h:

enum NegativeValuesMode {
    AllowNegativeValues,
    ForbidNegativeValues
};

And extend SVGLength::construct() by a NegativeValuesMode & SVGParsingError parameter:

SVGLength SVGLength::construct(const SVGLengthMode& mode, const String& valueAsString, SVGParsingError& error, NegativeValuesMode negativeValuesMode = AllowNegativeValues)
{
    ExceptionCode ec = 0;
    SVGLength length(mode);
    length.setValueAsString(valueAsString, ec);
    if (ec)
        error |= AttributeParsingFailedError;
    if (negativeValuesMode == ForbidNegativeValues && length.value() < 0)
        error |= NegativeValueForbiddenError;
    return length;
}

Then SVGCircleElement parseMappedAttribute would look as easy as:

void SVGCircleElement::parseMappedAttribute(Attribute* attr)
{
    SVGParsingError error = NoError;
    if (attr->name() == SVGNames::cxAttr)
        setCxBaseValue(SVGLength::construct(LengthModeWidth, attr->value(), error));
    else if (attr->name() == SVGNames::cyAttr)
        setCyBaseValue(SVGLength::construct(LengthModeHeight, attr->value(), error));
    else if (attr->name() == SVGNames::rAttr) {
        setRBaseValue(SVGLength::construct(LengthModeOther, attr->value(), error, ForbidNegativeValues));
    else if (SVGTests::parseMappedAttribute(attr)
                || SVGLangSpace::parseMappedAttribute(attr)
                || SVGExternalResourcesRequired::parseMappedAttribute(attr)) {
        return;
    }
    reportAttributeParsingErrorIfNeeded(error, attr);
}

void SVGElement::reportAttributeParsingErrorIfNeeded(const SVGParsingError& error, Attribute* attribute)
{
     if (error == NoError || !document())
         return;
    String errorString = "<" + tagName() + "> attribute " + attribute->name().toString() + "=\"" + attribute->value() + "\"";
    SVGDocumentExtensions* extensions = document()->accessSVGExtensions();
    if (error & NegativeValueForbiddenError) {
        extensions->reportError("Invalid negative value for " + errorString);
        return;
    }

    if (error & AttributeParsingFailedError) {
        extensions->reportError("Invalid value for " + errorString);
        return;
    }

    ASSERT_NOT_REACHED();
}


What do you think of this?

[Tim Horton]

> The actual ExceptionCode is not used at all, so I propose adding following new enum to SVGElement.h:
> enum SVGParsingError {
>     NoError = 0,
>     AttributeParsingFailedError = 1 << 0,
>     NegativeValueForbiddenError = 1 << 1
> };

I think eventually it would make sense to print different things for different ExceptionCodes, though, wouldn't it?

We could… map negative values when they're unwanted to INDEX_SIZE_ERR: "Index or size was negative, or greater than the allowed value." Even better: it looks like we can add *new* ExceptionCodes (see SVGException.h).

Ehh, maybe it doesn't make sense. Looking through the exception codes, the only one that seems like it would ever apply (besides INDEX_SIZE_ERR) is SYNTAX_ERR, the one we're currently using. (actually, it looks like we use NOT_SUPPORTED_ERR too)

> Then I'd add following enum to SVGLength.h:
> …………
> What do you think of this?

I think this is quite reasonable, and I'll make it go. One question: any reason we're using that enum instead of a single boolean for whether or not we accept negatives? If it's a readability/clarity thing, I'm cool with that, I'm just checking!

[Tim Horton]

> Even better: it looks like we can add *new* ExceptionCodes (see SVGException.h).

Never mind that part! Those are well-defined in the spec.

[Tim Horton]

Created attachment 97324 [details]
0001 - Add SVGElement::reporteAttributeParsingErrorIfNeeded

[Tim Horton]

Created attachment 97325 [details]
0002 - Add SVGLength::construct

[Tim Horton]

Created attachment 97327 [details]
0003 - Make use of SVGLength::construct

[Tim Horton]

Created attachment 97328 [details]
0004 - Update tests to match new console output

[Tim Horton]

Can I mark patches as depending on each other? None of these are going to build if it does them all separately...

[WebKit Review Bot]

Comment on attachment 97325 [details]
0002 - Add SVGLength::construct

Attachment 97325 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8843744

[WebKit Review Bot]

Comment on attachment 97327 [details]
0003 - Make use of SVGLength::construct

Attachment 97327 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8867123

[Gustavo Noronha (kov)]

Comment on attachment 97325 [details]
0002 - Add SVGLength::construct

Attachment 97325 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8857443

[Gustavo Noronha (kov)]

Comment on attachment 97327 [details]
0003 - Make use of SVGLength::construct

Attachment 97327 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8841835

[WebKit Review Bot]

Comment on attachment 97328 [details]
0004 - Update tests to match new console output

Attachment 97328 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8841831

New failing tests:
svg/hixie/error/015.xml
svg/custom/svg-parse-overflow-5.html
svg/custom/svg-parse-overflow-3.html
svg/custom/svg-parse-overflow-1.html
svg/hixie/error/001.xml
svg/custom/invalid-length-units.html
svg/custom/svg-parse-overflow-2.html
svg/hixie/error/007.xml
svg/custom/svg-parse-overflow-4.html

[WebKit Review Bot]

Created attachment 97333 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adele Peterson]

(In reply to comment #9)
> I think this is quite reasonable, and I'll make it go. One question: any reason we're using that enum instead of a single boolean for whether or not we accept negatives? If it's a readability/clarity thing, I'm cool with that, I'm just checking!

We tend to favor enums over booleans for code readability.

[Tim Horton]

(In reply to comment #22)
> We tend to favor enums over booleans for code readability.

Ok, sounds good.

I've cleared the review flags on the other three, and I'll get them through one at a time so things don't go wrong with the bots.

[Rob Buis]

Comment on attachment 97324 [details]
0001 - Add SVGElement::reporteAttributeParsingErrorIfNeeded

View in context: https://bugs.webkit.org/attachment.cgi?id=97324&action=review

This looks very good! But you need to include the new file in the other build files as well, like WebCore.gypi, for non OS X builds.

> Source/WebCore/ChangeLog:8
> +        Add SVGElement::reporteAttributeParsingErrorIfNeeded, which will

Typo in reporteAttributeParsingErrorIfNeeded

[Tim Horton]

Created attachment 97397 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

[Nikolas Zimmermann]

Comment on attachment 97397 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

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 <[e-mail]>

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.

[Tim Horton]

(In reply to comment #26)
> (From update of attachment 97397 [details])
> 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.
> 
> Great! This needs new tests though....

Does it? Once all of the patches attached are applied (there are four!), there are a bunch of tests which make sure this is working correctly (see patch 0004).

> 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.

Yep, the ones for SVGLength are attached to this bug, the rest will come later.
 
> > 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.

Oh, OK. Hadn't seen that, but I'll be sure to do it.

> > 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.

That explains why I couldn't find mention of the limit in the style guide. OK.

> > 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.

OK.

> > Source/WebCore/svg/SVGParsingError.h:2
> > + * Copyright (C) 2011 Nikolas Zimmermann <[e-mail]>
> 
> 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?

EXCELLENT POINT. That was totally automatic out of a habit grown over the last many years, which I'll have to break pronto. I'll look at another file for what we're supposed to put.

I'll find your correct address, I just grabbed the header from a nearby file that had you in it.

> > 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.

OK, sure.

[Tim Horton]

Created attachment 97454 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

Made all changes except tests (which come in 0004).

[Tim Horton]

Created attachment 97455 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

[Sam Weinig]

Comment on attachment 97455 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

View in context: https://bugs.webkit.org/attachment.cgi?id=97455&action=review

> Source/WebCore/svg/SVGParsingError.h:19
> +/*
> + * Copyright (C) Research In Motion Limited 2011. All rights reserved.
> + * Copyright (C) Apple, Inc. 2011. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */

In general, the license we like to use for new content is the two clause BSD one at found http://www.webkit.org/coding/bsd-license.html.

> Source/WebCore/svg/SVGParsingError.h:32
> +typedef enum {
> +    NoError = 0,
> +    ParsingAttributeFailedError,
> +    NegativeValueForbiddenError
> +} SVGParsingError;

This is c++, so the syntax for an enum is:

enum Foo {
    Bar
};

No typedef needed.

[Tim Horton]

Created attachment 97466 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

[Darin Adler]

Comment on attachment 97466 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

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.

[Tim Horton]

(In reply to comment #32)
> (From update of attachment 97466 [details])
> 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.

Ah, missed that one.

> > 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.

I was questioning that too. Fixed!

> > 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.

Fixed.

> > 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 "\"".

Can't do that (doesn't build, in an odd-but-somewhat-expected way: "invalid operands to binary expression ('int' and 'WTF::String')").

> > 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.

Fixed.

> > 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.

In future patches (0003 especially) it gets pulled into a lot of files. 0002 also uses it in SVGLength.h, which doesn't include Element/friends.

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

Initially came from a comment above, from Nikolas.

> > 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.

Ok, sure.

[Tim Horton]

Created attachment 97499 [details]
0001 - Add SVGElement::reportAttributeParsingError

[Tim Horton]

Created attachment 97502 [details]
0001 - Add SVGElement::reportAttributeParsingError

[Dirk Schulze]

Comment on attachment 97502 [details]
0001 - Add SVGElement::reportAttributeParsingError

Looks great in general. Is is possible to do a sample implementation? Will need inspector tests as well. At least reportMessage should be testable right now.But I am not aware about inspector tests.

[Nikolas Zimmermann]

(In reply to comment #32)
> (From update of attachment 97466 [details])
> 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.
Oh I was not aware the style guide actually says that!

> 
> > 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.
I thought Tim meant to use !inDocument() here.
Can we get accurate parserLineNumber() information, if a node is not in the document?
Do we still want to report errors for newly created elements?
var mySVGRect = document.createElementNS("svgNamespace...", "rect");
mySVGRect.setAttribute("width", "-20px");
?
  
> > 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.
I'm interessting why this is more efficient - can you tell us more about it Darin? Or do you have a link handy for a page covering this topic?

If this is really a speed penalty to use const references for enums, we should nail it down in the style guide to disallow it, no?
 
> > 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?
I wrote the code, see the first comment from me on this page :-)

> 
> > 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.
Right, it's a leftover from the time where we used flags "FooError = 1 << 0", "BlubError = 1 << 1".

Thanks for the review!

[Nikolas Zimmermann]

Comment on attachment 97325 [details]
0002 - Add SVGLength::construct

View in context: https://bugs.webkit.org/attachment.cgi?id=97325&action=review

Some comments regarding the SVGLengtH::construct patch.

> Source/WebCore/svg/SVGLength.cpp:163
> +SVGLength SVGLength::construct(SVGLengthMode mode, const String& valueAsString,
> +                               SVGParsingError& parseError, SVGLengthNegativeValuesMode negativeValuesMode)

Don't wrap lines.

> Source/WebCore/svg/SVGLength.cpp:166
> +    SVGLength length = SVGLength(mode);

SVGLength length(mode);

> Source/WebCore/svg/SVGLength.cpp:171
> +    if (ec)
> +        parseError = ParsingAttributeFailedError;

I'd return here immediately after setting the parseError. I think a general parsing error should take precedence over negative values.

> Source/WebCore/svg/SVGLength.cpp:187
> +float SVGLength::rawValue() const
> +{
> +    return m_valueInSpecifiedUnits;
> +}

Move this right in the header please.

> Source/WebCore/svg/SVGLength.h:87
> +    static SVGLength construct(SVGLengthMode, const String&, SVGParsingError&,
> +                               SVGLengthNegativeValuesMode = AllowNegativeValues);

Don't wrap lines here.

[Nikolas Zimmermann]

Comment on attachment 97327 [details]
0003 - Make use of SVGLength::construct

View in context: https://bugs.webkit.org/attachment.cgi?id=97327&action=review

Some comments regarding the make-use-of-SVGLength-construct-patch:

> Source/WebCore/svg/SVGCircleElement.cpp:89
> +    else if (SVGTests::parseMappedAttribute(attr)
> +             || SVGLangSpace::parseMappedAttribute(attr)
> +             || SVGExternalResourcesRequired::parseMappedAttribute(attr)) { }
> +    else
> +        ASSERT_NOT_REACHED();

General style comment:

else if (SVGTests::parseMapped..
            || foo...) {
} else
    ASSERT_NOT_REACHED();

[Nikolas Zimmermann]

Comment on attachment 97328 [details]
0004 - Update tests to match new console output

View in context: https://bugs.webkit.org/attachment.cgi?id=97328&action=review

> LayoutTests/platform/mac/svg/hixie/error/001-expected.txt:1
> +CONSOLE MESSAGE: line 3: Error: Invalid value for <circle> attribute r="r"

These don't include the document URI, I guess these results are old?

[Nikolas Zimmermann]

Comment on attachment 97502 [details]
0001 - Add SVGElement::reportAttributeParsingError

View in context: https://bugs.webkit.org/attachment.cgi?id=97502&action=review

> Source/WebCore/ChangeLog:26
> +        * svg/SVGParsingError.h: Added.

In general this looks good, though r- as SVGParsingError.h is superfluous, just move it into SVGElement.h. I agree with Darin.

[Csaba Osztrogonác]

Comment on attachment 97325 [details]
0002 - Add SVGLength::construct

r- to let Qt EWS release this patch

[Csaba Osztrogonác]

Comment on attachment 97327 [details]
0003 - Make use of SVGLength::construct

r- to let Qt EWS release this patch

[Tim Horton]

Created attachment 97625 [details]
0001 - Add SVGElement::reportAttributeParsingError

(In reply to comment #40)
> These don't include the document URI, I guess these results are old?

1. Patches 2/3/4 are very old, I'll update them (probably one at a time) once 1 gets through.

2. However, the document URI doesn't seem to get printed there (?!?). It shows up in Web Inspector! Just not in that output. Or maybe there's something different about those documents? I'll take a look in a bit.

> In general this looks good, though r- as SVGParsingError.h is superfluous,
> just move it into SVGElement.h. I agree with Darin.

Ok, I've fixed that too.

[Eric Seidel (no email)]

Comment on attachment 97466 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

Cleared Darin Adler's review+ from obsolete attachment 97466 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Rob Buis]

Hi Tim,

(In reply to comment #44)
> 2. However, the document URI doesn't seem to get printed there (?!?). It shows up in Web Inspector! Just not in that output. Or maybe there's something different about those documents? I'll take a look in a bit.

Any update on that? Personally I hold off reviewing when I feel there are still some loose ends/questions to be answered.
Cheers,

Rob.

[Tim Horton]

(In reply to comment #46)
> Hi Tim,
> 
> (In reply to comment #44)
> > 2. However, the document URI doesn't seem to get printed there (?!?). It shows up in Web Inspector! Just not in that output. Or maybe there's something different about those documents? I'll take a look in a bit.
> 
> Any update on that? Personally I hold off reviewing when I feel there are still some loose ends/questions to be answered.
> Cheers,
> 
> Rob.

No updates there yet, I've been working on other things, waiting for the first part of this patch to go through.

I don't think this is related to this patch at all; as it stands, the few errors that do make it through (primarily the old "negative value" ones) *also* don't show up in this case, even in the current release.

Should probably be filed as a separate bug, if it's a bug at all.

[Darin Adler]

Comment on attachment 97625 [details]
0001 - Add SVGElement::reportAttributeParsingError

View in context: https://bugs.webkit.org/attachment.cgi?id=97625&action=review

> Source/WebCore/svg/SVGElement.cpp:112
> +    String errorString = "<" + tagName() + "> attribute " + attribute->name().toString() + "=\"" + attribute->value() + "\"";
> +    SVGDocumentExtensions* extensions = document()->accessSVGExtensions();

Is there a way to save the string-construction work if there is no console?

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

Could this be a switch?

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 97625 [details]:

http/tests/media/media-can-load-when-hidden.html bug 61341 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 97625 [details]
0001 - Add SVGElement::reportAttributeParsingError

Clearing flags on attachment: 97625

Committed r90304: <http://trac.webkit.org/changeset/90304>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Created attachment 99759 [details]
0002 - Add SVGLength::construct

[Tim Horton]

Reopening because *this is not over* :-)

[Tim Horton]

<rdar://problem/9727074>

[Tim Horton]

Created attachment 101347 [details]
0002 - Add SVGLength::construct (rebase, add <rdar>)

[Nikolas Zimmermann]

Comment on attachment 101347 [details]
0002 - Add SVGLength::construct (rebase, add <rdar>)

View in context: https://bugs.webkit.org/attachment.cgi?id=101347&action=review

Looks great, r=me with one comment. Though as you can't land on your own yet, this needs another iteration, I fear.

> Source/WebCore/svg/SVGLength.h:26
> +#include "SVGElement.h"

This include doesn't seem to be needed - can you please remove it?
Ah, I see it depends on SVGParsingError.... time to move it into a different header?

[Tim Horton]

(In reply to comment #56)
> (From update of attachment 101347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101347&action=review
> 
> Looks great, r=me with one comment. Though as you can't land on your own yet, this needs another iteration, I fear.
> 
> > Source/WebCore/svg/SVGLength.h:26
> > +#include "SVGElement.h"
> 
> This include doesn't seem to be needed - can you please remove it?
> Ah, I see it depends on SVGParsingError.... time to move it into a different header?

Err... from before:

> In general this looks good, though r- as SVGParsingError.h is superfluous, just move it into SVGElement.h. I agree with Darin.

I'm fine with either way, just let me know which way it should be and I'll go for it!

[Nikolas Zimmermann]

Comment on attachment 101347 [details]
0002 - Add SVGLength::construct (rebase, add <rdar>)

View in context: https://bugs.webkit.org/attachment.cgi?id=101347&action=review

>>> Source/WebCore/svg/SVGLength.h:26
>>> +#include "SVGElement.h"
>> 
>> This include doesn't seem to be needed - can you please remove it?
>> Ah, I see it depends on SVGParsingError.... time to move it into a different header?
> 
> Err... from before:

Oops, I think I forgot about this usecase for the seperate header in the last review!
I guess you'll see Darin? Can you get his okay to create the SVGParsingError.h header, to avoid having to pull-in SVGElement.h in SVGLength.h

[Tim Horton]

Created attachment 101534 [details]
0002 - Add SVGLength::construct

Swapped back to having a separate SVGParsingError.h

[WebKit Review Bot]

Comment on attachment 101534 [details]
0002 - Add SVGLength::construct

Clearing flags on attachment: 101534

Committed r91437: <http://trac.webkit.org/changeset/91437>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Reopening because there are still more patches.

[Tim Horton]

Created attachment 101607 [details]
0003 - Make use of SVGLength::construct (and update the tests)

[WebKit Review Bot]

Comment on attachment 101607 [details]
0003 - Make use of SVGLength::construct (and update the tests)

Attachment 101607 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9201856

New failing tests:
svg/hixie/error/013.xml

[Nikolas Zimmermann]

Comment on attachment 101607 [details]
0003 - Make use of SVGLength::construct (and update the tests)

View in context: https://bugs.webkit.org/attachment.cgi?id=101607&action=review

Looks great to me, r=me! Please take special care of Chromium, the bot is red.

> Source/WebCore/ChangeLog:9
> +        Make use of SVGLength::construct when parsing Length attributes

Another sentence should be added, what's now changed, you've only said what you changed.
Something regarding unify console output, etc..

[Tim Horton]

(In reply to comment #65)
> (From update of attachment 101607 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101607&action=review
> 
> Looks great to me, r=me! Please take special care of Chromium, the bot is red.
> 
> > Source/WebCore/ChangeLog:9
> > +        Make use of SVGLength::construct when parsing Length attributes
> 
> Another sentence should be added, what's now changed, you've only said what you changed.
> Something regarding unify console output, etc..

Committed in r92419, with additional test updates for qt and gtk and Chromium, and a revised changelog entry.

[Tim Horton]

(In reply to comment #66)
> Committed in r92419, with additional test updates for qt and gtk and Chromium, and a revised changelog entry.

Recommitted in r92423 due to some SVN madness.