Closed Bug 1018658 Opened 7 years ago Closed 7 years ago

Move XPathNSResolver to WebIDL

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ehsan.akhgari, Assigned: peterv)

References

Details

Attachments

(4 files, 6 obsolete files)

No description provided.
Assignee: nobody → ehsan
Attachment #8432166 - Flags: review?(bzbarsky)
Attachment #8432167 - Flags: review?(bzbarsky)
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...
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
I think I'll use a union of a callback function and one of these interfaces.
(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.
Flags: needinfo?(annevk)
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?
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(annevk)
bz, you forgot DocumentType.
Ah, good point.  So we can implement on nsINode and only expose in IDL on the things it's currently exposed on.
(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.
Flags: needinfo?(bzbarsky)
(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.
> 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.
Flags: needinfo?(bzbarsky)
(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?
Flags: needinfo?(bzbarsky)
(And with the above, would I not be able to just pass XPathNSResolverBuiltin directly to evaluate and just union it with the callback function?)
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.
Flags: needinfo?(bzbarsky)
OK, makes sense.  Thanks!
Note that lookupNamespaceURI is already on Node and defined by DOM.
Comment on attachment 8432166 [details] [diff] [review]
Part 1: Rename nsXPathNSResolver to mozilla::dom::XPathNSResolver

r=me
Attachment #8432166 - Flags: review?(bzbarsky) → review+
Comment on attachment 8432167 [details] [diff] [review]
Part 2: Move XPathNSResolver to WebIDL

r=me
Attachment #8432167 - Flags: review?(bzbarsky) → review+
Depends on: 1021670
Hmm, I already have patches for this (see https://etherpad.mozilla.org/classinfo).
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
...away with it.
Oh, sorry for stepping on your toes, I just finished implementing comment 18... :(

Should I stop?
Flags: needinfo?(peterv)
(Will post my WIP patches so far for reference...)
Assignee: ehsan → nobody
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?
I'm splitting up my patch, will attach soonish (though I need to finish bug 787070 first).
Flags: needinfo?(peterv)
Assignee: nobody → peterv
Attachment #8432166 - Attachment is obsolete: true
Attachment #8432167 - Attachment is obsolete: true
Attachment #8435791 - Attachment is obsolete: true
Attachment #8435792 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Peter, are those ready for review?
Flags: needinfo?(peterv)
No.
Flags: needinfo?(peterv)
Attachment #8502393 - Attachment is obsolete: true
Attachment #8502392 - Flags: review?(bzbarsky)
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.
Attachment #8508098 - Attachment is obsolete: true
Attachment #8508889 - Flags: review?(bzbarsky)
Comment on attachment 8502392 [details] [diff] [review]
Support passing a node to CreateExpression v1

r=me
Attachment #8502392 - Flags: review?(bzbarsky) → review+
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.
Attachment #8508889 - Flags: review?(bzbarsky) → review+
(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.
Attachment #8502394 - Flags: review?(bzbarsky)
Attachment #8502395 - Flags: review?(bzbarsky)
Comment on attachment 8502394 [details] [diff] [review]
Remove nsIDOMXPathEvaluator::createNSResolver v1

r=me
Attachment #8502394 - Flags: review?(bzbarsky) → review+
Comment on attachment 8502395 [details] [diff] [review]
Remove nsIDOMXPathNSResolver v1

r=me.  Nice to see that tearoff finally go!
Attachment #8502395 - Flags: review?(bzbarsky) → review+
Depends on: 1089049
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.