[webkit-reviews] review denied: [Bug 47810] [HTML5] Add DOMSettableTokenList : [Attachment 71149] Patch V1

[bugzilla-daemon at webkit.org]

Kenichi Ishibashi <bashi at google.com> has denied Kenichi Ishibashi
<bashi at google.com>'s request for review:
Bug 47810: [HTML5] Add DOMSettableTokenList
https://bugs.webkit.org/show_bug.cgi?id=47810

Attachment 71149: Patch V1
https://bugs.webkit.org/attachment.cgi?id=71149&action=review

------- Additional Comments from Kenichi Ishibashi <bashi at google.com>
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.