Bug 132570 – [HTML] Default argument to HTMLTableSectionElement.insertRow() should be -1

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

The default argument to HTMLTableSectionElement.insertRow() should be -1, not 0, according to the specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#the-tbody-element 

Firefox 29 and IE11 both match the specification. Blink is about to change its behavior as well:
http://code.google.com/p/chromium/issues/detail?id=369803
https://codereview.chromium.org/263193002/

[Chris Dumez]

Created attachment 230837 [details]
Patch

[Chris Dumez]

Comment on attachment 230837 [details]
Patch

FYI, this change has now landed in Blink.

[Chris Dumez]

FYI, if this change is approved, I will upload another one for HTMLTableRowElement.insertCell() as well (https://codereview.chromium.org/263323004/).

[Darin Adler]

Comment on attachment 230837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230837&action=review

> Source/WebCore/html/HTMLTableSectionElement.cpp:61
> +PassRefPtr<HTMLElement> HTMLTableSectionElement::insertRow(ExceptionCode& ec)
> +{
> +    // The default 'index' argument value is -1.
> +    return insertRow(-1, ec);
> +}

Why not put this into the header and mark it inline? I see no reason to add another level of function call overhead just to supply this default argument.

[Chris Dumez]

Created attachment 230915 [details]
Patch

[Chris Dumez]

Comment on attachment 230837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230837&action=review

>> Source/WebCore/html/HTMLTableSectionElement.cpp:61
>> +}
> 
> Why not put this into the header and mark it inline? I see no reason to add another level of function call overhead just to supply this default argument.

Agreed, I updated the patch.

[WebKit Commit Bot]

Comment on attachment 230915 [details]
Patch

Clearing flags on attachment: 230915

Committed r168365: <http://trac.webkit.org/changeset/168365>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.