Bug 120344 – Clean ClassList and DOMSettableTokenList

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Clean ClassList and DOMSettableTokenList

[Benjamin Poulain]

Created attachment 209723 [details]
Patch

[Build Bot]

Comment on attachment 209723 [details]
Patch

Attachment 209723 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1587206

[kov's GTK+ EWS bot]

Comment on attachment 209723 [details]
Patch

Attachment 209723 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1582232

[Benjamin Poulain]

Created attachment 209727 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 209727 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        -Move the implementation of virtual functions to the cpp file.
> +        -Clean the #includes.
> +        -Make the implemented pure virtual methods final.
> +        -Make the element() accessor const.

Maybe put a space after each "-"?

> Source/WebCore/html/ClassList.cpp:85
>      if (m_element->document()->inQuirksMode()) {

I don't get this. It's not like m_element's document transition between quirks mode and strict mode...
We should be able to determine this at the creation time (probably not in this patch though).

> Source/WebCore/html/DOMSettableTokenList.h:55
>      AtomicString m_value;

We should get rid of this member variable.

[Ryosuke Niwa]

Comment on attachment 209727 [details]
Patch

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

>> Source/WebCore/html/ClassList.cpp:85
>>      if (m_element->document()->inQuirksMode()) {
> 
> I don't get this. It's not like m_element's document transition between quirks mode and strict mode...
> We should be able to determine this at the creation time (probably not in this patch though).

Hm... I suppose you can move the element from a strict mode document to a quirks mode document.

[Benjamin Poulain]

Committed r154707: <http://trac.webkit.org/changeset/154707>

[Darin Adler]

Comment on attachment 209727 [details]
Patch

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

> Source/WebCore/html/ClassList.h:40
> +    virtual void ref() OVERRIDE FINAL;

Is the class ClassList not something we can mark FINAL? If we did we could save marking all those functions final.

> Source/WebCore/html/DOMSettableTokenList.h:38
>  class DOMSettableTokenList : public DOMTokenList, public RefCounted<DOMSettableTokenList> {

Can this class be marked FINAL?

[Benjamin Poulain]

(In reply to comment #8)
> (From update of attachment 209727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209727&action=review
> 
> > Source/WebCore/html/ClassList.h:40
> > +    virtual void ref() OVERRIDE FINAL;
> 
> Is the class ClassList not something we can mark FINAL? If we did we could save marking all those functions final.
> 
> > Source/WebCore/html/DOMSettableTokenList.h:38
> >  class DOMSettableTokenList : public DOMTokenList, public RefCounted<DOMSettableTokenList> {
> 
> Can this class be marked FINAL?

That's what I tried with the previous patch that did not build.
The bindings subclasses those.

I have more refactoring in progress. The end result should be nicer.