1018658 - Move XPathNSResolver to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[(no longer active)]

Attachment: Part 1: Rename nsXPathNSResolver to mozilla::dom::XPathNSResolver

Comment 2

[(no longer active)]

Attachment: Part 2: Move XPathNSResolver to WebIDL

Comment 3

[(no longer active)]

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

Comment 4

[(no longer active)]

So it seems like document.evaluate()'s third argument can also be a function: <http://mxr.mozilla.org/mozilla-central/source/dom/xslt/tests/mochitest/test_bug566629.html?force=1#49>.  I'm not sure what we have in WebIDL which allows us to express this properly if anything, but using a callback is probably the wrong this to do since that would break the return type of createNSResolver...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what we have in WebIDL which allows us to express this properly if anything

Callback interfaces.  If a callback interface only has a single method on it, then a function is allowed to be used as the implementation of the callback interface.

That said, having a createNSResolver return a callback interface would be pretty weird.  So maybe in WebIDL what we really want are two separate interfaces: a callback interface and a non-callback interface.  createNSResolver would return the latter.  evaluate() would accept a union type that could be either one.

Comment 6

[(no longer active)]

I think I'll use a union of a callback function and one of these interfaces.

Comment 7

[(no longer active)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> I think I'll use a union of a callback function and one of these interfaces.

Anne, do you think this is worth raising a spec issue for?  The spec we claim to be implementing <http://www.w3.org/TR/2002/WD-DOM-Level-3-XPath-20020208/> is *ancient* for one thing, and it doesn't use WebIDL for the other.

Comment 8

[(no longer active)]

So, there is another issue that <http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_bug384003.xhtml> hits.  It seems like currently Attr, nsGenericDOMDataNode, FragmentOrElement and nsDocument all implement a tearoff nsIDOMXPathNSResolver.  The tearoff just forwards its work to nsINode <http://mxr.mozilla.org/mozilla-central/source/content/base/src/FragmentOrElement.cpp#441>, so I'm not sure what we're winning by this setup really, but I guess I would need to make Node implement XPathNSResolver as well?

Also, while I was looking around, WebKit/Blink seem to implement this as [NoInterfaceObject].  Should I do the same?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> so I'm not sure what we're winning by this setup really

Not having an extra vtable pointer on all those node classes.

> but I guess I would need to make Node implement XPathNSResolver as well

That seems like the simplest thing, yes.  I guess Attr, CharacterData, Element, DocumentFragment, and Document does cover all the node types...

Comment 10

[Anne (:annevk)]

Ehsan, Ms2ger and I maintain http://wiki.whatwg.org/wiki/DOM_XPath We haven't really figured out yet whether this is worth specifying or something that we can maybe eventually kill off.

Comment 11

[Anne (:annevk)]

bz, you forgot DocumentType.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, good point.  So we can implement on nsINode and only expose in IDL on the things it's currently exposed on.

Comment 13

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > so I'm not sure what we're winning by this setup really
> 
> Not having an extra vtable pointer on all those node classes.

Ah, of course!

> > but I guess I would need to make Node implement XPathNSResolver as well
> 
> That seems like the simplest thing, yes.  I guess Attr, CharacterData,
> Element, DocumentFragment, and Document does cover all the node types...

(In reply to Boris Zbarsky [:bz] from comment #12)
> Ah, good point.  So we can implement on nsINode and only expose in IDL on
> the things it's currently exposed on.

That would make for one annoying union.  ;-)  But OK.

Resetting the needinfo? flag for the last question in comment 8.

Comment 14

[(no longer active)]

(In reply to Anne (:annevk) from comment #10)
> Ehsan, Ms2ger and I maintain http://wiki.whatwg.org/wiki/DOM_XPath We
> haven't really figured out yet whether this is worth specifying or something
> that we can maybe eventually kill off.

Thanks.  I'll edit the wiki page once I come up with the IDL in this patch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> That would make for one annoying union.

Hmm.  Does it not work to have them all do "Document implements XPathNSResolverBuiltin" or whatnot and just use XPathNSResolverBuiltin in the union?  I don't recall whether we actually made that work or not.

Making this [NoInterfaceObject] totally makes sense to me given the "implements" stuff, yes.

Comment 16

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> > That would make for one annoying union.
> 
> Hmm.  Does it not work to have them all do "Document implements
> XPathNSResolverBuiltin" or whatnot and just use XPathNSResolverBuiltin in
> the union?  I don't recall whether we actually made that work or not.

What would XPathNSResolverBuiltin be?  Do you mean something like:

interface XPathNSResolverBuiltin {
  DOMString lookupNamespaceURI(DOMString prefix);
};

Document implements XPathNSResolverBuiltin;

[NoInterfaceObject]
interface XPathNSResolver {};
XPathNSResolver implements XPathNSResolverBuiltin;

or some such?

Comment 17

[(no longer active)]

(And with the above, would I not be able to just pass XPathNSResolverBuiltin directly to evaluate and just union it with the callback function?)

Comment 18

[Boris Zbarsky [:bzbarsky]]

XPathNSResolverBuiltin would be whatever the non-callback interface is.  If you're calling it XPathNSResolver and calling the callback something else, that's fine too.

So basically I'm thinking:

[NoInterfaceObject]
interface X {
  DOMString lookupNamespaceURI(DOMString prefix);
};

Document implements X;

callback interface Y {
  DOMString lookupNamespaceURI(DOMString prefix);
};

and then take (X or Y) as argument.

Comment 19

[(no longer active)]

OK, makes sense.  Thanks!

Comment 20

[Anne (:annevk)]

Note that lookupNamespaceURI is already on Node and defined by DOM.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8432166 [details] [diff] [review]
Part 1: Rename nsXPathNSResolver to mozilla::dom::XPathNSResolver

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8432167 [details] [diff] [review]
Part 2: Move XPathNSResolver to WebIDL

r=me

Comment 23

[Peter Van der Beken [:peterv]]

Hmm, I already have patches for this (see https://etherpad.mozilla.org/classinfo).

Comment 24

[Peter Van der Beken [:peterv]]

And I've just made createNSResolver return Node, and made XPathNSResolver a callback interface and removed the native class completely. I think it's what we should do if we can get a

Comment 25

[Peter Van der Beken [:peterv]]

...away with it.

Comment 26

[(no longer active)]

Oh, sorry for stepping on your toes, I just finished implementing comment 18... :(

Should I stop?

Comment 27

[(no longer active)]

(Will post my WIP patches so far for reference...)

Comment 28

[(no longer active)]

Attachment: Part 3: Make XPathNSResolver wrapper-cached; r=bzbarsky

Comment 29

[(no longer active)]

Attachment: Part 4: Make document.evaluate also accept user defined objects

Comment 30

[Boris Zbarsky [:bzbarsky]]

Peter, this and boxobject are the only remaining quickstubs consumers, and boxobject is on the chopping block.  Are you willing to at least post your patches here or something?

Comment 31

[Peter Van der Beken [:peterv]]

I'm splitting up my patch, will attach soonish (though I need to finish bug 787070 first).

Comment 32

[Peter Van der Beken [:peterv]]

Attachment: Support passing a node to CreateExpression v1

Comment 33

[Peter Van der Beken [:peterv]]

Attachment: Switch XPathNSResolver to WebIDL v1

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIDOMXPathEvaluator::createNSResolver v1

Comment 35

[Peter Van der Beken [:peterv]]

Attachment: Remove nsIDOMXPathNSResolver v1

Comment 36

[Boris Zbarsky [:bzbarsky]]

Peter, are those ready for review?

Comment 37

[Peter Van der Beken [:peterv]]

No.

Comment 38

[Peter Van der Beken [:peterv]]

Attachment: Switch XPathNSResolver to WebIDL v1.1

Comment 39

[Peter Van der Beken [:peterv]]

Attachment: Switch XPathNSResolver to WebIDL v1.2

The controversial bit is changing createNSResolver() to return Node instead of XPathNSResolver. The problem would be if people start relying on that to actually return a Node and use it as such, and other browsers keep returning an object that just has lookupNamespaceURI() then the code will work in Firefox but not other browsers. All uses of createNSResolver() that I've seen just pass the result to evaluate() or createExpression(), but that could just be because currently there's really not much else you could do with it :-).

Note that the overhead of the XPathNSResolver as CallbackInterface is not ideal, since if you pass in a node it'll go through the node's lookupNamespaceURI JS method. I'm not sure it matters much, most expressions probably only use very few prefixes. We could of course make everything that accepts an XPathNSResolver accept (XPathNSResolver or Node) instead. Let me know if you'd prefer that.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8502392 [details] [diff] [review]
Support passing a node to CreateExpression v1

r=me

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8508889 [details] [diff] [review]
Switch XPathNSResolver to WebIDL v1.2

> The controversial bit is changing createNSResolver() to return Node 

This is just because we don't want to create a random class with one single method that forwards to a Node?

I'm probably OK either way here.  We really want a spec for this stuff... :(

> We could of course make everything that accepts an XPathNSResolver accept
> (XPathNSResolver or Node) instead.

That's observably different in the case when someone fiddles with Node.prototype.lookupNamespaceURI, right?

I'm fine with the uniform callback thing for now.

Comment 42

[Anne (:annevk)]

(In reply to Boris Zbarsky [:bz] from comment #41)
> We really want a spec for this stuff... :(

Please keep https://wiki.whatwg.org/wiki/DOM_XPath updated. It's a bit unclear to me if all browsers support this API. If they do, I suppose I could add a "wish it were obsolete" section to DOM to document it.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8502394 [details] [diff] [review]
Remove nsIDOMXPathEvaluator::createNSResolver v1

r=me

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8502395 [details] [diff] [review]
Remove nsIDOMXPathNSResolver v1

r=me.  Nice to see that tearoff finally go!

Comment 45

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9aa9e2f4dfb
https://hg.mozilla.org/integration/mozilla-inbound/rev/afa1dbbce833
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fd115c18d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ab3cd59563

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9aa9e2f4dfb
https://hg.mozilla.org/mozilla-central/rev/afa1dbbce833
https://hg.mozilla.org/mozilla-central/rev/f7fd115c18d7
https://hg.mozilla.org/mozilla-central/rev/c1ab3cd59563