Bug 118292 – Move SVGTests attributes parsing to SVGGraphicsElement

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

[Chris Dumez]

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/

[Chris Dumez]

Created attachment 205894 [details]
Patch

[Dirk Schulze]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 205941 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 205971 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 205971 [details]
Patch for landing

Clearing flags on attachment: 205971

Committed r152343: <http://trac.webkit.org/changeset/152343>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.