HTMLTableSectionElement.insertRow(0) / HTMLTableRowElement.insertCell(0) do not behave correctly

HTMLTableSectionElement.insertRow(0) / HTMLTableRowElement.insertCell(0) do not behave correctly

HTMLTableSectionElement.insertRow(0) / HTMLTableRowElement.insertCell(0) do not
behave correctly according to the HTML specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#dom-tbody-insertrow
http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#dom-tr-insertcell

Previously, Chromium was adding the new element before the first node, instead
of before the first <tr> (for insertRow) or <td> / <th> (for insertCell)
element.

The new behavior also matches the behavior of Firefox 29.

R=arv@chromium.org, tkent@chromium.org
BUG=369803
TEST=fast/dom/HTMLTableSectionElement/insertRow-skips-non-tr.html
TEST=fast/dom/HTMLTableRowElement/insertCell-skips-non-td-th.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173459

[arv (Not doing code reviews), 2014-05-06 22:08:33]

LGTM

[Inactive, 2014-05-06 23:44:56]

Could an API OWNER take a look as well?

[tkent, 2014-05-07 01:18:23]

This looks a simple bug fix. I don't think it needs API owner approval.

https://codereview.chromium.org/262163008/diff/20001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLTableRowElement/insertCell-skips-non-td-th.html
(right):

https://codereview.chromium.org/262163008/diff/20001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLTableRowElement/insertCell-skips-non-td-th.html:17: //
The insertRow() method must create a td element, insert it as a child of the
insetRow() -> insertCell().

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

https://codereview.chromium.org/262163008/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableRowElement.cpp:135: Element* n =
children->item(index);
What happens if index == -2?

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

https://codereview.chromium.org/262163008/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableSectionElement.cpp:79: Element* n =
children->item(index);
nit: We can remove the local variable |n| and fold this line into the next.

[Inactive, 2014-05-07 01:33:20]

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

https://codereview.chromium.org/262163008/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableRowElement.cpp:135: Element* n =
children->item(index);
On 2014/05/07 01:18:23, tkent wrote:
> What happens if index == -2?

This cannot happen, we already check for this above (line 126).

[Inactive, 2014-05-07 01:37:13]

https://codereview.chromium.org/262163008/diff/20001/LayoutTests/fast/dom/HTM...
File LayoutTests/fast/dom/HTMLTableRowElement/insertCell-skips-non-td-th.html
(right):

https://codereview.chromium.org/262163008/diff/20001/LayoutTests/fast/dom/HTM...
LayoutTests/fast/dom/HTMLTableRowElement/insertCell-skips-non-td-th.html:17: //
The insertRow() method must create a td element, insert it as a child of the
On 2014/05/07 01:18:23, tkent wrote:
> insetRow() -> insertCell().

Done.

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

https://codereview.chromium.org/262163008/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableSectionElement.cpp:79: Element* n =
children->item(index);
On 2014/05/07 01:18:23, tkent wrote:
> nit: We can remove the local variable |n| and fold this line into the next.

Done.

[Inactive, 2014-05-07 01:39:37]

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

[commit-bot: I haz the power, 2014-05-07 01:40:38]

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

[tkent, 2014-05-07 01:46:07]

lgtm

[Inactive, 2014-05-07 01:53:03]

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

[Inactive, 2014-05-07 01:54:01]

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

[commit-bot: I haz the power, 2014-05-07 01:55:02]

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

[commit-bot: I haz the power, 2014-05-07 03:37:08]

Change committed as 173459