Kohei Yoshino [:kohei] Reporter
	Description • 12 years ago

Spec: http://dev.w3.org/html5/spec/the-table-element.html#dom-table-createtbody
WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=84465

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 12 years ago

Whoever implements this, please use WhatWG spec
http://www.whatwg.org/specs/web-apps/current-work/multipage/tabular-data.html#dom-table-createtbody

	Josh Matthews [:jdm]
	Comment 2 • 12 years ago

Relevant:
* http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/html/nsIDOMHTMLTableElement.idl
* http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTableElement.cpp

	Marc Jessome[:mjessome]
	Comment 3 • 12 years ago

Try results:
https://tbpl.mozilla.org/?tree=Try&rev=2a89c6831c65

	Marc Jessome[:mjessome]
	Comment 4 • 12 years ago

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?

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 6 • 12 years ago

Could you upload a patch to implement just createTBody, as the others don't exist in the spec?

	Marc Jessome[:mjessome]
	Comment 7 • 12 years ago

Remove implementation of deleteTBody() as it is not in spec and remove implementation of getTBody().

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 8 • 12 years ago

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.

	Marc Jessome[:mjessome]
	Comment 10 • 12 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 11 • 12 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 13 • 11 years ago

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.

	Marc Jessome[:mjessome]
	Comment 14 • 11 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 15 • 11 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 16 • 11 years ago

Stealing this; looking at writing tests now.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 17 • 11 years ago

I updated this.

	Mounir Lamouri (:mounir)
	Comment 18 • 11 years ago

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?

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 19 • 11 years ago

All I'm removing is the list of expected failures, not the test.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Assignee
	Comment 20 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/33290309ae97