[Webkit-unassigned] [Bug 47810] [HTML5] Add DOMSettableTokenList

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47810





--- Comment #26 from Kenichi Ishibashi <bashi at google.com>  2010-10-22 20:30:42 PST ---
(From update of attachment 71521)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.