Bug 47810 - [HTML5] Add DOMSettableTokenList
Summary: [HTML5] Add DOMSettableTokenList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 29363
  Show dependency treegraph
 
Reported: 2010-10-18 04:01 PDT by Kenichi Ishibashi
Modified: 2010-10-29 03:51 PDT (History)
8 users (show)

See Also:


Attachments
Patch V0 (55.97 KB, patch)
2010-10-18 05:41 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V1 (62.22 KB, patch)
2010-10-19 03:19 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V2 (66.34 KB, patch)
2010-10-19 23:32 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V3 (67.55 KB, patch)
2010-10-20 23:58 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V4 (73.49 KB, patch)
2010-10-21 20:42 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V5 (72.65 KB, patch)
2010-10-25 19:30 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V6 (76.42 KB, patch)
2010-10-29 01:56 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2010-10-18 04:01:05 PDT
Adding DOMSettableTokenList interface to implement <output> tag.

http://www.w3.org/TR/html5/common-dom-interfaces.html#domsettabletokenlist-0

This interface is required to implement HTML5 output element. See https://bugs.webkit.org/show_bug.cgi?id=29363 for more details.
Comment 1 Kenichi Ishibashi 2010-10-18 05:41:23 PDT
Created attachment 71026 [details]
Patch V0
Comment 2 Kenichi Ishibashi 2010-10-18 05:42:29 PDT
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,
Comment 3 Early Warning System Bot 2010-10-18 06:15:04 PDT
Attachment 71026 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4458067
Comment 4 WebKit Review Bot 2010-10-18 07:53:56 PDT
Attachment 71026 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4459074
Comment 5 Erik Arvidsson 2010-10-18 16:20:57 PDT
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 6 Kent Tamura 2010-10-18 18:25:52 PDT
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.
Comment 7 Kenichi Ishibashi 2010-10-19 03:19:17 PDT
Created attachment 71149 [details]
Patch V1
Comment 8 Kenichi Ishibashi 2010-10-19 03:21:05 PDT
(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.
Comment 9 Kenichi Ishibashi 2010-10-19 03:21:44 PDT
(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 10 Erik Arvidsson 2010-10-19 09:51:47 PDT
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 11 Kenichi Ishibashi 2010-10-19 18:16:43 PDT
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.
Comment 12 Kenichi Ishibashi 2010-10-19 23:32:24 PDT
Created attachment 71255 [details]
Patch V2
Comment 13 WebKit Review Bot 2010-10-19 23:35:31 PDT
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 14 Kent Tamura 2010-10-20 08:17:10 PDT
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 15 Erik Arvidsson 2010-10-20 12:06:42 PDT
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 ;-)
Comment 16 Kenichi Ishibashi 2010-10-20 18:21:51 PDT
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.
Comment 17 Kenichi Ishibashi 2010-10-20 20:02:45 PDT
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:-)
Comment 18 Kenichi Ishibashi 2010-10-20 23:58:12 PDT
Created attachment 71395 [details]
Patch V3
Comment 19 Erik Arvidsson 2010-10-21 10:38:23 PDT
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 20 Kenichi Ishibashi 2010-10-21 19:34:00 PDT
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.
Comment 21 Kenichi Ishibashi 2010-10-21 20:42:44 PDT
Created attachment 71521 [details]
Patch V4
Comment 22 Kent Tamura 2010-10-21 22:26:12 PDT
(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.
Comment 23 Kent Tamura 2010-10-21 22:38:31 PDT
(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.
Comment 24 Kenichi Ishibashi 2010-10-21 23:08:55 PDT
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 25 Erik Arvidsson 2010-10-22 10:24:26 PDT
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 26 Kenichi Ishibashi 2010-10-22 20:30:42 PDT
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 27 Kent Tamura 2010-10-24 23:44:13 PDT
Comment on attachment 71521 [details]
Patch V4

r- because of Erik's comments.
Comment 28 Erik Arvidsson 2010-10-25 09:50:55 PDT
(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.
Comment 29 Kenichi Ishibashi 2010-10-25 17:49:09 PDT
(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,
Comment 30 Kenichi Ishibashi 2010-10-25 19:30:26 PDT
Created attachment 71833 [details]
Patch V5
Comment 31 Kent Tamura 2010-10-27 22:17:31 PDT
(In reply to comment #30)
> Created an attachment (id=71833) [details]
> Patch V5

I'll set r+ tomorrow if no one objects.
Comment 32 Kent Tamura 2010-10-28 22:29:51 PDT
Comment on attachment 71833 [details]
Patch V5

I confirmed that this patch conflicted with the latest WebKit tree.
Would you rebase the patch please?
Comment 33 Kenichi Ishibashi 2010-10-29 01:56:34 PDT
Created attachment 72300 [details]
Patch V6
Comment 34 Kenichi Ishibashi 2010-10-29 01:57:45 PDT
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 35 Kent Tamura 2010-10-29 01:58:11 PDT
Comment on attachment 72300 [details]
Patch V6

ok
Comment 36 WebKit Commit Bot 2010-10-29 02:32:26 PDT
Comment on attachment 72300 [details]
Patch V6

Clearing flags on attachment: 72300

Committed r70854: <http://trac.webkit.org/changeset/70854>
Comment 37 WebKit Commit Bot 2010-10-29 02:32:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 38 WebKit Review Bot 2010-10-29 03:01:11 PDT
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
Comment 39 Kent Tamura 2010-10-29 03:51:21 PDT
(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.