Closed Bug 813034 Opened 9 years ago Closed 9 years ago

Implement table.createTBody

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: kohei.yoshino, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 6 obsolete files)

Keywords: dev-doc-needed
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++]
Attached patch table.createTBody v1 (obsolete) — Splinter Review
Try results:
https://tbpl.mozilla.org/?tree=Try&rev=2a89c6831c65
Assignee: nobody → marc.jessome
Attachment #688253 - Flags: feedback?(Ms2ger)
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?
Attached file Testcase1 (obsolete) —
Could you upload a patch to implement just createTBody, as the others don't exist in the spec?
Attached patch table.createTBody v2 (obsolete) — Splinter Review
Remove implementation of deleteTBody() as it is not in spec and remove implementation of getTBody().
Attachment #688253 - Attachment is obsolete: true
Attachment #688253 - Flags: feedback?(Ms2ger)
Attachment #688795 - Flags: feedback?(Ms2ger)
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.
Attachment #688795 - Flags: feedback?(Ms2ger) → feedback+
Attached file Testcase v2 (obsolete) —
Attachment #688287 - Attachment is obsolete: true
Attached patch table.createTBody v3 (obsolete) — Splinter Review
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.
Attachment #688795 - Attachment is obsolete: true
Attachment #689242 - Flags: feedback?(Ms2ger)
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();
}
Attached patch table.createTBody v4 (obsolete) — Splinter Review
Attachment #689242 - Attachment is obsolete: true
Attachment #689242 - Flags: feedback?(Ms2ger)
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.
Flags: needinfo?(Ms2ger)
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.
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.
Flags: needinfo?(Ms2ger)
Stealing this; looking at writing tests now.
Assignee: marc.jessome → Ms2ger
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++]
Attached patch Patch v5Splinter Review
I updated this.
Attachment #689237 - Attachment is obsolete: true
Attachment #689793 - Attachment is obsolete: true
Attachment #769329 - Flags: review?(mounir)
Flags: in-testsuite+
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?
Attachment #769329 - Flags: superreview?(bugs)
Attachment #769329 - Flags: review?(mounir)
Attachment #769329 - Flags: review+
All I'm removing is the list of expected failures, not the test.
Attachment #769329 - Flags: superreview?(bugs) → superreview+
https://hg.mozilla.org/mozilla-central/rev/33290309ae97
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.