Currently we support |var foo = new XPathEvaluator();| but the DOM XPath WD specifies "It is expected that the XPathEvaluator interface will be implemented on the same object which implements the Document interface in an implementation which supports the XPath DOM module.". I have a solution with a tear-off and dynamic QI and DOMCI which makes document an XPathEvaluator when the Transformiix module is installed. Patch coming up later today. Jst: do we want this for 1.0?

IMO we do want this for mozilla1.0, if we don't take this we'll force users of these standard API's to use non-standard mechanisms for accessing these API's.

I've created a cached tear-off for nsDocument which implements nsIDOMXPathEvaluator. The document creates this tear-off when it gets QI'ed to nsIDOMXPathEvaluator and we know that there is an implementation for the XPath DOM module. We detect that by querying for a ContractID when loading the content module. The tear-off forwards AddRef and Release to the document, and QI for interfaces other than nsIDOMXPathEvaluator too. When the document is destroyed the tear-off gets deleted. The tear-off creates and holds an nsIDOMXPathEvaluator and forwards calls to it. For the DOMCI, I do the same check for the implementation and add the IID for nsIDOMXPathEvaluator at the end if it's there. If it's not there, I just add two nsnull's to the end of the array instead of that IID and one nsnull. I think this approach is sound, though a bit strange. It does work, document.evaluate for example works and I saw no leaks. I would like to change the ContractID to something implementation-neutral (right now it uses the Transformiix one). Ideas for the name of the ContractID welcome. /me waits for jst to tell him that he's insane.

How does this work in Javascript, is the question. One would expect to just be able to call the evaluate method on document (because the level 3 getInterface method is not here yet, and even with this, I am not sure how it works with Javascript). Does the Javascript user have to explicitly call QuaryInterface?

No, you just do document.evaluate(...) or document.createExpression(...). getInterface won't work anyway since it returns a Node and XPathEvaluator doesn't inherit from Node.

Comment on attachment 75273 [details] [diff] [review] v1 r=rayw. It looks good to me. No major problems found. I only found one issue that affects behavior (responding to feature version). Any of these issues could be logged as seperate bugs and not block the checkin. Here follow my notes: >>>>>>> nsDOMClassInfo.cpp: 1. Assumes that GetClassInfo is called only after XPath module is registered. During an installation run it seems remotely possible it would not be true, but this doesn't seem like a big risk or problem if it does happen (because it will work the nest time). It might be good to make a note of it, unless you believe it cannot occur. 2. The name of the new macro would be really wrong if someone decides some document types shouldn't have XPaths. I might make it more obvious what it does and call it DOM_CLASSINFO_MAP_END_WITH_XPATH. 3. I usually put contractid's such as the one for the xpath-evaluators in the .idl file containing the primary interface, following the example set by others, and especially since it is used in multiple places, this might be the right thing to do. 1 and 3 occur in multiple files. >>>>>>> nsContentModule.cpp: gHaveXPathDOM should be in a header file since it is shared, or so I have been told. This applies to multiple files. >>>>>>> nsDocument.h You did not use nsCOMPtr for mXPathDocument (meaning you later had to explicitly delete). Presumably this is because it gave you some trouble. It might give less trouble if you made it of type nsIXPathEvaluator, since it is not clear why you need to keep the pointer to the fully-derived type. >>>>>>> nsDocument.cpp I believe you could use NS_DECL_ISUPPORTS_INHERITED (I believe it is intended for this circumstance) to declare QueryInterface, AddRef, Release, etc. >>>>>>> nsGenericElement.cpp When checking the version string, you should, I believe, also check for an empty string because according to the DOM spec, users may ask without a version number (and are quite likely to if there is only one version released so far).

We really need at least the one behavioral change, so here it is.

Comment on attachment 75759 [details] [diff] [review] v2 r=rayw. See previous comments.

>>>>>>>> nsDOMClassInfo.cpp: > > 1. Assumes that GetClassInfo is called only after XPath module is registered. > During an installation run it seems remotely possible it would not be true, > but this doesn't seem like a big risk or problem if it does happen (because > it will work the nest time). It might be good to make a note of it, unless > you believe it cannot occur. It seems extremely unlikely it could occur. > 2. The name of the new macro would be really wrong if someone decides some > document types shouldn't have XPaths. I might make it more obvious what it > does and call it DOM_CLASSINFO_MAP_END_WITH_XPATH. Agreed. DOM_CLASSINFO_DOCUMENT_WITH_XPATH_MAP_END seems better but is very long. Jst? > 3. I usually put contractid's such as the one for the xpath-evaluators in the > .idl file containing the primary interface, following the example set by > others, and especially since it is used in multiple places, this might be > the right thing to do. I haven't done this yet for two reasons. 1) I need to decide on the final contractID for an XPathEvaluator and 2) I had heard contractids should not be put in .idl files. Jst? > 1 and 3 occur in multiple files. > >>>>>>>> nsContentModule.cpp: > > gHaveXPathDOM should be in a header file since it is shared, or so I have > been told. This applies to multiple files. It is marked as extern so I think this should be ok. >>>>>>>> nsDocument.h > > You did not use nsCOMPtr for mXPathDocument (meaning you later had to > explicitly delete). Presumably this is because it gave you some trouble. > It might give less trouble if you made it of type nsIXPathEvaluator, since > it is not clear why you need to keep the pointer to the fully-derived > type. No way I can use an nsCOMPtr, since that would create a circular dependency between the document and the tear-off. Now, the ownership model is that the document keeps the evaluator alive as long as it (the document) is alive, and due to the forwarded AddRef and Release from the tear-off to the document, the document stays alive as long as someone wants a reference to the tear-off. >>>>>>>> nsDocument.cpp > > I believe you could use NS_DECL_ISUPPORTS_INHERITED (I believe it is intended > for this circumstance) to declare QueryInterface, AddRef, Release, etc. Hmm, I guess so. I avoided it because of the _INHERITED suffix, there's no inheritence involved here.

As I said before, for anything non-trivial, just get a superreview and land the thing first rather than debating too much right now and log bugs. All we are talking about is stylistic, not behavioral. [...] >Agreed. DOM_CLASSINFO_DOCUMENT_WITH_XPATH_MAP_END seems better but is very long. >Jst? You added back _DOCUMENT which was part of what I objected to (since it is not clear that all documents will implement xpath or all sources of xpaths will be documents) and made it too long again. My suggestion was one character longer than your original one. [...] >I haven't done this yet for two reasons. 1) I need to decide on the final >contractID for an XPathEvaluator and 2) I had heard contractids should not be >put in .idl files. Jst? ContractIDs are significantly higher-level than implementation but slightly-lower than interfaces (a collection of interfaces). This partially depends upon whether the final name has "transformix" in it or not -- i.e. whether it makes transformix-specific promises that another XPath DOM implementation would be likely to break. So you are probably right as you have defined this, although an .h file would be nice since it is repeated so many times. >> 1 and 3 occur in multiple files. >> >>>>>>>> nsContentModule.cpp: >> > gHaveXPathDOM should be in a header file since it is shared, or so I have >> been told. This applies to multiple files. > >It is marked as extern so I think this should be ok. I made the same argument once and was told adamantly that any extern should be in a header file. As you say, ask jst, his opinion may be different. >No way I can use an nsCOMPtr, since that would create a circular dependency Sorry, my mistake. I haven't done this style of aggregation before. Were that not the case the delete would have been more of a problem than stylistic. It looks correct as it is. >Hmm, I guess so. I avoided it because of the _INHERITED suffix, there's no inheritence involved here. As I looked, I saw two things that made me think it was appropriate: first, a comment that did not mention inheritance saying this is the way to declare those three methods, second, examples using it that were not inheriting. Perhaps the creator thought it would be mostly used for inheritance, just as you thought your new macro would be exclusively used on documents. I won't insist if you feel strongly.

I'd say go with the macro name DOM_CLASSINFO_MAP_END_WITH_XPATH, and as for where to put the contractID, keep that out of the nsIDOM file, we'll eventually want to freeze that file, and we don't want to freeze the contractID with the interface. As for gHaveXPathDOM, if there's a good non-public header file you can put it in, go for it, but if not, leave it as is. It's no big deal as long as the linker is happy. One thing we should check with this is that mozilla still functions if the xpath stuff is registerd but the component isn't availiable any more. I.e. install n' run an optimized build, exit, remove the dll where this xpath code lives and start mozilla again. As long as it runs n' window.document is accessable on a webpage we're ok. If not, we're in trouble.

Based upon a review of the code, the scenario that Johnny says to test is one that I believe will probably fail, so if it is important, then "we are in trouble".. gHaveXPathDOM is set acording to a simple check of the component registry, and the component registry does not remove components when they didappear (or did not when I last reviewed that code). Not only will the interface get erroneously added after it has disappeared, but also, the code which instandiates transformix, which looks harmless if gHaveXPathDOM is false, looks like it will throw an exception if it is true and the component cannot be created. I believe that there are other optional components in Mozilla that once they have been registered in the category manager or by contract id will cause Mozilla to fail if they are then removed. Is this really something we need? If so, depending upon how robust it needs to be, we may impact startup time trying to check for whether the module can really be activated before adding the interface. If we didn't insist on removing the interface, then it could be made a bit more robust -- just calls to the interface would raise exceptions.

I guess + NS_ENSURE_TRUE(evaluator, NS_ERROR_OUT_OF_MEMORY); should test for the error code better. If we know the exact nsresult we get for a removed component, we could unset gHaveXPathDOM. I don't think that we should be picky about interface flattening on a broken installation, though.

As I read the code, the first call to hasFeature("XPath") will erroneously tell the script that it has the functionality in question. Then, every time the code tries to access the features in question, there will be some kind of exception. Clearing the flag would make future calls to hasFeature("XPath) return false, but the flattened interface is still there so attempts to access parts of that would continue to fail. It is not clear to me that it is a good idea to have the document behave the first time it is retrieved from subsequent times in the same session (after the flag has been cleared so that hasFeature now returns false. You could make hasFeature("XPath") try to create the necessary component before reporting true. Arguably this only adds overhead to the case where it is being used., and make the error returned on failure to create the component (the first time) indistinguishable from the simple not found case, after logging a high-quality error message. This way, you get a more-accurate hasFeature response and indistinguishable results from one time to the next.

how about adding the contractID to http://lxr.mozilla.org/seamonkey/source/dom/public/nsDOMCID.h ? Now for some cool values of the ContractID, it ain't transformiix special, it's a DOM spec, so I'd go for something beneath @mozilla.org/dom. I would go directly for version ;3, or include the level as ?level=3. "@mozilla.org/dom/xpath-evaluator;3" or "@mozilla.org/dom/xpath-evaluator;1?level=3" macroname could something like NS_XPATH_EVALUATOR3_CONTRACTID

With this one hasFeature always returns the right answer, the only minor annoyance is that the flattened interfaces will include nsIXPathEvaluator even when the module has been removed. I've also removed the interface from the DOMCI for nsXULDocument because it's the only one that doesn't inherit from nsDocument.

Comment on attachment 78053 [details] [diff] [review] v3 >Index: mozilla/content/base/src/nsDocument.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/base/src/nsDocument.cpp,v >retrieving revision 3.372 >diff -u -3 -r3.372 nsDocument.cpp >--- mozilla/content/base/src/nsDocument.cpp 5 Apr 2002 05:41:37 -0000 3.372 >+++ mozilla/content/base/src/nsDocument.cpp 6 Apr 2002 16:37:44 -0000 <...> >@@ -592,6 +626,22 @@ > NS_INTERFACE_MAP_ENTRY(nsIDOM3Node) > NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference) > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDocument) >+ if (aIID.Equals(NS_GET_IID(nsIDOMXPathEvaluator)) && >+ (!gCheckedForXPathDOM || gHaveXPathDOM)) { >+ if (!mXPathDocument) { shouldn't this be if (!gCheckedForXPathDOM && !mXPathDocument) { >+ nsCOMPtr<nsIDOMXPathEvaluator> evaluator; >+ evaluator = do_CreateInstance(NS_XPATH_EVALUATOR_CONTRACTID); <..> no if here >+ gCheckedForXPathDOM = PR_TRUE; >+ gHaveXPathDOM = (evaluator != nsnull); <..> >+ NS_ENSURE_TRUE(evaluator, NS_ERROR_OUT_OF_MEMORY); as this is a unsupported interface, we should return NS_NOINTERFACE, IMHO >+ mXPathDocument = new nsXPathDocumentTearoff(evaluator, this); >+ NS_ENSURE_TRUE(mXPathDocument, NS_ERROR_OUT_OF_MEMORY); >+ } >+ foundInterface = mXPathDocument; >+ } >+ else other than that, r=me. tested on solaris debug.

We actually don't need the extra check for !gCheckedForXPathDOM

Comment on attachment 79262 [details] [diff] [review] v4.1 r=axel@pike.org.

Comment on attachment 79262 [details] [diff] [review] v4.1 + if (!mXPathDocument) { + nsCOMPtr<nsIDOMXPathEvaluator> evaluator; + nsresult rv; + evaluator = do_CreateInstance(NS_XPATH_EVALUATOR_CONTRACTID, &rv); + gCheckedForXPathDOM = PR_TRUE; + gHaveXPathDOM = (evaluator != nsnull); + if (rv == NS_ERROR_FACTORY_NOT_REGISTERED) { + return NS_ERROR_NO_INTERFACE; + } + NS_ENSURE_TRUE(evaluator, NS_ERROR_OUT_OF_MEMORY); Replace the above NS_ENSURE_TRUE(...) with NS_ENSURE_SUCCESS(rv, rv) to properly propagate all errors to the callers. sr=jst with that change.

Checked in on the trunk.

Comment on attachment 79262 [details] [diff] [review] v4.1 a=asa (on behalf of drivers) for checkin to the 1.0 branch

taking

checked in

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword with verified1.0.0.

Hi, I download the patch. Where am i supposed to install it??? or what am i supposed to do with it??? Regards, Ana