Bug 84465 – Implement createTBody for table element.

• Component: Tables
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

Implement createTBody for table element.

[Alexis Menard (darktears)]

Created attachment 138113 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #1)
> Created an attachment (id=138113) [details]
> Patch

First try.

[Ojan Vafai]

Comment on attachment 138113 [details]
Patch

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

Nice tests!

It's not clear to me that we want to implement this since this can all be done with the regular DOM APIs. Can you see if other browsers implement this? Namely, IE10, latest Firefox and latest Opera. If they do, then I'm fine with adding it to WebKit. If they don't we should probably just get it removed from the spec.

r- for the code comments and for needing more information about other implementations.

> Source/WebCore/html/HTMLTableElement.cpp:163
> +    if (existingBody) {

Since this variable is only used here, you can just call lastBody() here and don't need the local variable.

> Source/WebCore/html/HTMLTableElement.cpp:168
> +        insertBefore(body, child->nextSibling(), ec);

Isn't child == lastBody()?

[Alexis Menard (darktears)]

Comment on attachment 138113 [details]
Patch

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

>> Source/WebCore/html/HTMLTableElement.cpp:163
>> +    if (existingBody) {
> 
> Since this variable is only used here, you can just call lastBody() here and don't need the local variable.

Good point.

>> Source/WebCore/html/HTMLTableElement.cpp:168
>> +        insertBefore(body, child->nextSibling(), ec);
> 
> Isn't child == lastBody()?

Can't you have (maybe broken) <table>...<tbody>...</tbody><tbody></tbody><tr></tr>...</table>? I played it safe.

[Alexis Menard (darktears)]

(In reply to comment #3)
> (From update of attachment 138113 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138113&action=review
> 
> Nice tests!
> 
> It's not clear to me that we want to implement this since this can all be done with the regular DOM APIs. Can you see if other browsers implement this? Namely, IE10, latest Firefox and latest Opera. If they do, then I'm fine with adding it to WebKit. If they don't we should probably just get it removed from the spec.

createTFoot and createTHead existed before so maybe the goal was to be consistent but hey I'm in doubt now, I will send an email to whatwg.

> 
> r- for the code comments and for needing more information about other implementations.
> 
> > Source/WebCore/html/HTMLTableElement.cpp:163
> > +    if (existingBody) {
> 
> Since this variable is only used here, you can just call lastBody() here and don't need the local variable.
> 
> > Source/WebCore/html/HTMLTableElement.cpp:168
> > +        insertBefore(body, child->nextSibling(), ec);
> 
> Isn't child == lastBody()?

[Alexis Menard (darktears)]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 138113 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138113&action=review
> > 
> > Nice tests!
> > 
> > It's not clear to me that we want to implement this since this can all be done with the regular DOM APIs. Can you see if other browsers implement this? Namely, IE10, latest Firefox and latest Opera. If they do, then I'm fine with adding it to WebKit. If they don't we should probably just get it removed from the spec.
> 
> createTFoot and createTHead existed before so maybe the goal was to be consistent but hey I'm in doubt now, I will send an email to whatwg.
> 
> > 
> > r- for the code comments and for needing more information about other implementations.
> > 
> > > Source/WebCore/html/HTMLTableElement.cpp:163
> > > +    if (existingBody) {
> > 
> > Since this variable is only used here, you can just call lastBody() here and don't need the local variable.
> > 
> > > Source/WebCore/html/HTMLTableElement.cpp:168
> > > +        insertBefore(body, child->nextSibling(), ec);
> > 
> > Isn't child == lastBody()?

Seems like we should implement it.

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-April/035522.html

[Alexis Menard (darktears)]

Created attachment 138401 [details]
Patch

[Build Bot]

Comment on attachment 138401 [details]
Patch

Attachment 138401 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12524003

[Alexis Menard (darktears)]

(In reply to comment #8)
> (From update of attachment 138401 [details])
> Attachment 138401 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/12524003

Mac EWS segfault-ed :(.

[Ojan Vafai]

> > > It's not clear to me that we want to implement this since this can all be done with the regular DOM APIs. Can you see if other browsers implement this? Namely, IE10, latest Firefox and latest Opera. If they do, then I'm fine with adding it to WebKit. If they don't we should probably just get it removed from the spec.
 
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-April/035522.html

I'd still like to see the support this method has in different browsers before approving the patch.

(In reply to comment #4)
> (From update of attachment 138113 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138113&action=review
> 
> >> Source/WebCore/html/HTMLTableElement.cpp:168
> >> +        insertBefore(body, child->nextSibling(), ec);
> > 
> > Isn't child == lastBody()?
> 
> Can't you have (maybe broken) <table>...<tbody>...</tbody><tbody></tbody><tr></tr>...</table>? I played it safe.

I don't understand. The code to get child is exactly the same code as lastBody except it has an extra unnecessary check that it's an HTMLElement.

Also, insertBefore will call appendChild if you pass a null reference child. So, this code could just be:

RefPtr<HTMLTableSectionElement> body = HTMLTableSectionElement::create(tbodyTag, document());
var referenceElement = lastBody() ? lastBody()->nextSibling() : 0;
ExceptionCode ec;
insertBefore(body, referenceElement, ec);
return body.release();

Also, it would be good if you could add a test for the case above.

[Ojan Vafai]

Comment on attachment 138401 [details]
Patch

r- per the previous comment

[Alexis Menard (darktears)]

(In reply to comment #10)
> > > > It's not clear to me that we want to implement this since this can all be done with the regular DOM APIs. Can you see if other browsers implement this? Namely, IE10, latest Firefox and latest Opera. If they do, then I'm fine with adding it to WebKit. If they don't we should probably just get it removed from the spec.
> 
> > http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-April/035522.html
> 
> I'd still like to see the support this method has in different browsers before approving the patch.
> 
> (In reply to comment #4)
> > (From update of attachment 138113 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138113&action=review
> > 
> > >> Source/WebCore/html/HTMLTableElement.cpp:168
> > >> +        insertBefore(body, child->nextSibling(), ec);
> > > 
> > > Isn't child == lastBody()?
> > 
> > Can't you have (maybe broken) <table>...<tbody>...</tbody><tbody></tbody><tr></tr>...</table>? I played it safe.
> 
> I don't understand. The code to get child is exactly the same code as lastBody except it has an extra unnecessary check that it's an HTMLElement.
> 
> Also, insertBefore will call appendChild if you pass a null reference child. So, this code could just be:
> 
> RefPtr<HTMLTableSectionElement> body = HTMLTableSectionElement::create(tbodyTag, document());
> var referenceElement = lastBody() ? lastBody()->nextSibling() : 0;
> ExceptionCode ec;
> insertBefore(body, referenceElement, ec);
> return body.release();

Sorry for overlooking, yes you are totally right we don't need to loop around, lastBody does it for us.

> 
> Also, it would be good if you could add a test for the case above.

Well it appears that if you do : <table>...<tbody>...</tbody><tbody></tbody><tr></tr>...</table> the additional tr outside will be automatically wrapped into a tbody so the test doesn't bring much compared to the one which test with multiple tbodies.

[Alexis Menard (darktears)]

Created attachment 138439 [details]
Patch

New patch with comments taken into account, cq+ for later.

[Ian 'Hixie' Hickson]

createTBody() is new, I added it for consistency with the other two, as part of a general effort to make the API more sane. However, that doesn't mean you have to implement it. Only you can decide that. :-) If it's a good idea, you should implement it, as the more browsers implement it the more likely other browsers are to implement as well. If it's a bad idea, then you should not, and, if you think it's an especially bad idea, you should ask for it to be removed from the spec and encourage other browser vendors not to implement it either. At the end of the day, I will update the spec to match what the implementations do.

[Alexis Menard (darktears)]

(In reply to comment #14)
> createTBody() is new, I added it for consistency with the other two, as part of a general effort to make the API more sane. However, that doesn't mean you have to implement it. Only you can decide that. :-) If it's a good idea, you should implement it, as the more browsers implement it the more likely other browsers are to implement as well. If it's a bad idea, then you should not, and, if you think it's an especially bad idea, you should ask for it to be removed from the spec and encourage other browser vendors not to implement it either. At the end of the day, I will update the spec to match what the implementations do.

I feel like it's a nice convenience to have (avoid the user to write the DOM traversal code) and the patch is pretty small to be maintenance burden. We could also be the first to implement it.

[Julien Chaffraix]

Comment on attachment 138439 [details]
Patch

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

For the record, I am supportive of the change as it makes sense from an API perspective and it is pretty low maintenance.

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

several spaces here?

> Source/WebCore/html/HTMLTableElement.cpp:164
> +    ExceptionCode ec;
> +    insertBefore(body, referenceElement, ec);

You should have some checks that we don't dispatch an exception here.

insertBefore(body, referenceElement, ASSERT_NO_EXCEPTION);

> LayoutTests/fast/table/table-create-tbody-existing-tbody.html:1
> +<script src="../../resources/dump-as-markup.js"></script>

Not repeated, please add a doctype: <!DOCTYPE html>.

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

> LayoutTests/fast/table/table-create-tbody-existing-tbody.html:23
> +    <tr>
> +      <th></th>
> +      <th></th>
> +    </tr>
> +  </thead>
> +  <tfoot>
> +    <tr>
> +      <td></td>
> +      <td></td>
> +    </tr>
> +  </tfoot>
> +  <tbody>
> +    <tr>
> +      <td></td>
> +      <td></td>
> +    </tr>
> +    <tr>
> +      <td></td>
> +      <td></td>
> +    </tr>

Not repeated, but do we really need any content in the table-row-group (or sections in WebKit land). If you need to tell a <tbody> from another one, having an attribute should be enough.

> LayoutTests/fast/table/table-create-tbody-existing-tbody.html:27
> +var sel = window.getSelection();

Do we need this line in each test cases as we don't really need any selection?

[Ojan Vafai]

Comment on attachment 138439 [details]
Patch

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

[Alexis Menard (darktears)]

Created attachment 138685 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138685 [details]
Patch

Clearing flags on attachment: 138685

Committed r115160: <http://trac.webkit.org/changeset/115160>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.