Open Bug 893117 Opened 6 years ago Updated 6 months ago

Remove xpidl for HTML elements

Categories

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

defect
Not set

Tracking

()

ASSIGNED

People

(Reporter: dzbarsky, Assigned: dzbarsky)

References

Details

(Keywords: addon-compat, Whiteboard: [leave open])

Attachments

(25 files, 2 obsolete files)

11.46 KB, patch
peterv
: review+
Details | Diff | Splinter Review
12.81 KB, patch
peterv
: review+
Details | Diff | Splinter Review
13.35 KB, patch
peterv
: review+
Details | Diff | Splinter Review
9.31 KB, patch
peterv
: review+
Details | Diff | Splinter Review
6.34 KB, patch
peterv
: review+
Details | Diff | Splinter Review
10.63 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.70 KB, patch
peterv
: review+
Details | Diff | Splinter Review
14.25 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.62 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.67 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.07 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.81 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.84 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.18 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.79 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.36 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.92 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.71 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.96 KB, patch
bzbarsky
: feedback+
Details | Diff | Splinter Review
8.78 KB, patch
bzbarsky
: feedback+
Details | Diff | Splinter Review
5.90 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
35.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.50 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #774805 - Flags: review?(peterv)
Attached patch HTMLTableElementSplinter Review
We need the interface because NoScript uses it to test for instanceof.
Attachment #774809 - Flags: review?(peterv)
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #774948 - Flags: review?(peterv)
Attachment #774949 - Flags: review?(peterv)
Attachment #774950 - Flags: review?(peterv)
Attachment #774952 - Flags: review?(peterv)
Attachment #774955 - Flags: review?(peterv)
Attachment #775381 - Flags: review?(peterv)
Attachment #775446 - Flags: review?(peterv)
Attachment #775447 - Flags: review?(peterv)
Attached patch Clean up nsFormControlFrame (obsolete) — Splinter Review
Attachment #775758 - Flags: review?(Ms2ger)
Attachment #776786 - Flags: review?(peterv)
Attached patch HTMLModElementSplinter Review
Attachment #776789 - Flags: review?(peterv)
Attachment #776790 - Flags: review?(peterv)
(In reply to David Zbarsky (:dzbarsky) from comment #2)
> Created attachment 774809 [details] [diff] [review]
> HTMLTableElement
> 
> We need the interface because NoScript uses it to test for instanceof.

Please contact the NoScript author. NoScript is very actively maintained.
If you leave the interface anyway, please bump the iid.
Comment on attachment 775758 [details] [diff] [review]
Clean up nsFormControlFrame

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

There's no nsFormControlFrame here... As I mentioned on IRC, though, I've got a patch that does a lot of this; do you mind if we drop your patch for now?
Attachment #775758 - Flags: review?(Ms2ger)
(In reply to :Ms2ger from comment #16)
> Comment on attachment 775758 [details] [diff] [review]
> Clean up nsFormControlFrame
> 
> Review of attachment 775758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's no nsFormControlFrame here... As I mentioned on IRC, though, I've
> got a patch that does a lot of this; do you mind if we drop your patch for
> now?

Oops, I meant ListControlFrame.  Sure. I can rebase this after you land.
(In reply to Masatoshi Kimura [:emk] from comment #15)
> (In reply to David Zbarsky (:dzbarsky) from comment #2)
> > Created attachment 774809 [details] [diff] [review]
> > HTMLTableElement
> > 
> > We need the interface because NoScript uses it to test for instanceof.
> 
> Please contact the NoScript author. NoScript is very actively maintained.
> If you leave the interface anyway, please bump the iid.

So, let me understand: are all the nsIDOMHTML* interfaces going away?
NoScript performs quite a lot of instanceof checks against them from a JavaScript-implemented XPCOM service (with generally no access to DOM window globals). Is there any safe replacement?
Also how can binary addons access HTML elements without nsIDOMHTML* interfaces?
Attachment #774805 - Flags: review?(peterv) → review+
Comment on attachment 774805 [details] [diff] [review]
HTMLTableCellElement

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

::: content/html/content/src/HTMLTableCellElement.h
@@ +13,5 @@
>  
>  class HTMLTableElement;
>  
>  class HTMLTableCellElement MOZ_FINAL : public nsGenericHTMLElement,
> +                                       public nsIDOMHTMLElement

Although given that we're not able to remove all nsIDOMHTML*Element interfaces it might be a good idea to have a base class that inherits from nsGenericHTMLElement and implements nsIDOMHTMLElement that these could inherit from.
Attachment #774809 - Flags: review?(peterv) → review+
Attachment #774948 - Flags: review?(peterv) → review+
Attachment #774949 - Flags: review?(peterv) → review+
Attachment #774950 - Flags: review?(peterv) → review+
Attachment #774952 - Flags: review?(peterv) → review+
Attachment #774955 - Flags: review?(peterv) → review+
Attachment #775381 - Flags: review?(peterv) → review+
(In reply to Giorgio Maone from comment #18)
> (In reply to Masatoshi Kimura [:emk] from comment #15)
> > (In reply to David Zbarsky (:dzbarsky) from comment #2)
> > > Created attachment 774809 [details] [diff] [review]
> > > HTMLTableElement
> > > 
> > > We need the interface because NoScript uses it to test for instanceof.
> > 
> > Please contact the NoScript author. NoScript is very actively maintained.
> > If you leave the interface anyway, please bump the iid.
> 
> So, let me understand: are all the nsIDOMHTML* interfaces going away?
> NoScript performs quite a lot of instanceof checks against them from a
> JavaScript-implemented XPCOM service (with generally no access to DOM window
> globals). Is there any safe replacement?

I'll only get rid of the ones that aren't being used.(In reply to Masatoshi Kimura [:emk] from comment #19)
> Also how can binary addons access HTML elements without nsIDOMHTML*
> interfaces?

I think we can codegen some wrapper classes that expose all the functions as virtual so binary addons can call them.(In reply to Peter Van der Beken [:peterv] - gone till Aug 10th 2013 from comment #20)


> Comment on attachment 774805 [details] [diff] [review]
> HTMLTableCellElement
> 
> Although given that we're not able to remove all nsIDOMHTML*Element
> interfaces it might be a good idea to have a base class that inherits from
> nsGenericHTMLElement and implements nsIDOMHTMLElement that these could
> inherit from.

Good idea.  I'll do that in a followup.
(In reply to Giorgio Maone from comment #18)
> So, let me understand: are all the nsIDOMHTML* interfaces going away?
> NoScript performs quite a lot of instanceof checks against them from a
> JavaScript-implemented XPCOM service (with generally no access to DOM window
> globals). Is there any safe replacement?

Namespace and tag check should be just as reliable.

(In reply to Masatoshi Kimura [:emk] from comment #19)
> Also how can binary addons access HTML elements without nsIDOMHTML*
> interfaces?

Most of it is syntatic sugar and should be available through nsIDOMHTMLElement/Element/Node. We could do the codegen idea that David mentioned, but in general we'd like to stop supporting things that only binary addons use and that can be replaced by something equivalent.
David, given comment 24 are you still looking for review on those remaining 5 patches?
Flags: needinfo?(dzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #26)
> David, given comment 24 are you still looking for review on those remaining
> 5 patches?

The tests it failed were broken.  I just haven't had a chance to fix yet.  I think we should land the rest of these patches.
Flags: needinfo?(dzbarsky)
Comment on attachment 775446 [details] [diff] [review]
HTMLDataListElement

r=me
Attachment #775446 - Flags: review?(peterv) → review+
Comment on attachment 775447 [details] [diff] [review]
HTMLOutputElement

>+  GetAttr(kNameSpaceID_None, nsGkAtoms::name, aValue);

GetHTMLAttr(nsGkAtoms::name, aValue).  And you could probably inline this...

>   // nsIConstraintValidation::SetCustomValidity() is fine.

It's clearly not, though, since you had to override it!

Please remove that comment.  Might be worth making sure we have tests that fail if the override is removed, too.

r=me with the above
Attachment #775447 - Flags: review?(peterv) → review+
Comment on attachment 776786 [details] [diff] [review]
HTMLDirectoryElement

r=me but add a comment in the IDL that says it's only there so it'll show up in Components.interfaces?
Attachment #776786 - Flags: review?(peterv) → review+
Comment on attachment 776789 [details] [diff] [review]
HTMLModElement

>+    GetAttr(kNameSpaceID_None, nsGkAtoms::datetime, aDateTime);

GetHTMLAttr, please.

r=me
Attachment #776789 - Flags: review?(peterv) → review+
Comment on attachment 776790 [details] [diff] [review]
HTMLHeadingElement

>+    GetAttr(kNameSpaceID_None, nsGkAtoms::align, aAlign);

GetHTMLAttr.

r=me
Attachment #776790 - Flags: review?(peterv) → review+
Attachment #782934 - Flags: review?(bzbarsky)
Comment on attachment 782934 [details] [diff] [review]
Remove nsIDOMHTMLLegendElement r=bz

The QI bits are a bit weird.  Can we not just use an interface map throughout if that's what we want, since now there is no interface table involved at all?

r=me with that fixed or a followup filed for better macros if we need them.
Attachment #782934 - Flags: review?(bzbarsky) → review+
Attachment #782985 - Flags: review?(bzbarsky)
Attachment #782987 - Flags: review?(bzbarsky)
Attachment #782988 - Flags: review?(bzbarsky)
Attachment #782989 - Flags: review?(bzbarsky)
Attachment #782990 - Flags: review?(bzbarsky)
Comment on attachment 782985 [details] [diff] [review]
Remove nsIDOMHTMLFontElement r=bz

r=me, again modulo the QI weirdness.
Attachment #782985 - Flags: review?(bzbarsky) → review+
Comment on attachment 782987 [details] [diff] [review]
Remove nsIDOMHTMLLabelElement r=bz

r=me with the QI caveat.
Attachment #782987 - Flags: review?(bzbarsky) → review+
Comment on attachment 782988 [details] [diff] [review]
Remove nsIDOMHTMLParamElement r=bz

r=me
Attachment #782988 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #34)
> Comment on attachment 782934 [details] [diff] [review]
> Remove nsIDOMHTMLLegendElement r=bz
> 
> The QI bits are a bit weird.  Can we not just use an interface map
> throughout if that's what we want, since now there is no interface table
> involved at all?
> 
> r=me with that fixed or a followup filed for better macros if we need them.

Can we have nsGenericHTMLElement's QI do what NS_HTML_CONTENT_INTERFACES does and then get rid of the nsISupports declarations/implementations on these classes?
Comment on attachment 782989 [details] [diff] [review]
Remove nsIDOMHTMLMeterElement r=bz

Same thing with QI.

Please file a followup to make SetDoubleAttr take an ErrorResult out param?

>   // NOTE: if we were allowed to static_cast to HTMLMeterElement we would be
>   // able to use nicer getters...

That comment can go away now, right?

r=me
Attachment #782989 - Flags: review?(bzbarsky) → review+
> Can we have nsGenericHTMLElement's QI do what NS_HTML_CONTENT_INTERFACES does 

The problem is that NS_HTML_CONTENT_INTERFACES needs to be able to cast the object to nsIDOMHTMLElement* to call nsGenericHTMLElement::DOMQueryInterface.

If we got rid of all these nsIDOMHTMLElement subclasses, we could just implement nsIDOMHTMLElement and company on nsGenericHTMLElement, and things would just work...
Comment on attachment 782990 [details] [diff] [review]
Remove nsIDOMHTMLDListElement r=bz

r=me

Note that this is an example of how to avoid having an interface table for an element class if you wanted one.  ;)
Attachment #782990 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #45)
> > Can we have nsGenericHTMLElement's QI do what NS_HTML_CONTENT_INTERFACES does 
> 
> The problem is that NS_HTML_CONTENT_INTERFACES needs to be able to cast the
> object to nsIDOMHTMLElement* to call nsGenericHTMLElement::DOMQueryInterface.
> 
> If we got rid of all these nsIDOMHTMLElement subclasses, we could just
> implement nsIDOMHTMLElement and company on nsGenericHTMLElement, and things
> would just work...

Oh, right.  We can just make the nsIDOMHTMLFooElement inherit from nsISupports and put nsIDOMHTMLElement on nsGenericHTMLElement.
We could, at a binary compat hit, yes.
OK, I will change the QI implementations to use maps and we can figure out what we want to do in a followup.
(In reply to Boris Zbarsky (:bz) from comment #45)
> > Can we have nsGenericHTMLElement's QI do what NS_HTML_CONTENT_INTERFACES does 
> 
> The problem is that NS_HTML_CONTENT_INTERFACES needs to be able to cast the
> object to nsIDOMHTMLElement* to call nsGenericHTMLElement::DOMQueryInterface.

Hmm, or we could use AsDOMNode()...
That's an extra virtual function call, right?
Yes, but you're QIing to nsIDOMHTMLElement, so you're getting a lot of virtual calls anyway if you want to do anything with it. I don't feel strongly about it, but I thought it was worth considering.
(In reply to Boris Zbarsky (:bz) from comment #48)
> We could, at a binary compat hit, yes.

Authors will notice this because their code will no longer compile, and the solution is to stick in a QI call which is pretty easy.  It will only break extensions that are no longer being updated.
> Yes, but you're QIing to nsIDOMHTMLElement

True.  This used to be more of an issue when we had bindings code doing that...

> Authors will notice this because their code will no longer compile

That's true.  OK, I'm happy to try giving it a shot, now that the xpidl inheritance hierarchy doesn't affect JS proto chains.  ;)
Depends on: 895873
Comment on attachment 775758 [details] [diff] [review]
Clean up nsFormControlFrame

This is mostly fixed in bug 899931.
Attachment #775758 - Attachment is obsolete: true
Depends on: 900839
Depends on: 903106
Attached patch Remove nsIDOMHTMLDivElement r=bz (obsolete) — Splinter Review
Attachment #792453 - Flags: review?(bzbarsky)
Attachment #792454 - Flags: review?(bzbarsky)
Attachment #792455 - Flags: review?(bzbarsky)
Attachment #792456 - Flags: review?(bzbarsky)
Comment on attachment 792451 [details] [diff] [review]
Don't use nsIDOMHTMLFoo interfaces in metro r=bz

This seems fine to me, but needs review from someone who knows whether all the relevant code is running in a window scope.
Attachment #792451 - Flags: review?(bzbarsky) → feedback+
Comment on attachment 792452 [details] [diff] [review]
Don't use nsIDOMHTMLFoo interfaces in mobile/ r=bz

Again, seems fine but needs review from someone familiar with this code.
Attachment #792452 - Flags: review?(bzbarsky) → feedback+
Comment on attachment 792453 [details] [diff] [review]
Remove nsIDOMHTMLDivElement r=bz

Given the shim bit, why did we need the earlier removals?

What's the reason for the test changes here?
Comment on attachment 792454 [details] [diff] [review]
Remove nsIDOMHTMLParagraphElement r=bz

Have the getter take a DOMString?

r=me with that
Attachment #792454 - Flags: review?(bzbarsky) → review+
Comment on attachment 792455 [details] [diff] [review]
Remove nsIDOMHTMLTableCellElement r=bz

r=me
Attachment #792455 - Flags: review?(bzbarsky) → review+
Comment on attachment 792456 [details] [diff] [review]
Remove nsIDOMHTMLAnchorElement r=bz

>+++ b/editor/libeditor/html/nsHTMLEditor.cpp
>+  nsRefPtr<HTMLAnchorElement> anchor = HTMLAnchorElement::FromContent(content);

FromContentOrNull, I think.

r=me with that
Attachment #792456 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #71)
> Comment on attachment 792453 [details] [diff] [review]
> Remove nsIDOMHTMLDivElement r=bz
> 
> Given the shim bit, why did we need the earlier removals?
> 

They're not necessary.  It's just nicer to not use the Ci stuff and it passes tests.

> What's the reason for the test changes here?

The test is making sure that we only add interfaces to the shim if they exist on the real Ci object, which is no longer the case.
(In reply to David Zbarsky (:dzbarsky) from comment #75)
> The test is making sure that we only add interfaces to the shim if they
> exist on the real Ci object, which is no longer the case.

What exact interface is present only on the shim? You should remove it from the shim instead.
Hmm, I think the shim isn't quite what we want here.  The shim is exposed to web content, so instead we want to define nsIDOMHTMLDivElement only on chrome globals, or something.
(In reply to David Zbarsky (:dzbarsky) from comment #77)
> Hmm, I think the shim isn't quite what we want here.  The shim is exposed to
> web content, so instead we want to define nsIDOMHTMLDivElement only on
> chrome globals, or something.

On globals? not on Ci?
If you want to expose nsIDOMHTMLDivElement on chrome, why are you going to remove the interface in the first place?
If you keep nsIDOMHTMLDivElement intact and remove it from the shim, it will be exposed only on the real Ci (a.k.a. chrome). What's wrong with that?
(In reply to Masatoshi Kimura [:emk] from comment #78)
> (In reply to David Zbarsky (:dzbarsky) from comment #77)
> > Hmm, I think the shim isn't quite what we want here.  The shim is exposed to
> > web content, so instead we want to define nsIDOMHTMLDivElement only on
> > chrome globals, or something.
> 
> On globals? not on Ci?

No I meant we want to define it on Ci, but only on chrome globals.

> If you want to expose nsIDOMHTMLDivElement on chrome, why are you going to
> remove the interface in the first place?
> If you keep nsIDOMHTMLDivElement intact and remove it from the shim, it will
> be exposed only on the real Ci (a.k.a. chrome). What's wrong with that?

We'd like to remove nsIDOMHTMLDivElement because it's not needed and lets us shrink elements by a pointer.  The only thing that needs to work is (element instanceof Ci.nsIDOMHTMLDivElement) because addons do that.  Something like the shim lets us effectively map that to (element instanceof HTMLDivElement) which still works.
Attachment #792453 - Attachment is obsolete: true
Attachment #792453 - Flags: review?(bzbarsky)
Attachment #794105 - Flags: review?(bzbarsky)
Comment on attachment 794105 [details] [diff] [review]
Remove nsIDOMHTMLDivElement r=bz

r=me.

I assume you'll make some of this other stuff chromeonly too, right?
Attachment #794105 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #81)
> Comment on attachment 794105 [details] [diff] [review]
> Remove nsIDOMHTMLDivElement r=bz
> 
> r=me.
> 
> I assume you'll make some of this other stuff chromeonly too, right?

Yeah, I'll move the entries for the rest of the elements to the end
Comment on attachment 794105 [details] [diff] [review]
Remove nsIDOMHTMLDivElement r=bz

::: dom/base/nsDOMClassInfo.cpp
> +  bool globalIsChrome = xpc::AccessCheck::isChrome(global);
>    for (uint32_t i = 0; i < ArrayLength(kInterfaceShimMap); ++i) {
>  
> +    // Skip ChromeOnly interface shim entries on content globals.
> +    if (kInterfaceShimMap[i].isChromeOnly && !globalIsChrome) {
> +      continue;
> +    }
> +

DefineComponentShim will not be even called on chrome at all. I don't understand why this helps for add-ons.
Bobby, am I missing something?
Flags: needinfo?(bobbyholley+bmo)
(In reply to Masatoshi Kimura [:emk] from comment #83)
> Comment on attachment 794105 [details] [diff] [review]
> Remove nsIDOMHTMLDivElement r=bz
> 
> ::: dom/base/nsDOMClassInfo.cpp
> > +  bool globalIsChrome = xpc::AccessCheck::isChrome(global);
> >    for (uint32_t i = 0; i < ArrayLength(kInterfaceShimMap); ++i) {
> >  
> > +    // Skip ChromeOnly interface shim entries on content globals.
> > +    if (kInterfaceShimMap[i].isChromeOnly && !globalIsChrome) {
> > +      continue;
> > +    }
> > +
> 
> DefineComponentShim will not be even called on chrome at all. I don't
> understand why this helps for add-ons.

Well, theoretically if chrome wanted to access someContentWindow.Components, it would get the Components shim (given how nsWindowSH::NewResolve works). But yes, putting something on the shim and not on the real Ci won't help addons that try to do "foopy instanceof Ci.nsIFoo".
Flags: needinfo?(bobbyholley+bmo)
Bobby, how/where do we set up the real Ci object?
Flags: needinfo?(bobbyholley+bmo)
(In reply to David Zbarsky (:dzbarsky) from comment #86)
> Bobby, how/where do we set up the real Ci object?

See this stuff:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#250
Flags: needinfo?(bobbyholley+bmo)
It seems that parts of this bug have been landing already, and I learned about them from an add-ons developer whose add-on was broken. Since we look for compatibility issues by milestone, for resolved bugs, this bug hasn't been on our radar.

Could bugs like this be split into dependencies so the various parts that land have their own milestone and compatibility flags? Is this something that needs to be discussed in the newsgroups to adjust the workflow?
Hmm.  We should in fact split this up, but which addon was broken and how?  We aimed to not remove anything that the addons mxr showed was being used....
(In reply to Boris Zbarsky [:bz] from comment #89)
> Hmm.  We should in fact split this up, but which addon was broken and how? 
> We aimed to not remove anything that the addons mxr showed was being used....

I am the add-on developer who brought this to Jorge's attention.  Unfortunately, the add-on that broke is not hosted on AMO (we created it for another company).  It contained an instanceof Components.interfaces.nsIDOMHTMLParamElement check.  

There are a lot of add-ons that are not on AMO ;)
I'm glad to hear that the MXR checks were done for these changes. That at least minimizes the potential impact. However, we do post a detailed update in the Add-ons Blog for every Firefox version, and we include changes in interfaces that don't necessarily show up in MXR queries.
Mark, sorry this caused problems for you guys.  :(

Jorge, in the future we'll make sure to resolve bugs that make changes like this and open new ones for continuing work!
Our add-on was broken too :( why was this defect opened ? what is the trigger for deleting all those nsIDOMHTMLFooElement ?

What do you guys suggest that I do in my code where I use some of the elements that were deleted (nsIDOMHTMLTableRowElement & nsIDOMHTMLLegendElement) ? 

Thanks in advance for your help !
> what is the trigger for deleting all those nsIDOMHTMLFooElement ?

It reduces the memory consumption of element classes, as well as reducing the on-disk and memory footprint of the binary (because of the removed code) and making the code simpler and hence more maintainable.

> What do you guys suggest that I do in my code where I use some of the elements that were
> deleted (nsIDOMHTMLTableRowElement & nsIDOMHTMLLegendElement) ? 

Is this C++ code or JS code?  How exactly were you using theseinterfaces?  Was it for instanceof tests like comment 90, or something else?
C++ code, and I use these interfaces to get information on the different DOM elements. 
For example, following is how I use nsIDOMHTMLTableRowElement (a code that doesn't compile anymore because I don't have the needed methods & interfaces anymore). Is there a workaround for this ?


nsCOMPtr<nsIDOMHTMLCollection> rows;
m_tableElem->GetRows(getter_AddRefs(rows)); // m_tableElem is of type nsIDOMHTMLTableElement	
for (unsigned int i = 0; i < rowsCount; i++)
{
	pRows->Item(i, getter_AddRefs(pNode));
	if (pNode == NULL)
		return E_FAIL;
	nsCOMPtr<nsIDOMHTMLTableRowElement> pRowElement(do_QueryInterface(pNode));
	if (FAILED(pRowElement->GetCells(getter_AddRefs(pCells))) || pCells==NULL)
		return E_FAIL;
	
	// Some logic here
}


Thanks a lot for this.
The C++ story is not great.  :(

I assume just QI to mozilla::dom::Element and then casting to HTMLTableRowElement won't work, because HTMLTableRowElement::Cells is not virtual and presumably not exported.

The simplest solution for this particular case on your side is probably to just walk through the kids the the Element and check for the ones that are IsHTML() and have the right localName ("td" or th").

But in general we need to figure what our C++ API story is here...
Flags: needinfo?(peterv)
I agree with you on that. But maybe until then you guys should stop working on this "defect" because it's causing C++ Add-on developers some serious headache :) !

PS Will C++ Add-ons become obsolete over time ?
I don't think there are any current plans to remove more of the nsIDOM* APIs for the moment.

> PS Will C++ Add-ons become obsolete over time ?

In the grand scheme of things, yes (e.g. they obviously can't work with Servo).
(In reply to Boris Zbarsky [:bz] from comment #96)
> But in general we need to figure what our C++ API story is here...

I was under the impression that the solution going forward for interacting with the DOM in addons was to do it in JS.
(In reply to Bobby Holley (:bholley) from comment #99)
> (In reply to Boris Zbarsky [:bz] from comment #96)
> > But in general we need to figure what our C++ API story is here...
> 
> I was under the impression that the solution going forward for interacting
> with the DOM in addons was to do it in JS.

What about existing add-ons ?
(In reply to George Haddad [:georgehdd] from comment #100)
> (In reply to Bobby Holley (:bholley) from comment #99)
> What about existing add-ons ?

They should hoist their DOM manipulation into JS (writing a JS-implementated component that touches the DOM, and using that component from C++). The DOM specs (and consequently our implementation of them) are just too volatile to expose a useful C++ API to third-party code.
Also, the DOM specs are more and more tied to JS.  There are some parts of them that can't even be expressed in C++ terms without involving the JS engine.
(In reply to Bobby Holley (:bholley) from comment #101)
> (In reply to George Haddad [:georgehdd] from comment #100)
> > (In reply to Bobby Holley (:bholley) from comment #99)
> > What about existing add-ons ?
> 
> They should hoist their DOM manipulation into JS (writing a JS-implementated
> component that touches the DOM, and using that component from C++). The DOM
> specs (and consequently our implementation of them) are just too volatile to
> expose a useful C++ API to third-party code.

Do you have an example of how to do that ?
This might get you started: https://developer.mozilla.org/en-US/docs/How_to_Build_an_XPCOM_Component_in_Javascript

The simplest in-tree example of a js-implemented XPCOM component is in the XPConnect test suite - see js/xpconnect/tests/components/js.
(In reply to Boris Zbarsky [:bz] from comment #96)
> The simplest solution for this particular case on your side is probably to
> just walk through the kids the the Element and check for the ones that are
> IsHTML() and have the right localName ("td" or th").
> 
> But in general we need to figure what our C++ API story is here...

I'm not sure there's much of a story here. We could add helper functions somewhere that emulate some of the member functions that we removed, but make them take an Element. That at least wouldn't cost us any performance, but I'm not too fond of it.
Flags: needinfo?(peterv)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.