This will help speeding up a lot of code for developers. http://www.w3.org/TR/html5/semantics.html#classlist
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.
*** Bug 26358 has been marked as a duplicate of this bug. ***
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.
(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>.
(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>.
Erik found some existing tests for classList: <http://simon.html5.org/test/html/dom/reflecting/DOMTokenList/>.
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
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
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.
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.
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?
Created attachment 64365 [details] Patch
Attachment 64365 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3768133
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-.
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.
Created attachment 66784 [details] Patch
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.
(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.)
I looked at NamedNodeMap and now I understand what Darin meant. I'll rename ClassList to DOMTokenList.
Attachment 66784 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3938308
Created attachment 66817 [details] Patch
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).
Attachment 66817 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3976276
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.
Created attachment 66931 [details] Patch
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?
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.
(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
Attachment 66931 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3914329
Created attachment 67985 [details] Now with markDOMObjectWrapper and V8DOMWrapper::setHiddenWindowReference
Attachment 67985 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4035060
Created attachment 68606 [details] Patch
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?
Attachment 68606 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4021142
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.
Created attachment 68637 [details] Took care of Darin's comments and added a perf test for remove
Attachment 68637 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4079094
Created attachment 68696 [details] Trying to get this to build on GTK
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.
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.
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.
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”!
Created attachment 68732 [details] Taking care of Darin's comments
Darin, do you mind taking another look? Would it be better to submit the operator== for AtomicString and Vector<UChar> in another patch first?
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
By moving the operator== to AtomicString it got a lot simpler. See bug 46509.
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.
Created attachment 68962 [details] Patch
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.
Created attachment 68979 [details] Patch for landing
Comment on attachment 68979 [details] Patch for landing Clearing flags on attachment: 68979 Committed r68440: <http://trac.webkit.org/changeset/68440>
All reviewed patches have been landed. Closing bug.
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
(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
(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.
(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.)
(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.
(In reply to comment #57) > Will do (tomorrow morning). Thanks. Done by Alex: http://trac.webkit.org/changeset/68501
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.
(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.
(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.
(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.
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,