Bug 118292

Summary: Move SVGTests attributes parsing to SVGGraphicsElement
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: SVGAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, d-r, fmalita, krit, laszlo.gombos, pdr, rwlbuis, schenney, thorton, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
darin: review+, darin: commit-queue-
Patch for landing none

Description Chris Dumez 2013-07-02 04:47:04 PDT
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/
Comment 1 Chris Dumez 2013-07-02 05:10:31 PDT
Created attachment 205894 [details]
Patch
Comment 2 Dirk Schulze 2013-07-02 12:20:41 PDT
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 3 Chris Dumez 2013-07-02 13:06:28 PDT
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.
Comment 4 Chris Dumez 2013-07-02 13:19:28 PDT
Created attachment 205941 [details]
Patch
Comment 5 Darin Adler 2013-07-02 15:28:45 PDT
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.
Comment 6 Chris Dumez 2013-07-03 00:03:57 PDT
(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.
Comment 7 Chris Dumez 2013-07-03 00:04:50 PDT
Created attachment 205971 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2013-07-03 01:04:37 PDT
Comment on attachment 205971 [details]
Patch for landing

Clearing flags on attachment: 205971

Committed r152343: <http://trac.webkit.org/changeset/152343>
Comment 9 WebKit Commit Bot 2013-07-03 01:04:40 PDT
All reviewed patches have been landed.  Closing bug.