Bug 97911 – DOMTokenList add/remove should use Vector<AtomicString> instead of Vector<String>

• Status: RESOLVED
• Resolution: WONTFIX
• Component: DOM

[Erik Arvidsson]

Darin Adler wrote in https://bugs.webkit.org/show_bug.cgi?id=97335#c21

"I think we should investigate having the variadic string support use Vector<AtomicString> for the same reason that the single-argument functions use AtomicString."

[Erik Arvidsson]

Created attachment 166298 [details]
Patch

[Erik Arvidsson]

Comment on attachment 166298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166298&action=review

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1925
> +    Vector<Node*> tail;

Now the generated type is same for V8 and JSC.

[Ojan Vafai]

Comment on attachment 166298 [details]
Patch

WebCore/html changes LGTM.

[Adam Barth]

> "I think we should investigate having the variadic string support use Vector<AtomicString> for the same reason that the single-argument functions use AtomicString."

Did we investigate?

[Erik Arvidsson]

(In reply to comment #4)
> > "I think we should investigate having the variadic string support use Vector<AtomicString> for the same reason that the single-argument functions use AtomicString."
> 
> Did we investigate?

I'm not really sure what Darin wanted to investigate?

It is cleaner to use Vector<AtomicString> since all the other DOMTokenList functions use AtomicString.

[Sam Weinig]

Comment on attachment 166298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166298&action=review

> Source/WebCore/ChangeLog:9
> +        This makes DOMString... args use Vector<AtomicString> instead of Vector<String>.
> +

Why is that always the right decision.  I can see it making sense for DOMTokenList (where we want to treat the strings as tokens), but not in the general case.

> Source/WebCore/ChangeLog:12
> +        No new tests. No change in behavior.

Does this speed something up?

> Source/WebCore/bindings/js/JSDOMBinding.h:367
> +            indexedValue = jsValue.toString(exec)->value(exec);

jsValue.toWTFString(exec) would be slightly less code.  What is the return value for here? Should it return true even if there was an exception?

[Erik Arvidsson]

I did a perf test which does:

  element.classList.remove('aaaaa', 'bbbbb', 'ccccc');
  element.classList.add('aaaaa', 'bbbbb', 'ccccc');

Vector<String>
2941.29±2.54%
2935.29±2.46%
3035.28±2.99%

Vector<AtomicString>
2982.24±3.04%
3136.07±3.14%
3098.22±2.47%

Based on this simple test, using AtomicString is slightly faster but it is very close to the noise level.

Based on these I think it is cleaner to continue to use String, since it is more generic.

[Ryosuke Niwa]

Should we close this as WONTFIX then?

[Erik Arvidsson]

(In reply to comment #8)
> Should we close this as WONTFIX then?

I'm fine with that but I would still like to hear from Darin what he had in mind.