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

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: peterv)

Tracking

(Blocks 1 bug)

unspecified
mozilla20
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

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

  HTMLTableElement
  HTMLTableSectionElement
  HTMLTableCellElement
  HTMLTableRowElement
Assignee: bzbarsky → peterv
Status: NEW → ASSIGNED
Attachment #696512 - Flags: review?(bzbarsky)
Attachment #696514 - Flags: review?(bzbarsky)
Attachment #696515 - Flags: review?(bzbarsky)
Attachment #696516 - Flags: review?(bzbarsky)
Attachment #696517 - Flags: review?(bzbarsky)
Attachment #696518 - Flags: review?(bzbarsky)
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...
(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
(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).
> 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.
(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?
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.
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?
(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.
Hmm.  Ok, sold!
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
Attachment #696511 - Flags: review?(bzbarsky) → review+
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.
Attachment #696512 - Flags: review?(bzbarsky) → review+
Oh, and for that HTMLTableElement patch... the test changes seem to assume some of the later patches in this bug?
Comment on attachment 696514 [details] [diff] [review]
Convert HTMLTableCaptionElement v1

Why the debugger test change here?

r=me with that explained, at least.
Attachment #696514 - Flags: review?(bzbarsky) → review+
> 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 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.
Attachment #696515 - Flags: review?(bzbarsky) → review+
Comment on attachment 696516 [details] [diff] [review]
Convert HTMLTableCellElement v1

We need hasInstanceInterface for HTMLTableCellElement due to addons.

r=me with that.
Attachment #696516 - Flags: review?(bzbarsky) → review+
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.
Attachment #696517 - Flags: review?(bzbarsky) → review+
Comment on attachment 696518 [details] [diff] [review]
Convert HTMLTableColElement v1

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

r=me with that
Attachment #696518 - Flags: review?(bzbarsky) → review+
> 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
> 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...
Given that we switched to new bindings, how can that code be reached from script?
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.
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.  :(
Looks like the real issue there was bug 827214.  Mostly.  Still seeing some slowness on lastChild that I'm debugging.
Duplicate of this bug: 668818
Duplicate of this bug: 668819
Depends on: 828180
Depends on: 831408
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.