[webkit-reviews] review denied: [Bug 14955] Implement getElementsByClassName : [Attachment 16240] Actually include the header

[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 16240: Actually include the header
http://bugs.webkit.org/attachment.cgi?id=16240&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
Ok, a few comments:
+    for(const ClassNameList *t = this; t; t = t->next()) 
+    { 
+	 if(t->string() == str) 
+	     return true; 

Those don't agree with the style guidelines.  { should be on the same line as
for, and if( should be if (
http://webkit.org/coding/coding-style.html

No need for this-> here:
+void ClassNameList::parseClassAttribute(const String& classStr, const bool
inCompatMode)
+{
+    this->clear();

+    String classAttr = inCompatMode ? 
+	 (classStr.impl()->isLower() ? classStr :
String(classStr.impl()->lower())) :
+	 classStr;
doesnt' need to call all the impl() stuff, just call .lower() on string.

To me "start" and "end" are more readable than "ePos" and "sPos", but that's
not a huge issue.

Style guidelines again:
+ClassNodeList::ClassNodeList(PassRefPtr<Node> rootNode, const AtomicString&
className)
+: NodeList(rootNode), m_namespaceURI(nullAtom)


Why?
+    m_classList = *(new ClassNameList());
That shouldn't be needed.

Style again:
+    const ClassNameList* t = static_cast<Element*>(testNode)->getClassList();
+    
+    for (const ClassNameList* c = &m_classList; c; c = c->next())
+    {
+	 if(t->contains(c->string()))
+	    continue;

also, "t" is not a very readable/useful variable name

This is also more easily written as:
+    for (const ClassNameList* c = &m_classList; c; c = c->next())
+    {
+	 if(t->contains(c->string()))
+	    continue;
+
+	 return false;

if (!contains)
    return false;

IMO.

Shouldn't this be "isEmpty()" ?

+PassRefPtr<NodeList> Element::getElementsByClassName(const String& className)
+{
+    if (className.isNull())
+	 return 0;


Otherwise looks OK.  I think the style stuff should be corrected and a new
patch posted before landing though.

Thanks for the patch!