[webkit-reviews] review granted: [Bug 20709] Implement HTML 5's HTMLElement.classList property : [Attachment 68732] Taking care of Darin's comments

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 20709: Implement HTML 5's HTMLElement.classList property
https://bugs.webkit.org/show_bug.cgi?id=20709

Attachment 68732: Taking care of Darin's comments
https://bugs.webkit.org/attachment.cgi?id=68732&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=68732&action=review

> WebCore/dom/StyledElement.cpp:224
> +	   DOMTokenList* classList = optionalClassList();
> +	   if (classList)

Putting the definition of the local variable inside the if would make this even
clearer:

    if (DOMTokenList* classList = optionalClassList())
	classList->reset(newClassString);

> WebCore/html/DOMTokenList.cpp:37
> +bool operator==(const AtomicString& a, const Vector<UChar>& b)

Should be marked static so we get internal linkage.

> WebCore/html/DOMTokenList.cpp:43
> +    const UChar* chars = a.characters();

Please don't abbreviate this. Just use the word characters.

> WebCore/html/DOMTokenList.h:74
> +bool operator==(const AtomicString& a, const Vector<UChar>& b);
> +inline bool operator==(const Vector<UChar>& a, const AtomicString& b) {
return b == a; }

Why is this declared in the header? I don’t think it’s reasonable to have this
here as a public function. When I said it could be in DOMTokenList, I meant in
the .cpp file. If we want to put it in the header then we need to find a more
appropriate file.