Use Traversal<HTMLElement> API in HTMLTableRowsCollection

Use Traversal<HTMLElement> API in HTMLTableRowsCollection

Use Traversal<HTMLElement> API in HTMLTableRowsCollection for clarity and
performance:
- It makes rowAfter() and lastRow() more consistent
- It makes it clear we are only interested in HTMLElements
- We can use hadLocalName() instead of hasTagName() as we only iterate through
  HTML elements
- We can use the slightly faster lastChild(const ContainerNode&) overload in
  lastRow() now that we know at compile time the argument is not merely a
  generic Node.

R=esprehn, adamk
BUG=346733
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170120

[Inactive, 2014-03-20 22:15:24]

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLEle...
File Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.h:131: template <> inline bool
isElementOfType<const HTMLElement>(const Node& node) { return
node.isHTMLElement(); }
Without this template specialization, isElementOfType<const HTMLElement>(node)
would do:
node.isElementNode() && toElement(node).isHTMLElement()

Since isHTMLElement() is a method on Node and checks a flag on Node, the call to
isElementNode() is redundant and can be skipped for efficiency.

[adamk, 2014-03-24 17:40:30]

lgtm

[Inactive, 2014-03-27 01:37:21]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-03-27 01:37:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/206743005/20001

[commit-bot: I haz the power, 2014-03-27 02:07:38]

Change committed as 170120

[abarth-chromium, 2014-03-27 03:14:50]

neat

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

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableRowsCollection.cpp (right):

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableRowsCollection.cpp:85: for (; child; child =
Traversal<HTMLElement>::nextSibling(*child)) {
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.

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableRowsCollection.cpp:127: for (HTMLElement* child =
Traversal<HTMLElement>::lastChild(table); child; child =
Traversal<HTMLElement>::previousSibling(*child)) {
Why not use Traversal<HTMLTableSectionElement> ?

[Inactive, 2014-03-27 13:57:57]

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableRowsCollection.cpp (right):

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableRowsCollection.cpp:85: for (; child; child =
Traversal<HTMLElement>::nextSibling(*child)) {
On 2014/03/27 13:35:49, esprehn wrote:
> 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.

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.

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableRowsCollection.cpp:127: for (HTMLElement* child =
Traversal<HTMLElement>::lastChild(table); child; child =
Traversal<HTMLElement>::previousSibling(*child)) {
On 2014/03/27 13:35:49, esprehn wrote:
> Why not use Traversal<HTMLTableSectionElement> ?

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).

[Inactive, 2014-03-27 14:39:53]

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableRowsCollection.cpp (right):

https://codereview.chromium.org/206743005/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableRowsCollection.cpp:85: for (; child; child =
Traversal<HTMLElement>::nextSibling(*child)) {
On 2014/03/27 13:35:49, esprehn wrote:
> 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.

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.