| Summary: | Clean ClassList and DOMSettableTokenList | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||
| Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, esprehn+autocc, gtk-ews, philn, rniwa, xan.lopez | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Benjamin Poulain
2013-08-27 01:29:04 PDT
Created attachment 209723 [details]
Patch
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 Comment on attachment 209723 [details] Patch Attachment 209723 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1582232 Created attachment 209727 [details]
Patch
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. 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. Committed r154707: <http://trac.webkit.org/changeset/154707> 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? (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. |