[webkit-reviews] review denied: [Bug 14955] Implement getElementsByClassName : [Attachment 16497] More review fixage

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied David Smith <catfish.man at gmail.com>'s
request for review:
Bug 14955: Implement getElementsByClassName
http://bugs.webkit.org/show_bug.cgi?id=14955

Attachment 16497: More review fixage
http://bugs.webkit.org/attachment.cgi?id=16497&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
I am surprised.  I had figured class names were always case insensitive.  It
does not appear to be the case with your patch.  Does contains() need to
lower() the incoming string when in compatMode?

This is not needed, please remove:
+    m_classList = ClassNameList();

Constructor initializers are generally one on each line, with the comma (or
colon) leading the line (see the style guidelines). This allows easy use of #if
ENABLE(FOO) around certain initializers as well as easy removal/addition with
the smallest possible diff.

Maybe it's the need to return "false" when m_impl == 0, that has caused
isLower() not to be implemented before.  One could argue that isLower should
return true for "" or null.  Not sure it matters.  I'm OK having a  funny
isLower as such.  It's also OK to call string->impl()->toLower() (and NOT wrap
it in String(), the compiler will do that for you.)  I know I encouraged you to
add isLower. Doesn't matter either way, your call.

Need to fix the one small style issue, the extra initialization and answer
regarding case insensitive lookups.  I also think we need a compat-mode test
and some case-sensitivity and white-space sensitivity tests before this can
land.

Thanks again for taking the time to work on this.