[Webkit-unassigned] [Bug 14955] Implement getElementsByClassName

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=14955


eric at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #16240|review?                     |review-
               Flag|                            |




------- Comment #15 from eric at webkit.org  2007-10-01 12:17 PDT -------
(From update of attachment 16240)
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!


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.