[webkit-reviews] review denied: [Bug 84465] Implement createTBody for table element. : [Attachment 138439] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Alexis Menard (darktears)
<alexis.menard at openbossa.org>'s request for review:
Bug 84465: Implement createTBody for table element.
https://bugs.webkit.org/show_bug.cgi?id=84465

Attachment 138439: Patch
https://bugs.webkit.org/attachment.cgi?id=138439&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138439&action=review


r- per Julien's and my comments.

I'm a bit ambivalent on implementing this. I suppose the maintenance cost is
basically nil. If we were starting from scratch, I'd not implement any of these
table-specific DOM methods, but since we're stuck with the others, I'm OK with
implementing this.

Alexis, please address our comments and then one of Julien or I can r+.

>> Source/WebCore/html/HTMLTableElement.cpp:161
>> +	HTMLTableSectionElement* lastTBody =  lastBody();
> 
> several spaces here?

I don't think we need the local variable here. I'm pretty sure the compiler
will be intelligent about reusing lastBody() below.

>> LayoutTests/fast/table/table-create-tbody-existing-tbody.html:2
>> +<table id="table">
> 
> Could we have some text about: the bug id (bonus point if it's clickable),
the bug title and what you are the condition for the test to pass / what do you
expect in the output.

Also, the table doesn't need an id. You can just pass the pointer to the table
directly into Markup.dump.

> LayoutTests/fast/table/table-create-tbody-existing-tbody.html:30
> +Markup.dump("table");

This should be Markup.dump(table);