824907 - Convert HTML table/row/section/cell elements to WebIDL

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This is the table-related part of bug 824823.  Need to convert:

  HTMLTableElement
  HTMLTableSectionElement
  HTMLTableCellElement
  HTMLTableRowElement

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Move and rename some HTML element classes v1

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Convert HTMLTableElement v1

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Convert HTMLTableCaptionElement v1

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Convert HTMLTableSectionElement v1

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Convert HTMLTableCellElement v1

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: Convert HTMLTableRowElement v1

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: Convert HTMLTableColElement v1

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 696512 [details] [diff] [review]
Convert HTMLTableElement v1

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

::: content/html/content/src/HTMLTableElement.h
@@ +48,5 @@
> +  {
> +    DeleteCaption();
> +    if (aCaption) {
> +      mozilla::ErrorResult rv;
> +      nsINode::AppendChild(*aCaption, rv);

Assert that it didn't fail or propagate the exception

::: content/html/content/test/reflect.js
@@ +55,5 @@
>    element.removeAttribute(contentAttr);
>  
>    element[idlAttr] = null;
>    // TODO: remove this ugly hack when null stringification will work as expected.
> +  var todo = {

I hope there aren't any calls to todo() here...

Comment 9

[David Zbarsky (:dzbarsky)]

(In reply to :Ms2ger from comment #8)
> Comment on attachment 696512 [details] [diff] [review]
> Convert HTMLTableElement v1
> 
> ::: content/html/content/test/reflect.js
> @@ +55,5 @@
> >    element.removeAttribute(contentAttr);
> >  
> >    element[idlAttr] = null;
> >    // TODO: remove this ugly hack when null stringification will work as expected.
> > +  var todo = {
> 
> I hope there aren't any calls to todo() here...

I changed WebIDL elements to take the first branch here in https://bug824327.bugzilla.mozilla.org/attachment.cgi?id=696244

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to :Ms2ger from comment #8)
> I hope there aren't any calls to todo() here...

Good point, I'll rename.

(In reply to David Zbarsky (:dzbarsky) from comment #9)
> I changed WebIDL elements to take the first branch here in
> https://bug824327.bugzilla.mozilla.org/attachment.cgi?id=696244

I prefer listing the remaining todo cases so that we know when to remove the hack (when the array is empty).

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I prefer listing the remaining todo cases so that we know when to remove the hack 

The problem is that it makes converting elements to WebIDL a minefield.

The time we can remove the hack is when all elements are on WebIDL, so imo we can just add a comment in Bindings.conf saying to remove this hack when we remove the hasXPConnectImpls for Element.

Comment 12

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> The problem is that it makes converting elements to WebIDL a minefield.
> 
> The time we can remove the hack is when all elements are on WebIDL, so imo
> we can just add a comment in Bindings.conf saying to remove this hack when
> we remove the hasXPConnectImpls for Element.

Huh? What does hasXPConnectImpls have to do with stringification of attributes?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Once all elements are on WebIDL bindings, they'll all stringify per WebIDL, so the hack can go away.  Until they all are, we need the hack.  All elements being on WebIDL is when we can remove hasXPConnectImpls for Element.

Comment 14

[Boris Zbarsky [:bzbarsky]]

One note: for HTMLTableElement.insertRow, the spec was changed to do the sane thing and move the -1 default from prose to the IDL.  See https://www.w3.org/Bugs/Public/show_bug.cgi?id=20495

No need for a separate patch, just update it locally, ok?

Comment 15

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> The problem is that it makes converting elements to WebIDL a minefield.

It doesn't, it makes it explicit when we are changing the stringification of these attributes to follow the spec. And it is rather unrelated to whether the prototype object's stringification contains "XPC", we could just as easily fix those todo's with the right annotations in the xpidl interfaces. I'd also rather force anyone adding new attributes in the xpidl interfaces to make the stringification correct straight away, until we stop using those interfaces.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm.  Ok, sold!

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696511 [details] [diff] [review]
Move and rename some HTML element classes v1

Please use mozilla/dom/HTMLWhateverElement for the #includes and mozilla_dom_HTMLWhateverElement for the include guards?

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696512 [details] [diff] [review]
Convert HTMLTableElement v1

>+++ b/content/html/content/src/HTMLTableElement.cpp
> #define DO_FOR_EACH_ROWGROUP(_code)                                  \
>+        if (_node->IsHTML(nsGkAtoms::tbody)) {                       \
>+          rowGroup = static_cast<HTMLTableSectionElement*>(_node);   \
>+          if (rowGroup) {                                            \

rowGroup can't be null here, so you can drop the null-check.

> HTMLTableElement::SetCaption(nsIDOMHTMLTableCaptionElement* aValue)
>+  HTMLTableCaptionElement* caption =
>+    static_cast<HTMLTableCaptionElement*>(aValue);

We need to flag nsIDOMHTMLTableCaptionElement as builtinclass if we're going to do this.

> HTMLTableElement::SetTHead(nsIDOMHTMLTableSectionElement* aValue)
>+  HTMLTableSectionElement* section =
>+    static_cast<HTMLTableSectionElement*>(aValue);

Likewise for nsIDOMHTMLTableSectionElement.

>+HTMLTableElement::InsertRow(const Optional<int32_t>& aIndex,

Like I said, just default this to -1 in the idl.

>+    nsIContent* rowGroup = nullptr;

This needs to be an nsCOMPtr.  Otherwise we can end up calling AppendChildTo with the thing stored in rowGroup without holding a ref to it, which seems bad.

>+HTMLTableElement::DeleteRow(int32_t aIndex, ErrorResult& aError)
>+  row->RemoveFromParent();

Do we not need to hold a strong ref to row while doing that?  I think we do.

> HTMLTableElement::ParseAttribute(int32_t aNamespaceID,
>-                                 nsIAtom* aAttribute,
>-                                 const nsAString& aValue,
>-                                 nsAttrValue& aResult)
>+                                   nsIAtom* aAttribute,
>+                                   const nsAString& aValue,
>+                                   nsAttrValue& aResult)

This change looks wrong.

> HTMLTableElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
>-                             nsIContent* aBindingParent,
>-                             bool aCompileEventHandlers)
>+                              nsIContent* aBindingParent,
>+                              bool aCompileEventHandlers)

Likewise.

> HTMLTableElement::BeforeSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>-                                const nsAttrValueOrString* aValue,
>-                                bool aNotify)
>+                                  const nsAttrValueOrString* aValue,
>+                                  bool aNotify)

And here.

> HTMLTableElement::AfterSetAttr(int32_t aNameSpaceID, nsIAtom* aName,
>-                               const nsAttrValue* aValue,
>-                               bool aNotify)
>+                                 const nsAttrValue* aValue,
>+                                 bool aNotify)

And here.

>+++ b/content/html/content/src/HTMLTableElement.h
>-#ifndef HTMLTableElement_h
>-#define HTMLTableElement_h
>+#ifndef HTMLTableElement_h___
>+#define HTMLTableElement_h___

And this: multiple trailing '_' are reserved, no?

>+  void SetCaption(HTMLTableCaptionElement* aCaption)

Does this compile?  I would think that the bindings pass an nsIDOMHTMLTableCaptionElement* here, until that element is converted to WebIDL...

Similar for SetTHead, SetTFoot.

Presumably we want to make this take an nsIDOMWhatever and static_cast from it to the right class, then update as we convert those elements.

>+++ b/dom/bindings/Bindings.conf
>+'HTMLTableElement': {

We need hasInstanceInterface here.  There's instanceof stuff for this interface in Thunderbird and various addons.

r=me with all of the above fixed.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Oh, and for that HTMLTableElement patch... the test changes seem to assume some of the later patches in this bug?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696514 [details] [diff] [review]
Convert HTMLTableCaptionElement v1

Why the debugger test change here?

r=me with that explained, at least.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> the test changes seem to assume some of the later patches in this bug?

Nevermind, I was misreading them.  But why do those elements not need to take the todo codepath right now?   I just don't see how tests can be passing both before and after that change for something like <img> unless we're simply not testing it at the moment....

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696515 [details] [diff] [review]
Convert HTMLTableSectionElement v1

Again, why the debugger test changes?

>+++ b/content/html/content/src/HTMLTableElement.cpp
>+HTMLTableSectionElement::InsertRow(const Optional<int32_t>& aIndex,

Use a default value of -1 in the IDL, please.

>+HTMLTableSectionElement::DeleteRow(int32_t aValue, ErrorResult& aError)
>+  nsINode::RemoveChild(*row, aError);

Again, don't we need to hold a strong ref to row when we do that?

>+++ b/dom/bindings/Bindings.conf
>+'HTMLTableSectionElement': {

We need hasInstanceInterface; addons are doing instanceof with this interface.

r=me with the above fixed.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696516 [details] [diff] [review]
Convert HTMLTableCellElement v1

We need hasInstanceInterface for HTMLTableCellElement due to addons.

r=me with that.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696517 [details] [diff] [review]
Convert HTMLTableRowElement v1

>+++ b/content/html/content/src/HTMLTableRowElement.cpp
>+HTMLTableRowElement::InsertCell(const Optional<int32_t>& aIndex,

Just default it to -1 in the IDL.  I filed a spec bug on this: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20564

>+  nsINode::InsertBefore(*cell, nextSibling, aError);

Shouldn't we be holding a strong ref to nextSibling here?

>+HTMLTableRowElement::DeleteCell(int32_t aValue, ErrorResult& aError)
>+  nsINode::RemoveChild(*cell, aError);

And here holding a strong ref to cell?

>+++ b/dom/bindings/Bindings.conf
>+'HTMLTableRowElement': {

This needs hasInstanceInterface.  Yay extensions.

r=me with those fixed.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 696518 [details] [diff] [review]
Convert HTMLTableColElement v1

We need hasInstanceInterface for HTMLTableColElement as well: addons use it.

r=me with that

Comment 26

[Peter Van der Beken [:peterv]]

> rowGroup can't be null here, so you can drop the null-check.

Done.

> Like I said, just default this to -1 in the idl.

Done.

> This needs to be an nsCOMPtr.  Otherwise we can end up calling AppendChildTo
> with the thing stored in rowGroup without holding a ref to it, which seems
> bad.

Done.

> Do we not need to hold a strong ref to row while doing that?  I think we do.

Done.

> This change looks wrong.

> Likewise.

> And here.

> And here.

> And this: multiple trailing '_' are reserved, no?

Sorry about these, something went wrong when splitting up the patches.

> We need to flag nsIDOMHTMLTableCaptionElement as builtinclass if we're going
> to do this.

> Likewise for nsIDOMHTMLTableSectionElement.

> Does this compile?  I would think that the bindings pass an
> nsIDOMHTMLTableCaptionElement* here, until that element is converted to
> WebIDL...
> 
> Similar for SetTHead, SetTFoot.

We ended up using the XPCOM functions because I forgot to mark tHead and tFoot as SetterThrows. Fixed by doing a QI to nsIContent, checking IsHTML(...) and casting.

> We need hasInstanceInterface here.  There's instanceof stuff for this
> interface in Thunderbird and various addons.

Done.

> Use a default value of -1 in the IDL, please.

Done.

> Again, don't we need to hold a strong ref to row when we do that?

Done.

> We need hasInstanceInterface; addons are doing instanceof with this
> interface.

Done.

> We need hasInstanceInterface for HTMLTableCellElement due to addons.

Done.

> Just default it to -1 in the IDL.

Done.

> Shouldn't we be holding a strong ref to nextSibling here?

Done.

> And here holding a strong ref to cell?

Done.

> This needs hasInstanceInterface.  Yay extensions.

Done.

> We need hasInstanceInterface for HTMLTableColElement as well: addons use it.

Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/08a616b1e4f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e18a8008ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/7564aeb63a36
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ef9c24dd5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f91a4781c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/abd3e75c995c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a67f83f18cb

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/08a616b1e4f4
https://hg.mozilla.org/mozilla-central/rev/08e18a8008ec
https://hg.mozilla.org/mozilla-central/rev/7564aeb63a36
https://hg.mozilla.org/mozilla-central/rev/a4ef9c24dd5b
https://hg.mozilla.org/mozilla-central/rev/42f91a4781c1
https://hg.mozilla.org/mozilla-central/rev/abd3e75c995c
https://hg.mozilla.org/mozilla-central/rev/0a67f83f18cb
https://hg.mozilla.org/mozilla-central/rev/3bead9c2a59b

Comment 28

[Boris Zbarsky [:bzbarsky]]

> We need to flag nsIDOMHTMLTableCaptionElement as builtinclass 
> Likewise for nsIDOMHTMLTableSectionElement.

These two comments got lost or something?  I still see casts from these interfaces to concrete types in the code that landed, which is totally not safe as long as these are not builtinclass...

Comment 29

[Peter Van der Beken [:peterv]]

Given that we switched to new bindings, how can that code be reached from script?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Hmm...  Not easily; it would require a binary addon that has IDL taking those interfaces.

But adding builtinclass shouldn't break anything either, and I'd much rather be safe here.

Comment 31

[Boris Zbarsky [:bzbarsky]]

So one bit of medium-bad news: even with this fix we're not taking the Ion+TI fast-path on dom-traverse.  Looks like TI can't figure out what's going on there for some reason... I'll try to catch bhackett and see what's up.  If it's that fragile, we may want to reevaluate the slower path we take when TI fails right now.  :(

Comment 32

[Boris Zbarsky [:bzbarsky]]

Looks like the real issue there was bug 827214.  Mostly.  Still seeing some slowness on lastChild that I'm debugging.