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

RESOLVED FIXED in mozilla25

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Created attachment 764969 [details] [diff] [review]
Remove nsIDOMNodeSelector.
Attachment #764969 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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.
Attachment #764969 - Flags: review?(bugs) → review+
> 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.
https://hg.mozilla.org/mozilla-central/rev/74a1d7bd74dc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Comment 6

5 years ago
(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?
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

5 years ago
Created attachment 771335 [details] [diff] [review]
Add querySelector* to *Element/DocumentFragment/Document.idl
Attachment #771335 - Flags: review?(bzbarsky)

Comment 9

5 years ago
(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.
> 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

5 years ago
(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 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.
Attachment #771335 - Flags: review?(bzbarsky) → review+

Comment 13

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

https://tbpl.mozilla.org/?tree=Try&rev=efca40421a13
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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f521b97fa75f
You need to log in before you can comment on or make changes to this bug.