Last Comment Bug 813034 - Implement table.createTBody
: Implement table.createTBody
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla25
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on:
Blocks: html5
  Show dependency treegraph
 
Reported: 2012-11-19 04:00 PST by Kohei Yoshino [:kohei]
Modified: 2013-07-10 09:39 PDT (History)
12 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
table.createTBody v1 (11.94 KB, patch)
2012-12-04 07:16 PST, Marc Jessome[:mjessome]
no flags Details | Diff | Splinter Review
Testcase1 (520 bytes, text/html)
2012-12-04 08:46 PST, Marc Jessome[:mjessome]
no flags Details
table.createTBody v2 (1.76 KB, patch)
2012-12-05 08:10 PST, Marc Jessome[:mjessome]
Ms2ger: feedback+
Details | Diff | Splinter Review
Testcase v2 (1013 bytes, text/html)
2012-12-06 09:32 PST, Marc Jessome[:mjessome]
no flags Details
table.createTBody v3 (2.51 KB, patch)
2012-12-06 09:38 PST, Marc Jessome[:mjessome]
no flags Details | Diff | Splinter Review
table.createTBody v4 (2.98 KB, patch)
2012-12-07 10:04 PST, Marc Jessome[:mjessome]
no flags Details | Diff | Splinter Review
Patch v5 (4.94 KB, patch)
2013-06-29 00:16 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review+
bugs: superreview+
Details | Diff | Splinter Review

Comment 1 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-11-19 04:02:47 PST
Whoever implements this, please use WhatWG spec
http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#dom-table-createtbody
Comment 3 Marc Jessome[:mjessome] 2012-12-04 07:16:32 PST
Created attachment 688253 [details] [diff] [review]
table.createTBody v1

Try results:
https://tbpl.mozilla.org/?tree=Try&rev=2a89c6831c65
Comment 4 Marc Jessome[:mjessome] 2012-12-04 07:38:06 PST
I've just realized that the deleteTBody(), which I had implemented to delete the first TBody element, is not a part of the spec. Does this also remove the need for GetTBody() as implemented?
Comment 5 Marc Jessome[:mjessome] 2012-12-04 08:46:00 PST
Created attachment 688287 [details]
Testcase1
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-12-04 10:50:29 PST
Could you upload a patch to implement just createTBody, as the others don't exist in the spec?
Comment 7 Marc Jessome[:mjessome] 2012-12-05 08:10:12 PST
Created attachment 688795 [details] [diff] [review]
table.createTBody v2

Remove implementation of deleteTBody() as it is not in spec and remove implementation of getTBody().
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-12-05 11:08:32 PST
Comment on attachment 688795 [details] [diff] [review]
table.createTBody v2

Review of attachment 688795 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks good; will need tests, though.

::: content/html/content/src/nsHTMLTableElement.cpp
@@ +593,5 @@
> +    *aValue = nullptr;
> +
> +    nsCOMPtr<nsINodeInfo> nodeInfo;
> +    nsContentUtils::NameChanged(mNodeInfo, nsGkAtoms::tbody,
> +                                getter_AddRefs(nodeInfo));

Need to check for out-of-memory here

@@ +601,5 @@
> +    if (!newBody) {
> +        return NS_OK;
> +    }
> +
> +    AppendChildTo(newBody, true);

This is incorrect; you need to add the new tbody immediately after the last tbody element in the table. For example, calling createTBody on the following table:

<table>
<tbody></tbody>
<tfoot></tfoot>
</table>

should insert the new tbody between the tbody and the tfoot.
Comment 9 Marc Jessome[:mjessome] 2012-12-06 09:32:50 PST
Created attachment 689237 [details]
Testcase v2
Comment 10 Marc Jessome[:mjessome] 2012-12-06 09:38:22 PST
Created attachment 689242 [details] [diff] [review]
table.createTBody v3

This will add the new TBody element after the last one if there is one that exists, or as the last element of the table as the spec states.

I am unsure of what should be done in the case that "lastBody" cannot be retrieved.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2012-12-06 09:49:37 PST
Comment on attachment 689242 [details] [diff] [review]
table.createTBody v3

Review of attachment 689242 [details] [diff] [review]:
-----------------------------------------------------------------

This can be made somewhat more efficient by using nsINode APIs:

::: content/html/content/src/nsHTMLTableElement.cpp
@@ +604,5 @@
> +        return NS_OK;
> +    }
> +
> +    nsCOMPtr<nsIDOMHTMLCollection> bodies;
> +    GetTBodies(getter_AddRefs(bodies));

nsContentList* bodies = TBodies();

@@ +607,5 @@
> +    nsCOMPtr<nsIDOMHTMLCollection> bodies;
> +    GetTBodies(getter_AddRefs(bodies));
> +
> +    uint32_t bodyCount;
> +    bodies->GetLength(&bodyCount);

uint32_t bodyCount = bodies->GetLength();

@@ +609,5 @@
> +
> +    uint32_t bodyCount;
> +    bodies->GetLength(&bodyCount);
> +
> +    nsCOMPtr<nsIDOMNode> nextElem;

nsIContent* nextElem = nullptr;

@@ +615,5 @@
> +      // Already have tbody elements, insert after the last one
> +      nsCOMPtr<nsIDOMNode> lastBody;
> +      bodies->Item(bodyCount - 1, getter_AddRefs(lastBody));
> +
> +      lastBody->GetNextSibling(getter_AddRefs(nextElem));

Element* lastBody = bodies->GetElementAt(bodyCount - 1);
nextElem = lastBody->GetNextSibling();

@@ +628,5 @@
> +      nsresult r;
> +      // First tbody element
> +      r = AppendChildTo(newBody, true);
> +      NS_ENSURE_SUCCESS(r, r);
> +    }

This can all be replaced by

ErrorResult rv;
InsertBefore(*newBody, nextElem, rv);
if (rv.Failed()) {
  return rv.ErrorCode();
}
Comment 12 Marc Jessome[:mjessome] 2012-12-07 10:04:59 PST
Created attachment 689793 [details] [diff] [review]
table.createTBody v4
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2013-01-07 00:18:18 PST
Hey Marc,

Sorry I let this patch fall through the cracks. It'll need a bit of updating to the new WebIDL bindings. Let me know if you want to do that; else I'll look at it next week or so.
Comment 14 Marc Jessome[:mjessome] 2013-01-07 12:21:28 PST
I'd be curious to see some information on using the WebIDL bindings, as I am unfamiliar with them. I'd be interested in looking at it, but it would obviously take me a bit of time to look at it.
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2013-01-08 04:02:43 PST
There is some documentation at <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings>; it would be useful if you let us know if anything you need to get started isn't documented there.
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2013-04-15 08:09:08 PDT
Stealing this; looking at writing tests now.
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2013-06-29 00:16:06 PDT
Created attachment 769329 [details] [diff] [review]
Patch v5

I updated this.
Comment 18 Mounir Lamouri (:mounir) 2013-07-01 09:16:55 PDT
Comment on attachment 769329 [details] [diff] [review]
Patch v5

Review of attachment 769329 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but I would like Olli to do the spec review. I do not know much about table.

Also, why are you removing the test instead of moving it out of the failure directory?
Comment 19 :Ms2ger (⌚ UTC+1/+2) 2013-07-01 12:12:05 PDT
All I'm removing is the list of expected failures, not the test.
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2013-07-10 09:39:20 PDT
https://hg.mozilla.org/mozilla-central/rev/33290309ae97

Note You need to log in before you can comment on or make changes to this bug.