[webkit-reviews] review denied: [Bug 47810] [HTML5] Add DOMSettableTokenList : [Attachment 71255] Patch V2

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> 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 71255: Patch V2
https://bugs.webkit.org/attachment.cgi?id=71255&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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.