Bug 20709 – Implement HTML 5's HTMLElement.classList property

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Anthony Ricaud]

This will help speeding up a lot of code for developers.
http://www.w3.org/TR/html5/semantics.html#classlist

[Erik Arvidsson]

I'm interested in working on this.

How are CSS class names stored internally? For example if the class attribute is set to "a b c" is this stored as "a b c" or is it stored as a set with "a", "b" and "c"? If the former, is it worth changing it to a set internally in the case classList is accessed? The number of class names is usually pretty small (less than 10 in most cases) so maybe a vector is enough.

[Adam Roben (:aroben)]

*** Bug 26358 has been marked as a duplicate of this bug. ***

[Adam Roben (:aroben)]

See <http://www.whatwg.org/specs/web-apps/current-work/#domtokenlist-0> for the definition of the DOMTokenList interface, which the object returned by classList implements.

[Adam Roben (:aroben)]

(In reply to comment #1)
> How are CSS class names stored internally? For example if the class attribute
> is set to "a b c" is this stored as "a b c" or is it stored as a set with "a",
> "b" and "c"?

It is stored as a Vector with 3 items: "a", "b", "c". The original order is preserved.

> If the former, is it worth changing it to a set internally in the
> case classList is accessed? The number of class names is usually pretty small
> (less than 10 in most cases) so maybe a vector is enough.

I agree that in the common case a Vector seems fine. We could switch to using a set if the number of classes crosses some threshold, but maybe that isn't worth it. If we used a set we'd still need to have a way of preserving the original ordering of the classes, as this is required for classList.item(n).

Another possibility would be to use ListHashSet <http://trac.webkit.org/browser/trunk/JavaScriptCore/wtf/ListHashSet.h>.

[Adam Roben (:aroben)]

(In reply to comment #4)
> (In reply to comment #1)
> > How are CSS class names stored internally? For example if the class attribute
> > is set to "a b c" is this stored as "a b c" or is it stored as a set with "a",
> > "b" and "c"?
> 
> It is stored as a Vector with 3 items: "a", "b", "c". The original order is
> preserved.

You can see the implementation here: <http://trac.webkit.org/browser/trunk/WebCore/dom/ClassNames.h?rev=43365>.

[Adam Roben (:aroben)]

Erik found some existing tests for classList: <http://simon.html5.org/test/html/dom/reflecting/DOMTokenList/>.

[Adam Roben (:aroben)]

It looks like DOMTokenList provides a superset of ClassNames's functionality. Maybe a good way to go about this would be:

1) Rename ClassNames to DOMTokenList and ClassNamesData to DOMTokenListData
2) Expose the current functionality of DOMTokenList to JavaScript and add tests
3) Add the remaining DOMTokenList functionality (add, remove, toggle) and add tests

[Sylvain Pasche]

This feature was recently implemented in Gecko. You may be interested in the automated test [1] that could be easily transformed into a layout test. There are a few Gecko-isms that would need to be removed and you would need to provide is()/ok() implementations.

In the current spec the mutating methods (add/remove/toggle) are preserving whitespace. There was some discussions about an alternative approach of normalizing whitespace, which could make implementation more straightforward [2].
That change wasn't made for now, but if you think it would be better to go that way that's something that could be considered.

[1] http://hg.mozilla.org/mozilla-central/file/tip/content/base/test/test_classList.html
[2] http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-July/021653.html

[Erik Arvidsson]

I got something mostly working without any refactoring of ClassNames by just forwarding calls as needed. I'll upload a snapshot early next week before I start any refactoring.

[Erik Arvidsson]

The JSC bindins for HasIndexGetter in combination with ConvertNullStringTo=Null is broken so that the following fails in Safari (it works in Chromium):

document.body.className = 'x';
assertTrue(null === document.body.classList[10])

JSC generates code that checks the length and if the index is larger to or equal to the length it does not call the index getter.

[Erik Arvidsson]

Created attachment 43876 [details]
classList M1

This patch does not refactor ClassNames but instead uses ClassNames. I wanted something that more or less works before I do the refactoring and this is a good milestone.

I have a couple of questions:

Currently ClassNames converts all class names to lower case in quirks mode. HTML5 clearly states that DOMTokenList is case sensitive. We could store the non case folded class names and only do the case folding at runtime if needed. The downside is that I don't want to make class name matching slower since it seems crucial that this is as fast as it can get because it is used for matching CSS selectors.

Should the classList member be moved to RareData? NamedMappedAttrMap?

Should classList() be on StyledElement instead of HTMLElement?

Should I include ObjC bindings?

Do I need to upgrade WebKit/mac/MigrateHeaders.make?

[Erik Arvidsson]

Created attachment 64365 [details]
Patch

[WebKit Review Bot]

Attachment 64365 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3768133

[Sam Weinig]

Comment on attachment 64365 [details]
Patch

> -        typedef Vector<AtomicString, 8> StringVector;
>          String m_string;
> -        StringVector m_vector;
>          bool m_shouldFoldCase;
> +        bool m_folded;
> +        StringVector m_vector;
> +        StringVector m_vectorNoFold;

I am worried about the memory increase in adding a second vector here.  Have you done any memory benchmarking to make sure this doesn't regress things (eg. membuster)

> @@ -0,0 +1,151 @@
> +// Copyright (c) 2009 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

This is an invalid license header.  Please see http://webkit.org/coding/contributing.html for more on this.


> +++ b/WebCore/html/ClassList.h
> @@ -0,0 +1,44 @@
> +// Copyright (c) 2009 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Here too. (There are more as well).

r-.

[Erik Arvidsson]

Thanks for the review. I'm going on vacation but I'll want to get this done as soon as I come back.

(In reply to comment #14)

> > +        StringVector m_vectorNoFold;
> 
> I am worried about the memory increase in adding a second vector here.  Have you done any memory benchmarking to make sure this doesn't regress things (eg. membuster)

This is also worrying me. My current plan is to revert all the changes to ClassNames. Then add a member to ElementRareData that points to the ClassList. The ClassList uses the ClassNames in standards mode for fast contains and item lookup. For quirks mode there are two options. One is to have an internal ClassNames copy and the other option is to operate straight on the class attribute string.

[Erik Arvidsson]

Created attachment 66784 [details]
Patch

[Darin Adler]

Comment on attachment 66784 [details]
Patch

It seems unnecessary to define an abstract base class DOMTokenList and do everything through virtual functions just because we might have another derived class in the future. The same thing came up with NamedNodeMap and we decided to just put the concrete implementation into the base and save abstraction for later. So investigate combining DOMTokenList and ClassList into a single class.

[Erik Arvidsson]

(In reply to comment #17)
> (From update of attachment 66784 [details])
> It seems unnecessary to define an abstract base class DOMTokenList and do everything through virtual functions just because we might have another derived class in the future. The same thing came up with NamedNodeMap and we decided to just put the concrete implementation into the base and save abstraction for later. So investigate combining DOMTokenList and ClassList into a single class.

OK. I'll just fold it all into ClassList then. Should I rename the class to ClassListDOMTokenList?

(The dataset class that implements the DOMStringMap is called DatasetDOMStringMap.)

[Erik Arvidsson]

I looked at NamedNodeMap and now I understand what Darin meant. I'll rename ClassList to DOMTokenList.

[WebKit Review Bot]

Attachment 66784 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3938308

[Erik Arvidsson]

Created attachment 66817 [details]
Patch

[James Robinson]

Comment on attachment 66817 [details]
Patch

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

> WebCore/html/DOMTokenList.h:54
> +    virtual void ref();
> +    virtual void deref();
Per discussion in IRC, these should go away and this class should become RefCounted<DOMTokenList>

> WebCore/html/DOMTokenList.h:61
> +    virtual bool contains(const AtomicString& token, ExceptionCode&) const;
> +    virtual void add(const AtomicString& token, ExceptionCode& ec);
> +    virtual void remove(const AtomicString& token, ExceptionCode& ec);
> +    virtual bool toggle(const AtomicString& token, ExceptionCode& ec);
nit: don't need to name the ExceptionCode parameters here.  Arguably the AtomicString params could be unnamed as well (as they are in the Internal versions).

[WebKit Review Bot]

Attachment 66817 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3976276

[Erik Arvidsson]

I think I might just follow Weinig's work in 45358 after all.

I'm not arguing that his solution is better but we should be consistent.

[Erik Arvidsson]

Created attachment 66931 [details]
Patch

[Erik Arvidsson]

The latest patch follows Sam Weinig's pattern from bug 45358.

It would be great if DOMTokenList and DOMStringMap would not keep the Element alive but I don't think this is that of a common scenario. Like Sam wrote in bug 45358 we have a lot of places where this pattern is used and yes it would be good to improve them all but maybe that can be done in a separate bug?

[James Robinson]

Comment on attachment 66931 [details]
Patch

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

> WebCore/bindings/js/JSElementCustom.cpp:63
> +    markDOMObjectWrapper(markStack, globalData, element->optionalClassList());
Does this mean we'll eagerly create the classList object on every element that's wrapped from javascript?  It's not very optional in that case.  This seems a bit unfortunate.

Do you have to do anything for V8?

> WebCore/html/DOMTokenList.h:64
> +    virtual ~DOMTokenList() { }
> +
> +    virtual void ref();
> +    virtual void deref();
> +
> +    virtual unsigned length() const;
> +    virtual const AtomicString item(unsigned index) const;
> +    virtual bool contains(const AtomicString&, ExceptionCode&) const;
> +    virtual void add(const AtomicString&, ExceptionCode&);
> +    virtual void remove(const AtomicString&, ExceptionCode&);
> +    virtual bool toggle(const AtomicString&, ExceptionCode&);
> +    virtual String toString() const;
> +
> +    virtual void reset(const String&);
None of these should be virtual.

[Erik Arvidsson]

(In reply to comment #27)
> (From update of attachment 66931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66931&action=prettypatch
> 
> > WebCore/bindings/js/JSElementCustom.cpp:63
> > +    markDOMObjectWrapper(markStack, globalData, element->optionalClassList());
> Does this mean we'll eagerly create the classList object on every element that's wrapped from javascript?  It's not very optional in that case.  This seems a bit unfortunate.

No, optionalClassList does not create a DOMTokenList.

> Do you have to do anything for V8?

looking

[WebKit Review Bot]

Attachment 66931 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3914329

[Erik Arvidsson]

Created attachment 67985 [details]
Now with markDOMObjectWrapper and V8DOMWrapper::setHiddenWindowReference

[WebKit Review Bot]

Attachment 67985 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4035060

[Erik Arvidsson]

Created attachment 68606 [details]
Patch

[Darin Adler]

Comment on attachment 68606 [details]
Patch

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

Great work. Looks close to ready!

I am going to say review- but only because of the inefficient use of String here in the add and remove functions. If we add some test cases to prove we don’t have O(n^2) algorithms there then I think we’re ready to go.

> WebCore/dom/Element.cpp:1598
> +    if (hasRareData()) {
> +        ElementRareData* data = rareData();
> +        if (data && data->m_classList)
> +            data->m_classList->reset(newClassName);
> +    }

We’e normally use early return here:

    if (!hasRareData())
        return;
    ElementRareData* data = rareData();
    if (!data || !data->m_classList)
        return;
    data->m_classLiast->reset(newClassName);

But also, you could use optionalClassList for this:

    DOMTokenList* classList = optionalClassList();
    if (!classList)
        return;
    classList->reset(newClassName);

I think that’s the best way to write it. Or maybe even put that code in-line at the one call site and not have that function at all.

> WebCore/dom/StyledElement.cpp:224
> +    }
>      else {

Braces go on the same line as the else in the WebKit coding style.

> WebCore/html/DOMTokenList.cpp:38
> +namespace {
> +
> +bool validateToken(const AtomicString& token, ExceptionCode& ec)

WebKit code typically uses “static” to get internal linkage rather than an anonymous namespace, but perhaps this is something that we should change?

> WebCore/html/DOMTokenList.cpp:45
> +    const unsigned length = token.length();

WebKit code normally does not use const for local scalar variables. I see the attraction of doing so to state that a particular variable won’t change, but doing in sometimes and not others seems untidy, and it has no effect on generated code. I’d prefer that we not do it here, just to stay consistent.

> WebCore/html/DOMTokenList.cpp:62
> +        m_classNames = new SpaceSplitString(m_element->getAttribute(classAttr), false);

This can use fastGetAttribute.

> WebCore/html/DOMTokenList.cpp:91
> +     return containsInternal(token);

This line of code has an extra space.

> WebCore/html/DOMTokenList.cpp:108
> +    const AtomicString oldClassName(m_element->getAttribute(classAttr));

This can use const AtomicString& so it doesn’t ref and then deref.

This can use fastGetAttribute.

> WebCore/html/DOMTokenList.cpp:110
> +    if (oldClassName.isEmpty())
> +        m_element->setAttribute(classAttr, token);

Don’t we also want to do this when the old value consists entirely of whitespace? Is there a test case covering this?

> WebCore/html/DOMTokenList.cpp:112
> +        m_element->setAttribute(classAttr, oldClassName + " " + token);

This is the cleanest way to write the code, but it is slow and reallocates the string twice. You should use Vector<UChar> or StringBuilder for better efficiency.

> WebCore/html/DOMTokenList.cpp:133
> +    String output; // 3

Building up a String a character at a time is pathologically slow. We need to use Vector<UChar> instead of String here. I’d like to see a test case with a long class name or a class attribute with a lot of class names to verify the algorithm is not O(n^2).

> WebCore/html/DOMTokenList.cpp:185
> +    return m_element->getAttribute(classAttr);

Can use fastGetAttribute.

> WebCore/html/DOMTokenList.h:45
> +    ~DOMTokenList() { }

There is no need to explicitly declare and define this constructor.

> WebCore/html/DOMTokenList.idl:40
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +        [DontEnum] DOMString toString();
> +#endif

I don’t think this is the right way to make toString work. Isn’t it built in to the language already?

[WebKit Review Bot]

Attachment 68606 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4021142

[Erik Arvidsson]

Comment on attachment 68606 [details]
Patch

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

I'm adding a perf test as well.

>> WebCore/dom/Element.cpp:1598
>> +    }
> 
> We’e normally use early return here:
> 
>     if (!hasRareData())
>         return;
>     ElementRareData* data = rareData();
>     if (!data || !data->m_classList)
>         return;
>     data->m_classLiast->reset(newClassName);
> 
> But also, you could use optionalClassList for this:
> 
>     DOMTokenList* classList = optionalClassList();
>     if (!classList)
>         return;
>     classList->reset(newClassName);
> 
> I think that’s the best way to write it. Or maybe even put that code in-line at the one call site and not have that function at all.

Removed and inlined.

>> WebCore/dom/StyledElement.cpp:224
>>      else {
> 
> Braces go on the same line as the else in the WebKit coding style.

Done.

>> WebCore/html/DOMTokenList.cpp:38
>> +bool validateToken(const AtomicString& token, ExceptionCode& ec)
> 
> WebKit code typically uses “static” to get internal linkage rather than an anonymous namespace, but perhaps this is something that we should change?

Changed to static.

>> WebCore/html/DOMTokenList.cpp:45
>> +    const unsigned length = token.length();
> 
> WebKit code normally does not use const for local scalar variables. I see the attraction of doing so to state that a particular variable won’t change, but doing in sometimes and not others seems untidy, and it has no effect on generated code. I’d prefer that we not do it here, just to stay consistent.

Done.

>> WebCore/html/DOMTokenList.cpp:62
>> +        m_classNames = new SpaceSplitString(m_element->getAttribute(classAttr), false);
> 
> This can use fastGetAttribute.

Done.

>> WebCore/html/DOMTokenList.cpp:91
>> +     return containsInternal(token);
> 
> This line of code has an extra space.

Done.

>> WebCore/html/DOMTokenList.cpp:108
>> +    const AtomicString oldClassName(m_element->getAttribute(classAttr));
> 
> This can use const AtomicString& so it doesn’t ref and then deref.
> 
> This can use fastGetAttribute.

Done.

>> WebCore/html/DOMTokenList.cpp:110
>> +        m_element->setAttribute(classAttr, token);
> 
> Don’t we also want to do this when the old value consists entirely of whitespace? Is there a test case covering this?

The token cannot contain whitespace and invalid tokens are covered by the test.

>> WebCore/html/DOMTokenList.cpp:112
>> +        m_element->setAttribute(classAttr, oldClassName + " " + token);
> 
> This is the cleanest way to write the code, but it is slow and reallocates the string twice. You should use Vector<UChar> or StringBuilder for better efficiency.

Changed to StringBuilder

>> WebCore/html/DOMTokenList.cpp:133
>> +    String output; // 3
> 
> Building up a String a character at a time is pathologically slow. We need to use Vector<UChar> instead of String here. I’d like to see a test case with a long class name or a class attribute with a lot of class names to verify the algorithm is not O(n^2).

Changed to Vector<UChar>

>> WebCore/html/DOMTokenList.cpp:185
>> +    return m_element->getAttribute(classAttr);
> 
> Can use fastGetAttribute.

Done.

>> WebCore/html/DOMTokenList.h:45
>> +    ~DOMTokenList() { }
> 
> There is no need to explicitly declare and define this constructor.

Done.

>> WebCore/html/DOMTokenList.idl:40
>> +#endif
> 
> I don’t think this is the right way to make toString work. Isn’t it built in to the language already?

This is how other toString cases are done. I assume it is just so that the toString method is only added to the JS bindings.

[Erik Arvidsson]

Created attachment 68637 [details]
Took care of Darin's comments and added a perf test for remove

[WebKit Review Bot]

Attachment 68637 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4079094

[Erik Arvidsson]

Created attachment 68696 [details]
Trying to get this to build on GTK

[Darin Adler]

Comment on attachment 68606 [details]
Patch

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

>>> WebCore/html/DOMTokenList.cpp:110
>>> +        m_element->setAttribute(classAttr, token);
>> 
>> Don’t we also want to do this when the old value consists entirely of whitespace? Is there a test case covering this?
> 
> The token cannot contain whitespace and invalid tokens are covered by the test.

I think you may have misunderstood my question. I was talking about oldClassName containing entirely whitespace, not the token containing whitespace.

[Erik Arvidsson]

Comment on attachment 68606 [details]
Patch

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

>>>> WebCore/html/DOMTokenList.cpp:110
>>>> +        m_element->setAttribute(classAttr, token);
>>> 
>>> Don’t we also want to do this when the old value consists entirely of whitespace? Is there a test case covering this?
>> 
>> The token cannot contain whitespace and invalid tokens are covered by the test.
> 
> I think you may have misunderstood my question. I was talking about oldClassName containing entirely whitespace, not the token containing whitespace.

Indeed, I did misunderstand you.

Add needs some refinements though. Checking isEmpty is still a correct optimization. However, we should only add a space if the last character is not already a space.

[Darin Adler]

Comment on attachment 68696 [details]
Trying to get this to build on GTK

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

Please consider my comments. You may want to do one more round of refinements before landing this.

> WebCore/html/DOMTokenList.cpp:60
> +    if (m_element->document()->inQuirksMode())
> +        m_classNames = new SpaceSplitString(m_element->fastGetAttribute(classAttr), false);

I’m surprised this compiles without an explicit adoptPtr. Soon I will be turning on the “script OwnPtr” flag and then the adoptPtr will definitely be needed. Please use adoptPtr here.

> WebCore/html/DOMTokenList.cpp:135
> +    const unsigned inputLength = input.length();

This is another use of const like the one I asked you to remove earlier.

> WebCore/html/DOMTokenList.cpp:137
> +    Vector<UChar> tokenVector;
> +    append(tokenVector, token);

We could easily write a function that compares a vector to an AtomicString so you would not have to copy the characters into a Vector. That function could go into AtomicString.h or for now could just be here in this source file and moved to AtomicString if we need it later.

> WebCore/html/DOMTokenList.cpp:138
> +    Vector<UChar> output; // 3

We could probably improve performance when doing this on a long string by calling reserveCapacity on output, guessing that its size will be inputLength. I don’t know exactly what it takes to remove excess capacity at String::adopt time but even if we don’t do that the performance tradeoff still might be good.

> WebCore/html/DOMTokenList.cpp:177
> +    if (!validateToken(token,  ec))

Extra space here after the comma.

> WebCore/html/DOMTokenList.cpp:196
> +    if (m_element->document()->inQuirksMode())
> +        m_classNames->set(newClassName, false);

I suggest having this do if (m_classNames) instead of if (inQuirksMode).

> WebCore/html/DOMTokenList.cpp:202
> +    if (m_element->document()->inQuirksMode())
> +        return m_classNames.get();

I suggest having this do if (m_classNames) instead of if (inQuirksMode).

> WebCore/html/DOMTokenList.h:42
> +        return new DOMTokenList(element);

This line of code should call adoptPtr.

> WebCore/html/DOMTokenList.h:61
> +    DOMTokenList(Element* element);

You should omit the argument name “element” here.

> WebCore/html/DOMTokenList.h:70
> +    OwnPtr<SpaceSplitString> m_classNames;

It might be better to name this in a way that makes it more clear it’s used only in a particular case. Even a name such as m_caseSensitiveClassNamesForQuirksMode would not be too crazy, given that this is only used in three places and it would be a mistake to use it anywhere else.

Given that SpaceSplitString already uses a pointer-to-implementation idiom, using an OwnPtr is unnecessary. In fact, it costs one extra memory block for no good reason. Another way to do this is to use have m_classNames be a SpaceSplitString.

If stop using OwnPtr, then my other comments above about switching to a null check of m_classNames would require adding an isNull() function to SpaceSplitString, which I think would be OK. Or you could check quirks mode each time as you are doing now.

If stop using OwnPtr, you won’t have to worry about adoptPtr in the constructor. Instead you would use the SpaceSplitString set function.

[Darin Adler]

Comment on attachment 68696 [details]
Trying to get this to build on GTK

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

>> WebCore/html/DOMTokenList.cpp:60
>> +        m_classNames = new SpaceSplitString(m_element->fastGetAttribute(classAttr), false);
> 
> I’m surprised this compiles without an explicit adoptPtr. Soon I will be turning on the “script OwnPtr” flag and then the adoptPtr will definitely be needed. Please use adoptPtr here.

I mean “strict OwnPtr”!

[Erik Arvidsson]

Created attachment 68732 [details]
Taking care of Darin's comments

[Erik Arvidsson]

Darin, do you mind taking another look? 

Would it be better to submit the operator== for AtomicString and Vector<UChar> in another patch first?

[Erik Arvidsson]

Comment on attachment 68732 [details]
Taking care of Darin's comments

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

> WebCore/html/DOMTokenList.cpp:112
> +    if (!validateToken(token,  ec))

fixed locally

> WebCore/html/DOMTokenList.cpp:134
> +    if (!validateToken(token,  ec))

fixed locally

[Erik Arvidsson]

By moving the operator== to AtomicString it got a lot simpler. See bug 46509.

[Darin Adler]

Comment on attachment 68732 [details]
Taking care of Darin's comments

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

> WebCore/dom/StyledElement.cpp:224
> +        DOMTokenList* classList = optionalClassList();
> +        if (classList)

Putting the definition of the local variable inside the if would make this even clearer:

    if (DOMTokenList* classList = optionalClassList())
        classList->reset(newClassString);

> WebCore/html/DOMTokenList.cpp:37
> +bool operator==(const AtomicString& a, const Vector<UChar>& b)

Should be marked static so we get internal linkage.

> WebCore/html/DOMTokenList.cpp:43
> +    const UChar* chars = a.characters();

Please don't abbreviate this. Just use the word characters.

> WebCore/html/DOMTokenList.h:74
> +bool operator==(const AtomicString& a, const Vector<UChar>& b);
> +inline bool operator==(const Vector<UChar>& a, const AtomicString& b) { return b == a; }

Why is this declared in the header? I don’t think it’s reasonable to have this here as a public function. When I said it could be in DOMTokenList, I meant in the .cpp file. If we want to put it in the header then we need to find a more appropriate file.

[Erik Arvidsson]

Created attachment 68962 [details]
Patch

[Darin Adler]

Comment on attachment 68962 [details]
Patch

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

> WebCore/html/DOMTokenList.h:31
> +#include <wtf/OwnPtr.h>

I don’t see any use of OwnPtr in this header, so there is no need to include OwnPtr.h.

[Erik Arvidsson]

Created attachment 68979 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 68979 [details]
Patch for landing

Clearing flags on attachment: 68979

Committed r68440: <http://trac.webkit.org/changeset/68440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/68440 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/68440
http://trac.webkit.org/changeset/68439

[Csaba Osztrogonác]

(In reply to comment #53)
> http://trac.webkit.org/changeset/68440 might have broken Qt Linux Release
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/68440
> http://trac.webkit.org/changeset/68439

You guys shouldn't ignore this notice and burn always the Qt tree!
Qt specific expected files updated in http://trac.webkit.org/changeset/68488

[Erik Arvidsson]

(In reply to comment #54)
> (In reply to comment #53)
> > http://trac.webkit.org/changeset/68440 might have broken Qt Linux Release
> > The following changes are on the blame list:
> > http://trac.webkit.org/changeset/68440
> > http://trac.webkit.org/changeset/68439
> 
> You guys shouldn't ignore this notice and burn always the Qt tree!
> Qt specific expected files updated in http://trac.webkit.org/changeset/68488

I apologize. I'll make sure I wont neglect it again.

[Csaba Osztrogonác]

(In reply to comment #55)
> I apologize. I'll make sure I wont neglect it again.
Not problem. I realized you broke GTK bots too,
could you fix the GTK specific expected results?
(You should make similar changes, as I did on Qt.)

[Erik Arvidsson]

(In reply to comment #56)
> (In reply to comment #55)
> > I apologize. I'll make sure I wont neglect it again.
> Not problem. I realized you broke GTK bots too,
> could you fix the GTK specific expected results?
> (You should make similar changes, as I did on Qt.)

Will do (tomorrow morning). Thanks.

[Csaba Osztrogonác]

(In reply to comment #57)
> Will do (tomorrow morning). Thanks.

Done by Alex: http://trac.webkit.org/changeset/68501

[Kenichi Ishibashi]

Hi,

I'm interested in implementing HTML5 output element in WebKit. I happily realized that the DOMTokenList interface, which is used in the definition of the DOM interface of the output element, is implemented by this issue. However, unfortunately, the current implementation is tightly coupled to implement the classList property in spite of early patches weren't. So I couldn't employ the current DOMTokenList to implement output element. IMHO, we should decouple the classList property related implementation from the DOMTokenList class, like the implementation proposed in the early patches. I'd like to ask your opinions.

[Darin Adler]

(In reply to comment #59)
> I'm interested in implementing HTML5 output element in WebKit. I happily realized that the DOMTokenList interface, which is used in the definition of the DOM interface of the output element, is implemented by this issue. However, unfortunately, the current implementation is tightly coupled to implement the classList property in spite of early patches weren't. So I couldn't employ the current DOMTokenList to implement output element. IMHO, we should decouple the classList property related implementation from the DOMTokenList class, like the implementation proposed in the early patches. I'd like to ask your opinions.

Yes, when we implement the output element we can refactor the DOMTokenList class so it serves both purposes. This should be straightforward.

[Darin Adler]

(In reply to comment #60)
> Yes, when we implement the output element we can refactor the DOMTokenList class so it serves both purposes. This should be straightforward.

This refactoring will start by making all functions exposed to the DOMTokenList IDL file pure virtual ones in the DOMTokenList base, and then moving the body of the class into a new ClassList class, derived from DOMTokenList.

Then if there is any code that can be shared between the ClassList and whatever DOMTokenList needs to be used by the <output> implementation, we can move that up into the DOMTokenList class a function at a time.

[Erik Arvidsson]

(In reply to comment #59)
> I'm interested in implementing HTML5 output element in WebKit. I happily realized that the DOMTokenList interface, which is used in the definition of the DOM interface of the output element, is implemented by this issue. However, unfortunately, the current implementation is tightly coupled to implement the classList property...

This was done by design since classList is the only user of this today. When we add other users of DOMTokenList or SettableDOMTokenList it was understood that this would need to get refactored.

The only hard part of doing this refactoring is updating the myriad of build files :'(

I added you to bug 29363. Lets keep the discussion there.

[Kenichi Ishibashi]

Hi,

Thank you for your quick responses! Your comments definitely helpful for me. Although I'm going to be off this week, I'll start refactoring DOMTokenList class and implementing the output element next week. I'll keep the discussion on bug 29363.

Thanks,