Bug 62599 - Errors encountered within SVG documents should be reported to the console
Summary: Errors encountered within SVG documents should be reported to the console
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on: 65726
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-13 14:16 PDT by Tim Horton
Modified: 2011-08-04 15:48 PDT (History)
10 users (show)

See Also:


Attachments
Preliminary test (don't cq+), reports SVGLength parse errors on <circle> elements (4.54 KB, patch)
2011-06-13 16:08 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
[style] Preliminary test (don't cq+), reports SVGLength parse errors on <circle> elements (4.49 KB, patch)
2011-06-13 16:15 PDT, Tim Horton
zimmermann: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.23 MB, application/zip)
2011-06-13 16:48 PDT, WebKit Review Bot
no flags Details
0001 - Add SVGElement::reporteAttributeParsingErrorIfNeeded (9.96 KB, patch)
2011-06-15 10:57 PDT, Tim Horton
rwlbuis: review-
Details | Formatted Diff | Diff
0002 - Add SVGLength::construct (4.85 KB, patch)
2011-06-15 10:59 PDT, Tim Horton
ossy: review-
Details | Formatted Diff | Diff
0003 - Make use of SVGLength::construct (51.13 KB, patch)
2011-06-15 11:03 PDT, Tim Horton
ossy: review-
Details | Formatted Diff | Diff
0004 - Update tests to match new console output (7.79 KB, patch)
2011-06-15 11:05 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.52 MB, application/zip)
2011-06-15 11:23 PDT, WebKit Review Bot
no flags Details
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded (12.09 KB, patch)
2011-06-15 21:47 PDT, Tim Horton
zimmermann: review-
Details | Formatted Diff | Diff
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded (12.02 KB, patch)
2011-06-16 09:53 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded (12.25 KB, patch)
2011-06-16 09:58 PDT, Tim Horton
sam: review-
Details | Formatted Diff | Diff
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded (12.73 KB, patch)
2011-06-16 10:59 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
0001 - Add SVGElement::reportAttributeParsingError (10.61 KB, patch)
2011-06-16 14:31 PDT, Tim Horton
thorton: review-
Details | Formatted Diff | Diff
0001 - Add SVGElement::reportAttributeParsingError (12.61 KB, patch)
2011-06-16 14:36 PDT, Tim Horton
zimmermann: review-
Details | Formatted Diff | Diff
0001 - Add SVGElement::reportAttributeParsingError (4.78 KB, patch)
2011-06-17 11:21 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
0002 - Add SVGLength::construct (4.00 KB, patch)
2011-07-05 16:15 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
0002 - Add SVGLength::construct (rebase, add <rdar>) (4.07 KB, patch)
2011-07-19 11:17 PDT, Tim Horton
zimmermann: review+
zimmermann: commit-queue-
Details | Formatted Diff | Diff
0002 - Add SVGLength::construct (12.73 KB, patch)
2011-07-20 17:35 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
0003 - Make use of SVGLength::construct (and update the tests) (60.79 KB, patch)
2011-07-21 11:11 PDT, Tim Horton
zimmermann: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2011-06-13 14:16:59 PDT
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!)
Comment 1 Tim Horton 2011-06-13 16:08:32 PDT
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.
Comment 2 WebKit Review Bot 2011-06-13 16:10:37 PDT
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.
Comment 3 Tim Horton 2011-06-13 16:15:07 PDT
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...
Comment 4 WebKit Review Bot 2011-06-13 16:47:58 PDT
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
Comment 5 WebKit Review Bot 2011-06-13 16:48:02 PDT
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
Comment 6 Nikolas Zimmermann 2011-06-14 01:53:48 PDT
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.
Comment 7 Robert Longson 2011-06-14 02:43:42 PDT
> 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.
Comment 8 Nikolas Zimmermann 2011-06-14 03:37:57 PDT
(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?
Comment 9 Tim Horton 2011-06-14 10:05:48 PDT
> 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!
Comment 10 Tim Horton 2011-06-14 10:43:02 PDT
> Even better: it looks like we can add *new* ExceptionCodes (see SVGException.h).

Never mind that part! Those are well-defined in the spec.
Comment 11 Tim Horton 2011-06-15 10:57:24 PDT
Created attachment 97324 [details]
0001 - Add SVGElement::reporteAttributeParsingErrorIfNeeded
Comment 12 Tim Horton 2011-06-15 10:59:12 PDT
Created attachment 97325 [details]
0002 - Add SVGLength::construct
Comment 13 Tim Horton 2011-06-15 11:03:41 PDT
Created attachment 97327 [details]
0003 - Make use of SVGLength::construct
Comment 14 Tim Horton 2011-06-15 11:05:21 PDT
Created attachment 97328 [details]
0004 - Update tests to match new console output
Comment 15 Tim Horton 2011-06-15 11:06:27 PDT
Can I mark patches as depending on each other? None of these are going to build if it does them all separately...
Comment 16 WebKit Review Bot 2011-06-15 11:11:30 PDT
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
Comment 17 WebKit Review Bot 2011-06-15 11:15:01 PDT
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
Comment 18 Gustavo Noronha (kov) 2011-06-15 11:19:29 PDT
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
Comment 19 Gustavo Noronha (kov) 2011-06-15 11:23:25 PDT
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
Comment 20 WebKit Review Bot 2011-06-15 11:23:51 PDT
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
Comment 21 WebKit Review Bot 2011-06-15 11:23:57 PDT
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
Comment 22 Adele Peterson 2011-06-15 11:35:36 PDT
(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.
Comment 23 Tim Horton 2011-06-15 12:22:01 PDT
(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.
Comment 24 Rob Buis 2011-06-15 14:36:34 PDT
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
Comment 25 Tim Horton 2011-06-15 21:47:18 PDT
Created attachment 97397 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded
Comment 26 Nikolas Zimmermann 2011-06-16 01:26:34 PDT
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 <zimmermann@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.
Comment 27 Tim Horton 2011-06-16 09:00:28 PDT
(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 <zimmermann@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?

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.
Comment 28 Tim Horton 2011-06-16 09:53:22 PDT
Created attachment 97454 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded

Made all changes except tests (which come in 0004).
Comment 29 Tim Horton 2011-06-16 09:58:02 PDT
Created attachment 97455 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded
Comment 30 Sam Weinig 2011-06-16 10:06:49 PDT
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.
Comment 31 Tim Horton 2011-06-16 10:59:42 PDT
Created attachment 97466 [details]
0001 - Add SVGElement::reportAttributeParsingErrorIfNeeded
Comment 32 Darin Adler 2011-06-16 12:41:30 PDT
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.
Comment 33 Tim Horton 2011-06-16 14:05:14 PDT
(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.
Comment 34 Tim Horton 2011-06-16 14:31:58 PDT
Created attachment 97499 [details]
0001 - Add SVGElement::reportAttributeParsingError
Comment 35 Tim Horton 2011-06-16 14:36:51 PDT
Created attachment 97502 [details]
0001 - Add SVGElement::reportAttributeParsingError
Comment 36 Dirk Schulze 2011-06-16 22:19:56 PDT
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.
Comment 37 Nikolas Zimmermann 2011-06-17 00:16:29 PDT
(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!
Comment 38 Nikolas Zimmermann 2011-06-17 03:02:50 PDT
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.
Comment 39 Nikolas Zimmermann 2011-06-17 03:04:46 PDT
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();
Comment 40 Nikolas Zimmermann 2011-06-17 03:06:00 PDT
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?
Comment 41 Nikolas Zimmermann 2011-06-17 03:06:54 PDT
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.
Comment 42 Csaba Osztrogonác 2011-06-17 10:19:26 PDT
Comment on attachment 97325 [details]
0002 - Add SVGLength::construct

r- to let Qt EWS release this patch
Comment 43 Csaba Osztrogonác 2011-06-17 10:19:45 PDT
Comment on attachment 97327 [details]
0003 - Make use of SVGLength::construct

r- to let Qt EWS release this patch
Comment 44 Tim Horton 2011-06-17 11:21:11 PDT
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.
Comment 45 Eric Seidel (no email) 2011-06-18 13:22:15 PDT
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.
Comment 46 Rob Buis 2011-06-21 14:23:13 PDT
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.
Comment 47 Tim Horton 2011-06-21 14:25:29 PDT
(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.
Comment 48 Darin Adler 2011-07-01 16:48:09 PDT
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?
Comment 49 WebKit Review Bot 2011-07-01 17:42:14 PDT
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: hausmann@webkit.org)
The commit-queue is continuing to process your patch.
Comment 50 WebKit Review Bot 2011-07-01 17:43:58 PDT
Comment on attachment 97625 [details]
0001 - Add SVGElement::reportAttributeParsingError

Clearing flags on attachment: 97625

Committed r90304: <http://trac.webkit.org/changeset/90304>
Comment 51 WebKit Review Bot 2011-07-01 17:44:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 Tim Horton 2011-07-05 16:15:18 PDT
Created attachment 99759 [details]
0002 - Add SVGLength::construct
Comment 53 Tim Horton 2011-07-05 16:18:04 PDT
Reopening because *this is not over* :-)
Comment 54 Tim Horton 2011-07-05 17:53:50 PDT
<rdar://problem/9727074>
Comment 55 Tim Horton 2011-07-19 11:17:43 PDT
Created attachment 101347 [details]
0002 - Add SVGLength::construct (rebase, add <rdar>)
Comment 56 Nikolas Zimmermann 2011-07-20 00:30:38 PDT
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?
Comment 57 Tim Horton 2011-07-20 09:51:44 PDT
(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!
Comment 58 Nikolas Zimmermann 2011-07-20 12:47:11 PDT
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
Comment 59 Tim Horton 2011-07-20 17:35:39 PDT
Created attachment 101534 [details]
0002 - Add SVGLength::construct

Swapped back to having a separate SVGParsingError.h
Comment 60 WebKit Review Bot 2011-07-20 19:05:23 PDT
Comment on attachment 101534 [details]
0002 - Add SVGLength::construct

Clearing flags on attachment: 101534

Committed r91437: <http://trac.webkit.org/changeset/91437>
Comment 61 WebKit Review Bot 2011-07-20 19:05:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 62 Tim Horton 2011-07-21 10:20:28 PDT
Reopening because there are still more patches.
Comment 63 Tim Horton 2011-07-21 11:11:53 PDT
Created attachment 101607 [details]
0003 - Make use of SVGLength::construct (and update the tests)
Comment 64 WebKit Review Bot 2011-07-21 13:00:17 PDT
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
Comment 65 Nikolas Zimmermann 2011-07-21 13:12:36 PDT
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..
Comment 66 Tim Horton 2011-08-04 15:16:37 PDT
(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.
Comment 67 Tim Horton 2011-08-04 15:48:26 PDT
(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.