Bug 47810

Created attachment 71026 [details] Patch V0

Hi, The patch doesn't contain tests for DOMSettableTokenList interface because it is not able to generate its instance directory from JS so I couldn't come up with the way to write tests for this interface. I'm planning to add tests for this interface when I implement <output> element, but please let me know if there are another way to write tests for this interface. Thanks,

Attachment 71026 [details] did not build on qt: Build output: http://queues.webkit.org/results/4458067

Attachment 71026 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4459074

Comment on attachment 71026 [details] Patch V0 View in context: https://bugs.webkit.org/attachment.cgi?id=71026&action=review > WebCore/html/DOMSettableTokenList.cpp:87 > + remove(token, ec); This causes another validation. Please refactor to use an internal remove so that we do not validate twice. > WebCore/html/DOMSettableTokenList.cpp:103 > + builder.append(' '); I don't think this is correct. DOMTokenList has a pretty stupid API that requires that whitespace be preserved. For example tokenList = 'a b c' tokenList.remove('c') assertEquals(tokenList.toString(), 'a b');

Comment on attachment 71026 [details] Patch V0 Some build files have no ClassList.cpp and/or ClassList.h. We need a mock implementation for V8 binding to build Chromium successfully.

Created attachment 71149 [details] Patch V1

(In reply to comment #5) Hi Erik, > > WebCore/html/DOMSettableTokenList.cpp:87 > > + remove(token, ec); > > This causes another validation. Please refactor to use an internal remove so that we do not validate twice. Done. > I don't think this is correct. DOMTokenList has a pretty stupid API that requires that whitespace be preserved. > > For example > > tokenList = 'a b c' > tokenList.remove('c') > assertEquals(tokenList.toString(), 'a b'); Thank you for letting me know that. I didn't realize such behavior. I've changed the code to save the original value to follow the requirement.

(In reply to comment #6) Hi Kent-san, > (From update of attachment 71026 [details]) > Some build files have no ClassList.cpp and/or ClassList.h. I've added newly-added files into build files as far as I know, but it might be incomplete, so please let me know there are oversights > We need a mock implementation for V8 binding to build Chromium successfully. I've added a mock implementation. Thank you for your comments!

Comment on attachment 71149 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=71149&action=review I'm not sure we need to add add and remove to SpaceSplitString. I decided not to do it for classList since mutating the list always had to update the class attribute which triggers a lot of things (such as mutation events and style resolution). If you look at DOMTokenList and SpaceSplitString in isolation it does seem to make sense to not rebuild the SpaceSplitString for add and remove. > WebCore/html/DOMSettableTokenList.cpp:70 > + builder.append(m_value); This is not right. Please check the spec. The point is that you should not add a space if the last character is already a space. All of this is implemented in the old DOMTokenList and covered by the layout tests. You should be able to reuse almost all of the tests for classList since DOMTokenList is a subset of SettableDOMTokenList. > WebCore/html/DOMSettableTokenList.cpp:74 > + m_tokens.set(builder.toString(), true); Can we add an add to SpaceSplitString? > WebCore/html/DOMSettableTokenList.cpp:90 > + m_tokens.clear(); I think you could call setValue here and in addInternal to remove some duplicate code. I'm not sure if it is really worth it though? > WebCore/html/DOMSettableTokenList.cpp:92 > + m_tokens.set(m_value, true); Can we add a remove to SpaceSplitString?

Comment on attachment 71149 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=71149&action=review Hi Erik, Thank you for your helpful comments! I'll post revised patch later. >> WebCore/html/DOMSettableTokenList.cpp:70 >> + builder.append(m_value); > > This is not right. Please check the spec. > > The point is that you should not add a space if the last character is already a space. > > All of this is implemented in the old DOMTokenList and covered by the layout tests. You should be able to reuse almost all of the tests for classList since DOMTokenList is a subset of SettableDOMTokenList. Thank you for correction. I should be more careful.. I've modified the patch to reuse existing code. >> WebCore/html/DOMSettableTokenList.cpp:74 >> + m_tokens.set(builder.toString(), true); > > Can we add an add to SpaceSplitString? Added. >> WebCore/html/DOMSettableTokenList.cpp:90 >> + m_tokens.clear(); > > I think you could call setValue here and in addInternal to remove some duplicate code. I'm not sure if it is really worth it though? I reused existing code so the code no longer exists. >> WebCore/html/DOMSettableTokenList.cpp:92 >> + m_tokens.set(m_value, true); > > Can we add a remove to SpaceSplitString? Added.

Created attachment 71255 [details] Patch V2

Attachment 71255 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/SpaceSplitString.h:79: More than one command on the same line in if [whitespace/parens] [4] WebCore/dom/SpaceSplitString.h:80: More than one command on the same line in if [whitespace/parens] [4] Total errors found: 2 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 71255 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=71255&action=review I think WebCore/Android.mk and WebCore/CMakeList.txt need to have ClassLis.cpp. > WebCore/ChangeLog:12 > + * Android.derived.jscbindings.mk: You should add a comment to each of files/functions in ChangeLog as possible. In this case, you can add comments like - "Add <new files>" for build files - "Moved from Foobar.cpp" for copied functions Such comments would be very helpful for reviewing. > WebCore/bindings/v8/custom/V8DOMSettableTokenListCustom.cpp:37 > + // TODO(bashi): Implement this function. We don't use TODO(username): style in WebKit. Just put FIXME: please. > WebCore/dom/SpaceSplitString.h:79 > + void add(const AtomicString& string) { if (m_data) m_data->add(string); } Please move the implementation to SpaceSplitString.cpp. > WebCore/dom/SpaceSplitString.h:80 > + void remove(const AtomicString& string) { if (m_data) m_data->remove(string); } ditto.

Comment on attachment 71255 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=71255&action=review Thanks for adding add and remove but... > WebCore/dom/SpaceSplitString.cpp:108 > + m_createdVector = false; This kind of defeats my point. If we add add and remove to SpaceSplitString/SpaceSplitStringData we should operate on the vector which allows us to do faster add and removes. I really don't want SpaceSplitString to carry the burden of the white space preserving semantics of DOMTokenList and I don't think it makes sense to tear down and rebuild the vector for adds and removes. > WebCore/dom/SpaceSplitString.cpp:111 > +void SpaceSplitStringData::remove(const AtomicString& string) Lets keep this silly algorithm in DOMTokenList ;-)

Kent-san, Thank you very much for reviewing! (In reply to comment #14) > I think WebCore/Android.mk and WebCore/CMakeList.txt need to have ClassLis.cpp. I'll add ClassList.cpp into these files in next patch. BTW, as for WebCore/Android.mk, it doesn't contain DOMTokenList.cpp so it was difficult to determine whether I have to put new files into WebCore/Android.mk (and other build files). Should I also include DOMTokenList.cpp in this file? Is there any guideline for that? > You should add a comment to each of files/functions in ChangeLog as possible. > In this case, you can add comments like > - "Add <new files>" for build files > - "Moved from Foobar.cpp" for copied functions > Such comments would be very helpful for reviewing. Sorry for inconvenience you. I'll add a comment from next time. > We don't use TODO(username): style in WebKit. Just put FIXME: please. Done. > > WebCore/dom/SpaceSplitString.h:79 > > + void add(const AtomicString& string) { if (m_data) m_data->add(string); } > > Please move the implementation to SpaceSplitString.cpp. Done. > > WebCore/dom/SpaceSplitString.h:80 > > + void remove(const AtomicString& string) { if (m_data) m_data->remove(string); } > > ditto. Done.

Hi Erik, (In reply to comment #15) > If we add add and remove to SpaceSplitString/SpaceSplitStringData we should operate on the vector which allows us to do faster add and removes. I really don't want SpaceSplitString to carry the burden of the white space preserving semantics of DOMTokenList and I don't think it makes sense to tear down and rebuild the vector for adds and removes. Oh, I misunderstood what you were getting at. I've modified add() and remove() following your suggestion. > > WebCore/dom/SpaceSplitString.cpp:111 > > +void SpaceSplitStringData::remove(const AtomicString& string) > > Lets keep this silly algorithm in DOMTokenList ;-) OK, I've put it in DOMTokenList again:-)

Created attachment 71395 [details] Patch V3

Comment on attachment 71395 [details] Patch V3 View in context: https://bugs.webkit.org/attachment.cgi?id=71395&action=review > WebCore/dom/SpaceSplitString.cpp:110 > + break; The vector can contain duplicates so we cannot break here. > WebCore/html/ClassList.cpp:95 > + m_classNamesForQuirksMode.add(token); This needs more work. m_element.setAttribute(classAttr...) cause the SpaceSplitString to be reset already and by calling add we recreate the vector immediately, even if we will never need it. One of the power of the SpaceSplitString is that it does not create the vector until needed the first time. This is important because it is used a lot in style resolutions for matching class names. In a follow up patch I would like classList add/remove to keep the SpaceSplitString but that requires changes to how we reset the SpaceSplitString which I think is out of scope for this patch. > WebCore/html/ClassList.cpp:113 > + m_classNamesForQuirksMode.remove(token); same here

Comment on attachment 71395 [details] Patch V3 View in context: https://bugs.webkit.org/attachment.cgi?id=71395&action=review >> WebCore/dom/SpaceSplitString.cpp:110 >> + break; > > The vector can contain duplicates so we cannot break here. Fixed. >> WebCore/html/ClassList.cpp:95 >> + m_classNamesForQuirksMode.add(token); > > This needs more work. > > m_element.setAttribute(classAttr...) cause the SpaceSplitString to be reset already and by calling add we recreate the vector immediately, even if we will never need it. One of the power of the SpaceSplitString is that it does not create the vector until needed the first time. This is important because it is used a lot in style resolutions for matching class names. > > In a follow up patch I would like classList add/remove to keep the SpaceSplitString but that requires changes to how we reset the SpaceSplitString which I think is out of scope for this patch. I see. Thanks for the advice. I've introduced a flag which shows whether the SpaceSplitString is up-to-date, but it might be somewhat ugly. I would appreciate if you have another idea to address it.

Created attachment 71521 [details] Patch V4

(In reply to comment #16) > I'll add ClassList.cpp into these files in next patch. BTW, as for WebCore/Android.mk, it doesn't contain DOMTokenList.cpp so it was difficult to determine whether I have to put new files into WebCore/Android.mk (and other build files). Should I also include DOMTokenList.cpp in this file? Is there any guideline for that? Probably Android build is just broken at this moment :-) I think someone needs to add DOMTokenList.cpp to Android.mk. The latest patch looks fine to me. I'll set r+ if Erik makes no more suggestions.

(In reply to comment #22) > Probably Android build is just broken at this moment :-) I think someone needs to add DOMTokenList.cpp to Android.mk. Android build files have been removed by r70290. Ishibashi-san needs to update the patch anyway.

Kent-san, (In reply to comment #23) > Android build files have been removed by r70290. Ishibashi-san needs to update the patch anyway. Thank you for the information. It's good to hear since it'll reduce a troublesome task:-) I'd like to wait Erik's comment and will revise the patch later. Regards,

Comment on attachment 71521 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=71521&action=review The more I think about this the more I think we could probably use SettableDOMTokenList for classList as well since we do allow setValue from C++. I don't really think we need DOMTokenList at all in C++. For the ClassList class we could just override how we get the tokens and how we update the DOM. > WebCore/html/ClassList.cpp:63 > + if (!m_classNamesNeedsUpdate) I think I may have misguided you. SpaceSplitString is already smart enough to actually not build the vector until needed. When calling set() it will only clear the current vector and keep the string for the future. The problem was that you were calling add and remove (which rebuild the vector) after we already called setAttribute (which in turn call set() on the SpaceSplitString) so we lost the optimization. I think this whole method can be removed since classList is always up to date due to reset being called when someone sets the class attribute. > WebCore/html/ClassList.cpp:74 > + // We need this ulgy const_cast to conform with the className attribute typo > WebCore/html/ClassList.cpp:89 > + // We need this ulgy const_cast to conform with the className attribute typo > WebCore/html/ClassList.cpp:118 > + m_classNamesNeedsUpdate = true; This line is not needed since reset gets called due to setAttribute > WebCore/html/ClassList.cpp:138 > + m_classNamesNeedsUpdate = true; same here ... and now there are no more places where it gets set to true so that means that updateClassNamesForQuirksModeIfNeeded is a noop > WebCore/html/ClassList.h:57 > + void reset(const String&); why don't we rename this to setValue for consistency?

Comment on attachment 71521 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=71521&action=review Hi Erik, Thank you for your comments. I'll follow your comment on removing the flag and unnecessary functions. But, as for integrating ClassList and DOMSettableTokenList, I am wondering that we should do it. There are several difference between them. ClassList has the reference of the element but DOMSettableTokenList needs not to have it. In addition, the way to handle reference counting is also different. So IMHO, it would have less benefits to integrate these classes. I'll post revised patch after receiving responses. >> WebCore/html/ClassList.cpp:63 >> + if (!m_classNamesNeedsUpdate) > > I think I may have misguided you. > > SpaceSplitString is already smart enough to actually not build the vector until needed. When calling set() it will only clear the current vector and keep the string for the future. > > The problem was that you were calling add and remove (which rebuild the vector) after we already called setAttribute (which in turn call set() on the SpaceSplitString) so we lost the optimization. > > I think this whole method can be removed since classList is always up to date due to reset being called when someone sets the class attribute. Thank you for collecting my mistake. I thought at first SpaceSplitString is not smart enough, but actually it is, as you explained above. So I've just removed these functions and the flag. >> WebCore/html/ClassList.cpp:74 >> + // We need this ulgy const_cast to conform with the className attribute > > typo The comment is no longer exist. >> WebCore/html/ClassList.cpp:89 >> + // We need this ulgy const_cast to conform with the className attribute > > typo Ditto. >> WebCore/html/ClassList.cpp:138 >> + m_classNamesNeedsUpdate = true; > > same here > > ... and now there are no more places where it gets set to true so that means that updateClassNamesForQuirksModeIfNeeded is a noop Thank you for detailed explanation. I understand it. >> WebCore/html/ClassList.h:57 >> + void reset(const String&); > > why don't we rename this to setValue for consistency? Done.

Comment on attachment 71521 [details] Patch V4 r- because of Erik's comments.

(In reply to comment #26) > Thank you for your comments. I'll follow your comment on removing the flag and unnecessary functions. But, as for integrating ClassList and DOMSettableTokenList, I am wondering that we should do it. There are several difference between them. ClassList has the reference of the element but DOMSettableTokenList needs not to have it. In addition, the way to handle reference counting is also different. So IMHO, it would have less benefits to integrate these classes. I was thinking that ClassList would extend SettableDOMTokenList and override a few methods so that it can hook the changes to the class attribute. Either way, I'm fine either way.

(In reply to comment #28) > I was thinking that ClassList would extend SettableDOMTokenList and override a few methods so that it can hook the changes to the class attribute. Either way, I'm fine either way. Thank you for your suggestion. However, I'd like to stay them to be divided following reasons: (1) DOMSettableTokenList has a string member variable, which is not necessary for ClassList so it would waste memory. (2) The reference counting strategy is different between ClassList and DOMSettableTokenList, and RefCounted<T>, which is used by DOMSettableTokenList, defines it's ref() and deref() as non-virtual functions so we might need to modify RefCounted<T> class. I'll post revised patch soon. Regards,

Created attachment 71833 [details] Patch V5

(In reply to comment #30) > Created an attachment (id=71833) [details] > Patch V5 I'll set r+ tomorrow if no one objects.

Comment on attachment 71833 [details] Patch V5 I confirmed that this patch conflicted with the latest WebKit tree. Would you rebase the patch please?

Created attachment 72300 [details] Patch V6

Kent-san, I've rebased. Thanks! (In reply to comment #32) > (From update of attachment 71833 [details]) > I confirmed that this patch conflicted with the latest WebKit tree. > Would you rebase the patch please?

Comment on attachment 72300 [details] Patch V6 ok

Comment on attachment 72300 [details] Patch V6 Clearing flags on attachment: 72300 Committed r70854: <http://trac.webkit.org/changeset/70854>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/70854 might have broken Qt Linux Release The following tests are not passing: fast/dom/Window/window-properties.html fast/dom/Window/window-property-descriptors.html fast/dom/prototype-inheritance.html fast/js/global-constructors.html

(In reply to comment #38) > http://trac.webkit.org/changeset/70854 might have broken Qt Linux Release > The following tests are not passing: > fast/dom/Window/window-properties.html > fast/dom/Window/window-property-descriptors.html > fast/dom/prototype-inheritance.html > fast/js/global-constructors.html Fixed.