[webkit-reviews] review denied: [Bug 26371] SVGLangSpace duplicates xml:lang and xml:space values : [Attachment 31239] First attempt

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 26371: SVGLangSpace duplicates xml:lang and xml:space values
https://bugs.webkit.org/show_bug.cgi?id=26371

Attachment 31239: First attempt
https://bugs.webkit.org/attachment.cgi?id=31239&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Hi Rob,

nice patch, going in the right direction :-)

> -void SVGElement::setXmlbase(const String& value, ExceptionCode&)
> +void SVGElement::setXmlbase(const String&, ExceptionCode& ec)
>  {
> -    setAttribute(XMLNames::baseAttr, value);
> +    //setAttribute(XMLNames::baseAttr, value);
> +    ec = NO_MODIFICATION_ALLOWED_ERR;
>  }
Is that change related? It contains commented code, hence I'm wondering.
I guess you wanted to fix "attribute [ConvertNullToNullString] DOMString
xmlbase setter raises(DOMException);"

I'd rather commit this in a seperated patch with an associated testcase.

> -void SVGLangSpace::setXmllang(const AtomicString& xmlLang)
> +void SVGLangSpace::setXmllang(const AtomicString&/*, ExceptionCode &ec*/)
>  {
> -    m_lang = xmlLang;
> +    //ec = NO_MODIFICATION_ALLOWED_ERR;
>  }
This either needs to throw an exception (then we need to correct the
SVGLangSpace.idl file as well, fixing the commented out "setter raises..."
lines).
I guess this is what W3C says IIRC. If you go for that, we need a testcase. If
you're on this anyway, I think it would make sense to go for that...

>  const AtomicString& SVGLangSpace::xmlspace() const
>  {
> -    if (!m_space) {
> -	   DEFINE_STATIC_LOCAL(const AtomicString, defaultString, ("default"));

> -	   return defaultString;
> -    }
> -
> -    return m_space;
> -}
> -
> -void SVGLangSpace::setXmlspace(const AtomicString& xmlSpace)
> -{
> -    m_space = xmlSpace;
> +    return contextElement()->getAttribute(XMLNames::spaceAttr);
>  }
>  
> -bool SVGLangSpace::parseMappedAttribute(MappedAttribute* attr)
> +void SVGLangSpace::setXmlspace(const AtomicString&/*, ExceptionCode& ec*/)
>  {
> -    if (attr->name().matches(XMLNames::langAttr)) {
> -	   setXmllang(attr->value());
> -	   return true;
> -    } else if (attr->name().matches(XMLNames::spaceAttr)) {
> -	   setXmlspace(attr->value());
> -	   return true;
> -    }
> -
> -    return false;
> + //	 ec = NO_MODIFICATION_ALLOWED_ERR;
>  }
Same comment as above.

Everything else looks fine. Because of lacking testcases setting r- to clear it
out the review queue for now.