Move SVGTests attributes parsing to SVGGraphicsElement instead of doing this in each of its subclasses. SVGGraphicsElement is now the one subclassing SVGTests after r152167. Corresponding Blink CL: https://codereview.chromium.org/18328007/
Created attachment 205894 [details] Patch
Comment on attachment 205894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205894&action=review Some questions to the patch. > Source/WebCore/svg/SVGSVGElement.cpp:255 > +bool SVGSVGElement::isSupportedAttribute(const QualifiedName& attrName) > +{ > + DEFINE_STATIC_LOCAL(HashSet<QualifiedName>, supportedAttributes, ()); > + if (supportedAttributes.isEmpty()) { > + SVGExternalResourcesRequired::addSupportedAttributes(supportedAttributes); > + SVGFitToViewBox::addSupportedAttributes(supportedAttributes); > + SVGLangSpace::addSupportedAttributes(supportedAttributes); > + SVGZoomAndPan::addSupportedAttributes(supportedAttributes); > + supportedAttributes.add(SVGNames::xAttr); > + supportedAttributes.add(SVGNames::yAttr); > + supportedAttributes.add(SVGNames::widthAttr); > + supportedAttributes.add(SVGNames::heightAttr); > + supportedAttributes.add(SVGNames::onzoomAttr); > + supportedAttributes.add(HTMLNames::onabortAttr); > + supportedAttributes.add(HTMLNames::onerrorAttr); > + supportedAttributes.add(HTMLNames::onunloadAttr); > + supportedAttributes.add(HTMLNames::onresizeAttr); > + supportedAttributes.add(HTMLNames::onscrollAttr); > + } > + return supportedAttributes.contains<SVGAttributeHashTranslator>(attrName); Can you explain this? Why did you add these elements here? They were not removed anywhere else and are not part of SVGTest. Weren't they part of SVGLocatable? Should also be described in the ChangeLog.
Comment on attachment 205894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205894&action=review >> Source/WebCore/svg/SVGSVGElement.cpp:255 >> + return supportedAttributes.contains<SVGAttributeHashTranslator>(attrName); > > Can you explain this? Why did you add these elements here? They were not removed anywhere else and are not part of SVGTest. Weren't they part of SVGLocatable? Should also be described in the ChangeLog. I was registering supported attributes and forwarding handling to the parent class in parseAttribute() and svgAttributeChanged() if the attribute is not supported. This is similar to other SVG classes. However, the real issue is that SVGSVGElement::svgAttributeChanged() falls back to calling SVGStyledElement::svgAttributeChanged() instead of SVGGraphicsElement::svgAttributeChanged() when the attribute is not supported. I will fix SVGSVGElement::svgAttributeChanged() instead and reupload a patch.
Created attachment 205941 [details] Patch
Comment on attachment 205941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205941&action=review Is our test coverage for this good? > Source/WebCore/svg/SVGGraphicsElement.cpp:119 > + } else if (SVGTests::parseAttribute(name, value)) { > + return; > } WebKit coding style would make this a separate if statement ("no else after return") and no braces around the single line "return" if body.
(In reply to comment #5) > (From update of attachment 205941 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205941&action=review > > Is our test coverage for this good? Based on my grep'ing, yes, test coverage seems good for this. > > > Source/WebCore/svg/SVGGraphicsElement.cpp:119 > > + } else if (SVGTests::parseAttribute(name, value)) { > > + return; > > } > > WebKit coding style would make this a separate if statement ("no else after return") and no braces around the single line "return" if body. Right, will fix and reupload.
Created attachment 205971 [details] Patch for landing
Comment on attachment 205971 [details] Patch for landing Clearing flags on attachment: 205971 Committed r152343: <http://trac.webkit.org/changeset/152343>
All reviewed patches have been landed. Closing bug.