Bug 26371 - SVGLangSpace duplicates xml:lang and xml:space values
Summary: SVGLangSpace duplicates xml:lang and xml:space values
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Enhancement
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-13 08:53 PDT by Rob Buis
Modified: 2022-07-15 23:01 PDT (History)
7 users (show)

See Also:


Attachments
First attempt (19.95 KB, patch)
2009-06-13 09:29 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (40.12 KB, patch)
2012-05-13 19:06 PDT, Rob Buis
eric: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2009-06-13 08:53:44 PDT
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.
Comment 1 Rob Buis 2009-06-13 09:29:10 PDT
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.
Comment 2 Nikolas Zimmermann 2009-06-18 10:10:03 PDT
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.
Comment 3 Rob Buis 2012-05-13 19:06:02 PDT
Created attachment 141628 [details]
Patch
Comment 4 Rob Buis 2012-05-13 19:21:06 PDT
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.
Comment 5 Eric Seidel (no email) 2012-08-01 15:50:28 PDT
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.
Comment 6 WebKit Review Bot 2012-08-01 16:09:35 PDT
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
Comment 7 Andreas Kling 2013-01-01 16:14:39 PST
This looks like a nice improvement, do you plan to fix it up and land it?
Comment 8 Brent Fulgham 2022-07-15 17:40:20 PDT
This code was refactored and removed.