816343 - Get rid of nsIDOMNodeSelector once all elements and documents are on WebIDL bindings

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The only reason we need it at this point is to make Xrays for querySelector/querySelectorAll work.  So once all this stuff is using WebIDL for its Xrays, we can nuke it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove nsIDOMNodeSelector.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 764969 [details] [diff] [review]
Remove nsIDOMNodeSelector.

I'm not surprised if this breaks some binary addon.
I wonder if we'll need to add the methods to *Element/DocumentFragment/Document.idl, or add virtual versions of the methods to nsINode.
Perhaps an Aurora cycle is enough to get feedback and add the stuff only if needed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I'm not surprised if this breaks some binary addon.

Hrm.... Yes, that's possible.  :(

I don't really see a good way to check for that, sadly.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/74a1d7bd74dc

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74a1d7bd74dc

Comment 6

[Jacek Caban]

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #3)
> I don't really see a good way to check for that, sadly.

It will (did) break Wine, which uses Gecko embedding for its MSHTML implementation. I'm happy to provide a patch adding those methods to *Element/DocumentFragment/Document.idl, if you think that could be committed. Otherwise I will go with a local change in our fork.

BTW, I'm hearing mixed things about future of XPCOM bindings in general. Are there any reliable plans about if/when they will be removed?

Comment 7

[Boris Zbarsky [:bzbarsky]]

We should fix, that, then.

Would a virtual method on nsINode be ok for your purposes, or do you need it on an IDL-declared XPCOM interface?  Seems like having it on nsINode would be simpler to use than having it on three separate interfaces...

As far as XPCOM bindings for the DOM in general, we would like to remove them when we can, since at this point the web doesn't use them and they cause a nontrivial amount of memory overhead because of all the vtable pointers, not to mention the extra code complexity.  What we don't have right now is a reliable replacement for C++ consumers, not least because C++ can't do the "interface flattening" thing that JS can do.  We need to figure out what the plan is there, but you should assume that nsIDOMNode/nsIDOMElement, say will stick around for at least a year, while things like nsIDOMHTMLPreElement might go away sooner...

Comment 8

[Jacek Caban]

Attachment: Add querySelector* to *Element/DocumentFragment/Document.idl

Comment 9

[Jacek Caban]

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #7)
> We should fix, that, then.
> 
> Would a virtual method on nsINode be ok for your purposes, or do you need it
> on an IDL-declared XPCOM interface?  Seems like having it on nsINode would
> be simpler to use than having it on three separate interfaces...

Our situation is complicated by the fact that Wine has very high requirement for compatibility, so our code is in C, not C++. That makes using things like nsINode very ugly (although not impossible), while COM-like objects are easy to use. For our current needs, adding it to nsIDOMDocument would be enough. The attached patch adds it to nsIDOMElement and nsIDOMDocumentFragment as well, as suggested by Olli, which may be usefull for binary addons.

> As far as XPCOM bindings for the DOM in general, we would like to remove
> them when we can, since at this point the web doesn't use them and they
> cause a nontrivial amount of memory overhead because of all the vtable
> pointers, not to mention the extra code complexity. What we don't have
> right now is a reliable replacement for C++ consumers, not least because C++
> can't do the "interface flattening" thing that JS can do.  We need to figure
> out what the plan is there, but you should assume that
> nsIDOMNode/nsIDOMElement, say will stick around for at least a year, while
> things like nsIDOMHTMLPreElement might go away sooner...

That's something I'm very interested in and I meant to explore that for a while. Removing interfaces will hit us badly. While we may add some bindings in our fork, that's not going to work well. I've been thinking about exploring possibility of creating vtbl-based bindings off webidl (C++ bindings for C++ code...).I haven't investigated it yet, but it seems to me that it should be doable. Something like separated wrappers that would be entrely generated from webidl and would forward calls from simple virtual-like wrappers to the actual code should be possible. They would be allocated as separated objects, so there would be no memory overhead as long as they are not used by non-embedder code. Also, since they would be auogenerated, maintenance cost would be low.

That's probably wrong place to discuss it. At this point I may just hope to do some work on this soon, but I'd like to know if something like that could be considered to be merged to m-c.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> That makes using things like nsINode very ugly (although not impossible), while COM-like
> objects are easy to use.

Hmm.  How so?  On the C++ side, nsINode is just as COM-like as you want, no?

The idea of generating C++ bindings seems fairly plausible to me, yes.  There are some complications in terms of the types used to represent things....

Comment 11

[Jacek Caban]

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #10)
> > That makes using things like nsINode very ugly (although not impossible), while COM-like
> > objects are easy to use.
> 
> Hmm.  How so?  On the C++ side, nsINode is just as COM-like as you want, no?

It's much harder to extract vtbl layout to express that in C, nsINode uses multiplie inheritance (although not affecting vtbl AFAICS) and has many internal types. Its C declaration won't be pretty, but obviously possible. Anyway, that's our problem and I can't expect Mozilla devs to care about that, so I fully understand if you don't want my patch in. We will reconsider using nsINode on our side (which may be good idea after all).

> The idea of generating C++ bindings seems fairly plausible to me, yes. 
> There are some complications in terms of the types used to represent
> things....

Good, I will prepare better proposal for further discussion after I learn more about WebIDL.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 771335 [details] [diff] [review]
Add querySelector* to *Element/DocumentFragment/Document.idl

Ah, I see.  The issue is with expressing nsINode as a whole as a C struct, ok.  I agree that this would be fragile at best.

>+++ b/content/base/public/Element.h
>+NS_IMETHOD QuerySelector(const nsAString& aSelector, nsIDOMElement **aReturn) MOZ_FINAL \
>+{                                                                             \
>+  mozilla::ErrorResult rv;                                                    \
>+  mozilla::dom::Element * result = nsINode::QuerySelector(aSelector, rv);     \
>+  return result ? CallQueryInterface(result, aReturn) : rv.ErrorCode();       \

This will crash with a null-deref if a valid selector that does not match any nodes is passed in.  You want something more like:

  mozilla::ErrorResult rv;
  mozilla::dom::Element* result = nsINode::QuerySelector(aSelector, rv);
  if (rv.aFailed() {
    return rv.ErrorCode();
  }
  nsCOMPtr<nsIDOMElement> elt = do_QueryInterface(result);
  elt.forget(aReturn);
  return NS_OK;

and similar for the DocumentFragment and nsDocument cases.

It might make sense to put methods with the XPCOM signatures for this on nsINode and have the subclasses just call them instead of having several copy/pasted versions of the above.

r=me with this issue fixed.

Comment 13

[Jacek Caban]

Thanks for the review, I pushed a fixed version to try:

https://tbpl.mozilla.org/?tree=Try&rev=efca40421a13

Comment 14

[Boris Zbarsky [:bzbarsky]]

That has the same problem as the original patch: it's perfectly valid for QuerySelector to return a null result and have a success rv at the same time.  So just checking rv is not enough to know that CallQueryInterface is safe to call.  You need to either null-check result or use a null-safe QI method like do_QueryInterface.

Comment 15

[Jacek Caban]

Oh, sorry for stupid mistake. I pushed a fixed version:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f521b97fa75f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f521b97fa75f