Use Traversal<HTMLElement> API in HTMLTableRowsCollection

Source/core/html/HTMLTableRowsCollection.cpp

Index: Source/core/html/HTMLTableRowsCollection.cpp
diff --git a/Source/core/html/HTMLTableRowsCollection.cpp b/Source/core/html/HTMLTableRowsCollection.cpp
index 97214a62fdb1667ff502b09c466213331caa202a..b12d821c800570a45d02b16515d366e77ec9c709 100644
--- a/Source/core/html/HTMLTableRowsCollection.cpp
+++ b/Source/core/html/HTMLTableRowsCollection.cpp
@@ -53,15 +53,8 @@ static bool isInFoot(Element* row)
     return row->parentNode() && toElement(row->parentNode())->hasLocalName(tfootTag);
 }
 
-static inline HTMLTableRowElement* findTableRowElementInChildren(Element& current)
-{
-    return Traversal<HTMLTableRowElement>::firstChild(current);
-}
-
 HTMLTableRowElement* HTMLTableRowsCollection::rowAfter(HTMLTableElement& table, HTMLTableRowElement* previous)
 {
-    Element* child = 0;
-
     // Start by looking for the next row in this section.
     // Continue only if there is none.
     if (previous && previous->parentNode() != table) {
@@ -70,41 +63,42 @@ HTMLTableRowElement* HTMLTableRowsCollection::rowAfter(HTMLTableElement& table,
     }
 
     // If still looking at head sections, find the first row in the next head section.
+    HTMLElement* child = 0;
     if (!previous)
-        child = ElementTraversal::firstWithin(table);
+        child = Traversal<HTMLElement>::firstChild(table);
     else if (isInHead(previous))
-        child = ElementTraversal::nextSibling(*previous->parentNode());
-    for (; child; child = ElementTraversal::nextSibling(*child)) {
-        if (child->hasTagName(theadTag)) {
-            if (HTMLTableRowElement* row = findTableRowElementInChildren(*child))
+        child = Traversal<HTMLElement>::nextSibling(*previous->parentNode());
+    for (; child; child = Traversal<HTMLElement>::nextSibling(*child)) {
+        if (child->hasLocalName(theadTag)) {
+            if (HTMLTableRowElement* row = Traversal<HTMLTableRowElement>::firstChild(*child))
                 return row;
         }
     }
 
     // If still looking at top level and bodies, find the next row in top level or the first in the next body section.
     if (!previous || isInHead(previous))
-        child = ElementTraversal::firstWithin(table);
+        child = Traversal<HTMLElement>::firstChild(table);
     else if (previous->parentNode() == table)
-        child = ElementTraversal::nextSibling(*previous);
+        child = Traversal<HTMLElement>::nextSibling(*previous);
     else if (isInBody(previous))
-        child = ElementTraversal::nextSibling(*previous->parentNode());
-    for (; child; child = ElementTraversal::nextSibling(*child)) {
+        child = Traversal<HTMLElement>::nextSibling(*previous->parentNode());
+    for (; child; child = Traversal<HTMLElement>::nextSibling(*child)) {

[esprehn, 2014/03/27 13:35:49]

This isn't equivalent to the original code. If you had:


  <non-html>
     <span></span>
     <tr id="1">
     </tr>
  </non-html>
  <tr id="2"></td>

You return 1, it used to return 2. This might be okay, but you are changing
behavior.

[Inactive, 2014/03/27 13:57:58]

Arg, you are right, I caused a slight behavior change. This was unintentional, I
will address this in a follow-up today. Thanks for catching this.

[Inactive, 2014/03/27 14:39:54]

Actually, the new code returns 2 as well since I am calling
Traversal<HTMLElement>::firstChild(table) and not
Traversal<HTMLElement>::firstWithin(table). The new code will *not* traverse
descendants, only children.

Let me know if this is me misunderstanding your example.

         if (isHTMLTableRowElement(child))
             return toHTMLTableRowElement(child);
-        if (child->hasTagName(tbodyTag)) {
-            if (HTMLTableRowElement* row = findTableRowElementInChildren(*child))
+        if (child->hasLocalName(tbodyTag)) {
+            if (HTMLTableRowElement* row = Traversal<HTMLTableRowElement>::firstChild(*child))
                 return row;
         }
     }
 
     // Find the first row in the next foot section.
     if (!previous || !isInFoot(previous))
-        child = ElementTraversal::firstWithin(table);
+        child = Traversal<HTMLElement>::firstChild(table);
     else
-        child = ElementTraversal::nextSibling(*previous->parentNode());
-    for (; child; child = ElementTraversal::nextSibling(*child)) {
-        if (child->hasTagName(tfootTag)) {
-            if (HTMLTableRowElement* row = findTableRowElementInChildren(*child))
+        child = Traversal<HTMLElement>::nextSibling(*previous->parentNode());
+    for (; child; child = Traversal<HTMLElement>::nextSibling(*child)) {
+        if (child->hasLocalName(tfootTag)) {
+            if (HTMLTableRowElement* row = Traversal<HTMLTableRowElement>::firstChild(*child))
                 return row;
         }
     }
@@ -114,24 +108,24 @@ HTMLTableRowElement* HTMLTableRowsCollection::rowAfter(HTMLTableElement& table,
 
 HTMLTableRowElement* HTMLTableRowsCollection::lastRow(HTMLTableElement& table)
 {
-    for (Node* child = table.lastChild(); child; child = child->previousSibling()) {
-        if (child->hasTagName(tfootTag)) {
+    for (HTMLElement* child = Traversal<HTMLElement>::lastChild(table); child; child = Traversal<HTMLElement>::previousSibling(*child)) {
+        if (child->hasLocalName(tfootTag)) {
             if (HTMLTableRowElement* lastRow = Traversal<HTMLTableRowElement>::lastChild(*child))
                 return lastRow;
         }
     }
 
-    for (Node* child = table.lastChild(); child; child = child->previousSibling()) {
+    for (HTMLElement* child = Traversal<HTMLElement>::lastChild(table); child; child = Traversal<HTMLElement>::previousSibling(*child)) {
         if (isHTMLTableRowElement(child))
             return toHTMLTableRowElement(child);
-        if (child->hasTagName(tbodyTag)) {
+        if (child->hasLocalName(tbodyTag)) {
             if (HTMLTableRowElement* lastRow = Traversal<HTMLTableRowElement>::lastChild(*child))
                 return lastRow;
         }
     }
 
-    for (Node* child = table.lastChild(); child; child = child->previousSibling()) {
-        if (child->hasTagName(theadTag)) {
+    for (HTMLElement* child = Traversal<HTMLElement>::lastChild(table); child; child = Traversal<HTMLElement>::previousSibling(*child)) {

[esprehn, 2014/03/27 13:35:49]

Why not use Traversal<HTMLTableSectionElement> ?

[Inactive, 2014/03/27 13:57:58]

We are only interested in thead elements so using
Traversal<HTMLTableSectionElement> would be a bit less efficient:
- we would do the following checks while iterating:
element.hasTagName(HTMLNames::tbodyTag) ||
element.hasTagName(HTMLNames::tfootTag) ||
element.hasTagName(HTMLNames::theadTag);

Then we would still have to do a child->hasLocalName(theadTag) inside the loop.

It is a bit unfortunate we don't have a specific type for thead elements (e.g.
HTMLTableHeadElement which would subclass HTMLTableSectionElement).

+        if (child->hasLocalName(theadTag)) {
             if (HTMLTableRowElement* lastRow = Traversal<HTMLTableRowElement>::lastChild(*child))
                 return lastRow;
         }