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!)
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.
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.
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...
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 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.
> 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.
(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?
> 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 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
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
(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.
(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 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 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.
(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 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 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.
(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 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.
(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 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 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.
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.
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.
(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 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?
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 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?
(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 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 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..
(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.
(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.
2011-06-13 16:08 PDT, Tim Horton
2011-06-13 16:15 PDT, Tim Horton
webkit.review.bot: commit-queue-
2011-06-13 16:48 PDT, WebKit Review Bot
2011-06-15 10:57 PDT, Tim Horton
2011-06-15 10:59 PDT, Tim Horton
2011-06-15 11:03 PDT, Tim Horton
2011-06-15 11:05 PDT, Tim Horton
2011-06-15 11:23 PDT, WebKit Review Bot
2011-06-15 21:47 PDT, Tim Horton
2011-06-16 09:53 PDT, Tim Horton
2011-06-16 09:58 PDT, Tim Horton
2011-06-16 10:59 PDT, Tim Horton
2011-06-16 14:31 PDT, Tim Horton
2011-06-16 14:36 PDT, Tim Horton
2011-06-17 11:21 PDT, Tim Horton
2011-07-05 16:15 PDT, Tim Horton
2011-07-19 11:17 PDT, Tim Horton
zimmermann: commit-queue-
2011-07-20 17:35 PDT, Tim Horton
2011-07-21 11:11 PDT, Tim Horton
webkit.review.bot: commit-queue-