Bug 84465 - Implement createTBody for table element.
Summary: Implement createTBody for table element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-20 10:28 PDT by Alexis Menard (darktears)
Modified: 2012-04-24 19:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.48 KB, patch)
2012-04-20 10:30 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (9.39 KB, patch)
2012-04-23 12:47 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (9.25 KB, patch)
2012-04-23 15:39 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (8.10 KB, patch)
2012-04-24 16:40 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-04-20 10:28:29 PDT
Implement createTBody for table element.
Comment 1 Alexis Menard (darktears) 2012-04-20 10:30:31 PDT
Created attachment 138113 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-04-20 10:34:50 PDT
(In reply to comment #1)
> Created an attachment (id=138113) [details]
> Patch

First try.
Comment 3 Ojan Vafai 2012-04-20 17:59:26 PDT
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()?
Comment 4 Alexis Menard (darktears) 2012-04-23 06:06:38 PDT
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.
Comment 5 Alexis Menard (darktears) 2012-04-23 06:11:56 PDT
(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()?
Comment 6 Alexis Menard (darktears) 2012-04-23 10:14:12 PDT
(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
Comment 7 Alexis Menard (darktears) 2012-04-23 12:47:11 PDT
Created attachment 138401 [details]
Patch
Comment 8 Build Bot 2012-04-23 13:04:14 PDT
Comment on attachment 138401 [details]
Patch

Attachment 138401 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12524003
Comment 9 Alexis Menard (darktears) 2012-04-23 13:24:22 PDT
(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 :(.
Comment 10 Ojan Vafai 2012-04-23 14:48:19 PDT
> > > 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.
Comment 11 Ojan Vafai 2012-04-23 14:48:37 PDT
Comment on attachment 138401 [details]
Patch

r- per the previous comment
Comment 12 Alexis Menard (darktears) 2012-04-23 15:23:32 PDT
(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.
Comment 13 Alexis Menard (darktears) 2012-04-23 15:39:10 PDT
Created attachment 138439 [details]
Patch

New patch with comments taken into account, cq+ for later.
Comment 14 Ian 'Hixie' Hickson 2012-04-23 15:45:24 PDT
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.
Comment 15 Alexis Menard (darktears) 2012-04-23 16:23:13 PDT
(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.
Comment 16 Julien Chaffraix 2012-04-24 15:53:33 PDT
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?
Comment 17 Ojan Vafai 2012-04-24 16:15:25 PDT
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);
Comment 18 Alexis Menard (darktears) 2012-04-24 16:40:44 PDT
Created attachment 138685 [details]
Patch
Comment 19 WebKit Review Bot 2012-04-24 19:10:44 PDT
Comment on attachment 138685 [details]
Patch

Clearing flags on attachment: 138685

Committed r115160: <http://trac.webkit.org/changeset/115160>
Comment 20 WebKit Review Bot 2012-04-24 19:10:50 PDT
All reviewed patches have been landed.  Closing bug.