SVGLangSpace duplicates xml:lang and xml:space values

• Component: SVG
• Status: RESOLVED CONFIGURATION CHANGED

[Rob Buis]

SVGLangSpace basically copies the xml:lang and xml:space attribute values into its own member vars. Instead it can just ask the current values of these attributes when needed, when having a contextElement
construction like already present in other base classes. Note also that xmlbase implementation in SVGStyleElement works that way.

[Rob Buis]

Created attachment 31239 [details]
First attempt

Here is an enhancement for SVGLangSpace. Note that FF, Opera and Batik do not seem to know the js
attributes xmllang and xmlspace at all, so it is hard to compare. Also really exceptions should be thrown when the attribute is set, but I think due to a bug that objC interfaces can't handle the idl setting of raises exception, that that may depend on another bug. I dont think this fixes a big issue but the old implementation wasted memory and processing time IMHO. All tests still pass, also because we do not really test these attributes except for one SVG text test.
Cheers,

Rob.

[Nikolas Zimmermann]

Comment on attachment 31239 [details]
First attempt

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.

[Rob Buis]

Created attachment 141628 [details]
Patch

[Rob Buis]

I checked, but on svg/dom/SVGStyleElement/script-tests/style-langspace.js Mozilla and Opera do not throw exceptions either and allow setting of the attributes, so I ignore the exception stuff for now.

[Eric Seidel (no email)]

Comment on attachment 141628 [details]
Patch

LGTM.  Unclear to me why isKnownAttribute is needed for the early returns.  It would also be OK to let execution continue and just fall off the end of parseMappedAttribute.

[WebKit Review Bot]

Comment on attachment 141628 [details]
Patch

Rejecting attachment 141628 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 at 128 (offset -1 lines).
patching file Source/WebCore/svg/SVGTitleElement.h
patching file Source/WebCore/svg/SVGUseElement.cpp
Hunk #1 FAILED at 158.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/svg/SVGUseElement.cpp.rej
patching file Source/WebCore/svg/SVGUseElement.h
Hunk #1 succeeded at 130 (offset 2 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13421215

[Andreas Kling]

This looks like a nice improvement, do you plan to fix it up and land it?

[Brent Fulgham]

This code was refactored and removed.