321170 - Templates needs to be able to load datasources other than RDF

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

The new template code (bug 285631) supports other query languages besides RDF but it doesn't have any means of loading other data besides RDF. This needs to be smart and pick the right datasource. That is datasources="..." needs to allow non-RDF sources.

I have done some work here but it only works for RDF and XML.

Comment 1

[lekma]

(In reply to comment #0)
> 
> I have done some work here but it only works for RDF and XML.
> 

Could we have a look at the xml one?

Cause i'd like to try and start to write my own query processor (it would be for an application server, see www.gnuenterprise.org), and it would help if i had some example other than the rdf one.

thanks in advance

Comment 2

[Laurent Jouanneau]

I think that the parsing of datasources attribute should be let to the query processor. So the query processor could verify and the datasource can be everything it want.

So the nsIXULTemplateQueryProcessor interface could be have a new method :

  nsISupports getDatasources(in AString aDatasources);

Then, the XULTemplateBuilder call this method when it needed and the result is stored in its mDatasources member (added by the patch of bug 321171).

Comment 3

[Laurent Jouanneau]

sorry: 
s/could verify/could verify and parse the content of the attribute
s/could be have/could have

Comment 4

[Neil Deakin]

This is a possibility but the query processor should not be parsing the entire string. It should instead receive each uri in the attribute in turn after the template builder has vetted the uri for access. Also, in this case, a reload type of method would be needed.

Comment 5

[Laurent Jouanneau]

After a discussion on irc with Neil, I will suggest a patch with this following solution.

I will add this method on query processor : nsISupports getDatasources( in nsIArray aDatasources, in nsIDOMElement aRoot).

The builder will parse the datasources attribute, check security on uri given in this attribute, and then create a nsIArray object which contains nsIUri objects corresponding to the uris. Then it will call getDatasources method of the query processor with this array and with the root element of the template, so the query processor can read all attributes it wants for additional datas.

Comment 6

[Laurent Jouanneau]

Attachment: preview of the future patch

This a "preview" patch. It compiles but it doesn't work (Firefox crashes at startup and I don't know why for the moment, I will work on it later). However, it gives an idea on the future implementation.

I changed the signature of the getDatasource method : 

  nsISupports getDatasource(in nsIArray aDatasources,
                            in nsIDOMNode aRootNode,
                            in boolean aIsTrusted,
                            in nsIDOMEventListener aListener,
                            out boolean aShouldDelayBuilding);

This new parameters are needed by the query processor for XML, Storage and/or RDF.

For details, read the description in the patch on the IDL. Comments are welcomed.

Comment 7

[Laurent Jouanneau]

I forgot to say that some parts of this patch provide from the patch for bug 321171 (https://bugzilla.mozilla.org/attachment.cgi?id=263858&action=edit) ;-)

Comment 8

[Neil Deakin]

The listener argument should be removed. The XML Query Processor should handle this itself.

Comment 9

[Laurent Jouanneau]

Since the XML Query Processor couldn't access to the builder, I don't see how the builder can rebuild content after the loading of the xml document. I prepare a new patch for bug 321171 : I moved all related code of XML query processor, from the builder to a new getDatasource method in your XML query processor. We shouldn't keep specific code for the XML query processor in the builder, because any other query processor may have the same need (doing asynchronous loading).

Comment 10

[Neil Deakin]

It should pass the builder to the function then. The event listener is very XML datasource specific.

Comment 11

[Laurent Jouanneau]

Attachment: patch v1

Here is a new version of the patch. It works now and I replaced the event listener parameter by the builder.

Comment 12

[Neil Deakin]

Comment on attachment 266901 [details] [diff] [review]
patch v1

I can't really review all of this, since I wrote the parts from bug 321171 and this seems to include some of that code but not all of it. It would be best if that bug was checked in first. I'll be mostly offline though for a few days so if there's a need for this soon, feel free to harass some people ;)

Some comments:

----

Index: content/xul/templates/public/nsIXULTemplateQueryProcessor.idl
 
 #include "domstubs.idl"
 #include "nsISupports.idl"
+#include "nsIArray.idl"

This can just be done with:

interface nsIArray

+
+  /**
+   * Return on object corresponding to the datasources attribute.
+   * For example, for XML it can be a nsIDOMNode. This object will be given to
+   * other methods of the query processor.

Some better text:

Retrieve the datasource to use for the query processor. The list of datasources in a template 
is specified using the datasources attribute as a space separated list of URIs. This list is
processed by the builder and supplied to the query processor in the aDataSources array as
a list of nsIURI objects. This method may return a object corresponding to these URIs and
the builder will supply this object to other query processor methods. For example, for an
XML source, the datasource might be an nsIDOMNode.

+   * All these uri are checked by the 
+   * builder so it is safe to use them.
+   * If the query processor needs to load a document which is the datasource,
+   * it can use the given builder as an event listener on this document so the builder
+   * know when it can rebuild the content. In this case, the query processor should 
+   * set the aShouldDelayBuilding returned parameter to true, otherwise set it to false.

The query processor itself should be the event listener. We can use the cycle collector to
avoid reference issues.

+   * 
+   * @param aDatasources  the list of nsIURI objects

Use aDataSources instead of aDatasources

+   * @param aRootNode     the root node the builder is attached to
+   * @param aIsTrusted    says if the template is in a trusted document
+   * @param aBuilder     a listener which can be attached to a datasource document 

Just say: the template builder
 
+   * @param aShouldDelayBuilding returned parameter to says if the builder should wait 
+   *                             to build the content or not

'which says'

+   * @return a datasource object
+   */

'@returns'

Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
+nsXULTemplateBuilder::LoadDataSources(nsIDocument* aDocument,
...
+
+    nsAutoString datasources;
+    mRoot->GetAttr(kNameSpaceID_None, nsGkAtoms::datasources, datasources);
+
+    nsAutoString querytype;
+    mRoot->GetAttr(kNameSpaceID_None, nsGkAtoms::querytype, querytype);
+
+ 
remove the extra blank line

+    // create the query processor. The querytype attribute on the root element
+    // may be used to create one of a specific type.
+  

You removed the code that handled '#someelementid' for XML. You could solve this
by parsing this here and allowing the datasources array to be nsIURIs or DOM nodes.

+nsresult
+nsXULTemplateBuilder::LoadDataSourceUrls(nsIDocument* aDocument,
+                                         const nsAString& aDataSources,
+                                         PRBool aIsRDFQuery,
+                                         PRBool* aShouldDelayBuilding)
+{
     // Grab the doc's principal...
-    nsIPrincipal *docPrincipal = doc->NodePrincipal();
-
+    nsIPrincipal *docPrincipal = aDocument->NodePrincipal();
+  
     NS_ASSERTION(docPrincipal == mRoot->NodePrincipal(),
                  "Principal mismatch?  Which one to use?");
+  
 
extra blank line here too

-
-        mCompDB->AddDataSource(ds);
+        nsCOMPtr<nsISupports> uriSupports = do_QueryInterface(uri);
+        urlList->AppendElement ( uriSupports , PR_FALSE );

Should be able to just append without the QI.

 }
 
+NS_IMETHODIMP
+nsXULTemplateBuilder::HandleEvent(nsIDOMEvent* aEvent)
+{
+    return Rebuild();
+}
+

This should be done by the XML query processor.

Index: content/xul/templates/src/nsXULTemplateQueryProcessorRDF.cpp
+nsXULTemplateQueryProcessorRDF::GetDatasource(nsIArray* aDatasources,
...
+    nsCOMPtr<nsISimpleEnumerator> enumerator;
+    rv = aDatasources->Enumerate(getter_AddRefs(enumerator));
+    if (!enumerator)
+        return NS_ERROR_UNEXPECTED;
+

Easier and faster to just iterate through the indices using a loop rather than creating an enumeration.

+    PRBool hasMoreElements;
+
+    enumerator->HasMoreElements(&hasMoreElements);
+
+    while(hasMoreElements){

add space before ( and after )
 
+
+        nsCOMPtr<nsISupports> item;
+        nsCOMPtr<nsIURI> uri;
+        enumerator->GetNext(getter_AddRefs(item));
+        uri = do_QueryInterface(item);
+        if(!uri)

add space before (


+            return NS_ERROR_UNEXPECTED;
+
+        enumerator->HasMoreElements(&hasMoreElements);
+
+        nsCOMPtr<nsIRDFDataSource> ds;
+        nsCAutoString uristrC;
+        uri->GetSpec(uristrC);
+
+        rv = gRDFService->GetDataSource(uristrC.get(), getter_AddRefs(ds));
+
+        if (NS_FAILED(rv)) {
+            // This is only a warning because the data source may not
+            // be accessible for any number of reasons, including
+            // security, a bad URL, etc.
+    #ifdef DEBUG
+            nsCAutoString msg;
+            msg.Append("unable to load datasource '");
+            msg.Append(uristrC);
+            msg.Append('\'');
+            NS_WARNING(msg.get());
+    #endif
+            continue;
+        }
+
+        compDB->AddDataSource(ds);
+    }
+
+
+    // check if we were given an inference engine type
+    nsAutoString infer;
+    nsCOMPtr<nsIRDFDataSource> db;
+    root->GetAttr(kNameSpaceID_None, nsGkAtoms::infer, infer);
+    if (!infer.IsEmpty()) {
+        nsCString inferContractID(NS_RDF_INFER_DATASOURCE_CONTRACTID_PREFIX);
+        AppendUTF16toUTF8(infer, inferContractID);
+        nsCOMPtr<nsIRDFInferDataSource> inferDB =
+            do_CreateInstance(inferContractID.get());
+
+        if (inferDB) {
+            inferDB->SetBaseDataSource(compDB);
+            db = do_QueryInterface(inferDB);
+        } else {
+            NS_WARNING("failed to construct inference engine specified on template");
+        }
+    }
+  

Instead of having two blocks of code to handle this inference stuff, we should just handle this entirely in the RDF query processor and return the inferdb. Then, mCompDB in the builder can be removed. This only time it is needed is for builder::GetDataSources which could just return the actual datasource (the declaration changed to return an nsISupports).

 NS_IMETHODIMP
 nsXULTemplateQueryProcessorRDF::InitializeForBuilding(nsISupports* aDatasource,

Comment 13

[Laurent Jouanneau]

Attachment: patch v2

new patch which fixes Neil's requests :

- updated the comment, remove blank lines and other syntaxic stuff
- the template builder is not implementing any more the nsIDOMEventListener interface
- removed mCompDB property. However, I don't know if it is a good idea because it is used in GetDatabase and in Refresh() methods  so I needed to duplicate the code which check if the datasource si a nsIRDFInferDataSource object or not.

>You removed the code that handled '#someelementid' for XML. You could solve
this by parsing this here and allowing the datasources array to be nsIURIs or DOM

In fact, I moved the code which handle the case of '#someelementid', into the XML query processor. In this query processor, I just get the relative url between  the given url and the document url, and if this is only a ref part, then it run the code which returns the DOMNode as a datasource.

Comment 14

[Laurent Jouanneau]

> You could solve this by parsing this here and allowing the datasources 
> array to be nsIURIs or DOM

This could be a good idea, but in this case, I don't know really how I could handle the case of the "profile" word in the storage query processor. I saw this cases  :

1) the user could set datasources="profile" : but it is not correct. The "profile" string could be a relative URL to the current url document. To support this case, we should insert a "hugly" hack into the template builder :-( (if(storage and datasources == "profile") then store a string into the list else  store a nsIURI.)

2) datasources="#profile" : it could be a solution, if the template builder does not try to resolve it by storing a DOM node into the datasources list (so your solution is not recommended in this case). The storage query processor just get the relative url between  the given url and the document url, and if it is equals "#profile", then it loads the "profile" database, else it tries to load the database located at the given url.

3) datasources="^profile" : if the template builder find a ^ at the begining  of a string, then it stores a nsISupportsString into the datasources list. So the list could have nsIURI, nsIDOMNode or nsISupportsString objects (Of course, the query processor should not try to perform a nsISupportsString value as an url). If the storage query processor find a nsISupportsString into the list, then it verifies that it is the "profile" word, and then loads the corresponding database.

Which solutions do you prefer ? Other ideas ?

Comment 15

[Neil Deakin]

What does 'profile' refer to? There are numerous database files in use currently, all stored in the profile.

The uri should still be a uri, like 'rdf:bookmarks' but with a different prefix.

Comment 16

[Laurent Jouanneau]

Attachment: patch v3

Here is a new version of the patch. The builder now check if given URIs are only reference to a node of the current document. If so, it puts a nsIDOMNode in the list instead of a nsIURI object. I made tests with an updated patch of the XML query processor and it works.

For storage query processor, I will follow Neil's suggestion and I will use a URIs like "storage:foo" (or "db:foo"...) for internal databases.

Comment 17

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

Could you upload a patch with a bit more context, at least
-u8 and with -p too, so -u8p

Comment 18

[Laurent Jouanneau]

Attachment: patch v3.1

New patch made with cvs diff -u8p

Comment 19

[Neil Deakin]

Comment on attachment 267948 [details] [diff] [review]
patch v3.1

Please wait until bug 321171 in reviewed and checked in before doing this. Both conflict with each other and it makes it difficult to work with.

Comment 20

[Neil Deakin]

Laurent, now that bug 321171 is done, could you make an updated patch for this and I'll review it?

Comment 21

[Laurent Jouanneau]

Ok, I'm going to update my patch.

Comment 22

[Laurent Jouanneau]

Attachment: patch v4

This is a new version of the patch after the landing of the patch for bug 321171. 

It fixes also a regression I found in the trunk : the value of aShouldDelayBuilding was kept to true even if the datasource is a RDF content. Consequence : the update of the "datasources" attribute with an other RDF content didn't cause a rebuild. The RDF query processor now sets aShouldDelayBuilding to FALSE, and now the default value of aShouldDelayBuilding is FALSE. I don't see a reason why it should be TRUE.

Comment 23

[Neil Deakin]

Comment on attachment 268607 [details] [diff] [review]
patch v4 

Index: content/xul/templates/public/nsIXULTemplateQueryProcessor.idl

+   *This list is processed by the builder and supplied to the query processor
+   * in the aDataSources array as a list of nsIURI objects.

It could also contain nsIDOMNode objects.

+   * All of these URIs are checked by the builder so it is safe to use them.
+   * If the query processor needs to load a document which is the datasource,
+   * it can use the given builder in an event listener on this document so the
+   * event listener can call the builder to rebuild the content. In this case,
+   * the query processor should set the aShouldDelayBuilding returned parameter
+   * to true, otherwise set it to false.

This last part isn't quite correct, the builder isn't used as an event listener now. Just say:

  If the query processor needs to load the datasource asynchronously, it
  may set the aShouldDelayBuilding returned parameter to true to delay building
  the template content, and call the builder's Rebuild method when the data is
  available.

Also, adjust the whole comment to fit into 80 columns, some people are finicky that way ;)

+   *
+   * @param aDataSources  the list of nsIURI objects
+   * @param aRootNode     the root node the builder is attached to
+   * @param aIsTrusted    says if the template is in a trusted document

s/says/true/

+   * @param aBuilder      a template builder which can be used in an event listener

change this to just: the template builder

Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
 
 NS_IMETHODIMP
 nsXULTemplateBuilder::Refresh()
 {
     nsresult rv;
-

The patch contains a number of whitespace only changes. Avoid making those changes.

 nsresult
 nsXULTemplateBuilder::LoadDataSourceUrls(nsIDocument* aDocument,
                                          const nsAString& aDataSources,
                                          PRBool aIsRDFQuery,
                                          PRBool* aShouldDelayBuilding)
 {
     // Grab the doc's principal...
     nsIPrincipal *docPrincipal = aDocument->NodePrincipal();
 
     NS_ASSERTION(docPrincipal == mRoot->NodePrincipal(),
                  "Principal mismatch?  Which one to use?");
 
     PRBool isTrusted = PR_FALSE;
     nsresult rv = IsSystemPrincipal(docPrincipal, &isTrusted);
     NS_ENSURE_SUCCESS(rv, rv);

@@ -1284,28 +1229,42 @@ nsXULTemplateBuilder::LoadDataSourceUrls
+            if (dsnode)
+                urlList->AppendElement ( dsnode , PR_FALSE );

No extra spaces around the parantheses. Also, it would be more correct if 'uriList'
was the variable name rather than 'urlList'.

+    nsCOMPtr<nsIDOMNode> rootNode = do_QueryInterface(mRoot);
+    rv = mQueryProcessor->GetDatasource(urlList,
+                                        rootNode,
+                                        isTrusted,
+                                        this,
+                                        aShouldDelayBuilding,
+                                        getter_AddRefs(mDataSource));
+    NS_ENSURE_SUCCESS(rv,rv);

Add a space after the comma here

+    if (aIsRDFQuery && mDataSource) {  
         // check if we were given an inference engine type
-        nsAutoString infer;
-        mRoot->GetAttr(kNameSpaceID_None, nsGkAtoms::infer, infer);
-        if (!infer.IsEmpty()) {
-            nsCString inferCID(NS_RDF_INFER_DATASOURCE_CONTRACTID_PREFIX);
-            AppendUTF16toUTF8(infer, inferCID);
-            nsCOMPtr<nsIRDFInferDataSource> inferDB =
-                do_CreateInstance(inferCID.get());
-
-            if (inferDB) {
-                inferDB->SetBaseDataSource(mCompDB);
-                mDB = do_QueryInterface(inferDB);
-            } else {
-                NS_WARNING("failed to construct inference engine specified on template");
-            }
-        }
-  
-        if (!mDB)
-            mDB = mCompDB;
-        mDataSource = mDB;
+        nsCOMPtr<nsIRDFInferDataSource> inferDB = do_QueryInterface(mDataSource);
+        if (inferDB) {
+            nsCOMPtr<nsIRDFDataSource> ds;
+            inferDB->GetBaseDataSource(getter_AddRefs(ds));
+            if (ds)
+                mCompDB = do_QueryInterface(ds);
+        }
+        
+        if(!mCompDB)
+            mCompDB = do_QueryInterface(mDataSource);
+    
+        mDB = do_QueryInterface(mDataSource);

File a followup bug on cleaning up mCompDB. The only time it is needed is in
GetDataSources and Refresh and those calls could just do the infer datasource
check then.

Index: content/xul/templates/src/nsXULTemplateQueryProcessorRDF.cpp
+NS_IMETHODIMP
+nsXULTemplateQueryProcessorRDF::GetDatasource(nsIArray* aDataSources,
+                                              nsIDOMNode* aRootNode,
+                                              PRBool aIsTrusted,
+                                              nsIXULTemplateBuilder* aBuilder,
+                                              PRBool * aShouldDelayBuilding,

No extra space before * here

+                                              nsISupports** _retval)
+{
+    nsCOMPtr<nsIRDFCompositeDataSource> compDB;
+    nsCOMPtr<nsIContent> root = do_QueryInterface(aRootNode);
+    nsresult rv;
+
+    *_retval = nsnull;
+    * aShouldDelayBuilding = PR_FALSE;
+
+    if(! root)
+        return NS_ERROR_UNEXPECTED;

NS_ENSURE_TRUE(root, NS_ERROR_UNEXPECTED)

+    PRUint32 length, index;
+    rv = aDataSources->GetLength(&length);
+    NS_ENSURE_SUCCESS(rv,rv);
+
+    for (index =0; index < length; index++) {

Add a space after =
 
Index: content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp
 NS_IMETHODIMP
+nsXULTemplateQueryProcessorXML::GetDatasource(nsIArray* aDataSources,
+    mTemplateBuilder = aBuilder;

+NS_IMETHODIMP
+nsXULTemplateQueryProcessorXML::HandleEvent(nsIDOMEvent* aEvent)
+{
+    NS_PRECONDITION (aEvent , "aEvent null");

No extra spaces here

+        // to avoid leak. we don't need it after...
+        mTemplateBuilder = nsnull;
+
+        return rv;
+    }else
+        return NS_OK;

Add braces around the else block too.

I think I filed a bug about adding cycle collection to the xml query processor.
This would remove the need for this mTemplateBuilder cycle issue, as I'm
concerned about the case when the load event never fires.

Index: content/xul/templates/src/nsXULTemplateQueryProcessorXML.h
 
+    // nsIDomEventListener interface
+    NS_DECL_NSIDOMEVENTLISTENER

The case is the comment is wrong here

Comment 24

[Laurent Jouanneau]

Attachment: patch v4.1

This is a new patch after Neil's comments. I will fill a followup bug on cleaning up mCompDB.

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 269865 [details] [diff] [review]
patch v4.1

>Index: content/xul/templates/public/nsIXULTemplateQueryProcessor.idl
>===================================================================

>+   * nsIURI objects or nsIDOMNode objects. This method may return a object

an object

>+   * @param aShouldDelayBuilding returned parameter which says if the builder
>+   *                             should wait to build the content or not

I think we usually do:

   * @param aShouldDelayBuilding [out] whether the builder...


>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
>===================================================================

>+        if(!mCompDB)

Space after if.

>+    if(!mDB && isTrusted) {

Ditto.

>+         gRDFService->GetDataSource("rdf:local-store", getter_AddRefs(mDB));

Weird indenting.

>Index: content/xul/templates/src/nsXULTemplateQueryProcessorRDF.cpp
>===================================================================

>@@ -210,16 +212,129 @@ nsXULTemplateQueryProcessorRDF::InitGlob
>     return MemoryElement::Init() ? NS_OK : NS_ERROR_FAILURE;
> }
> 
> //----------------------------------------------------------------------
> //
> // nsIXULTemplateQueryProcessor interface
> //
> 
>+NS_IMETHODIMP
>+nsXULTemplateQueryProcessorRDF::GetDatasource(nsIArray* aDataSources,
>+                                              nsIDOMNode* aRootNode,
>+                                              PRBool aIsTrusted,
>+                                              nsIXULTemplateBuilder* aBuilder,
>+                                              PRBool* aShouldDelayBuilding,
>+                                              nsISupports** _retval)
>+{
>+    nsCOMPtr<nsIRDFCompositeDataSource> compDB;
>+    nsCOMPtr<nsIContent> root = do_QueryInterface(aRootNode);
>+    nsresult rv;
>+
>+    *_retval = nsnull;
>+    * aShouldDelayBuilding = PR_FALSE;
>+
>+    NS_ENSURE_TRUE(root, NS_ERROR_UNEXPECTED);
>+
>+    // make sure the RDF service is set up
>+    rv = InitGlobals();
>+    if (NS_FAILED(rv))
>+        return rv;

    NS_ENSURE_SUCCESS(rv, rv);

>+
>+    // create a database for the builder
>+    compDB = do_CreateInstance(NS_RDF_DATASOURCE_CONTRACTID_PREFIX "composite-datasource");

Long line.

>+    if (! compDB) {

Space after !

>+        rv = gRDFService->GetDataSource("rdf:local-store", getter_AddRefs(localstore));

Long line.

>+        aDataSources->QueryElementAt(index, NS_GET_IID(nsIURI), getter_AddRefs(uri));

Use do_QueryElementAt.

>+        if (inferDB) {
>+            inferDB->SetBaseDataSource(compDB);
>+            db = do_QueryInterface(inferDB);
>+        } else {

Move the else on its own line.

>+    CallQueryInterface(db, _retval);
>+    return NS_OK;

Return the result of CallQueryInterface.
Use aResult instead of _retval.

>Index: content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp
>===================================================================

>+                                              nsISupports** _retval)

Use aResult instead of _retval.

>+    if (! root)

Space after !

>+        return NS_ERROR_UNEXPECTED;
>+
>+    nsCOMPtr<nsIDocument> doc = root->GetDocument();

GetCurrentDoc()

>+    if (! doc)

Space after !

>+    rv = aDataSources->QueryElementAt(0, NS_GET_IID(nsIDOMNode), getter_AddRefs(node));

do_QueryElementAt

>+    if (NS_SUCCEEDED(rv) && node) {

No need to check rv

>+        CallQueryInterface(node, _retval);
>+        return NS_OK;

Return the result of CallQueryInterface.

>+    rv = aDataSources->QueryElementAt(0, NS_GET_IID(nsIURI), getter_AddRefs(uri));
>+    if (NS_FAILED(rv) || !uri)
>+        return NS_ERROR_UNEXPECTED;

do_QueryElementAt
No need to check rv

>+        CallQueryInterface(domDocument, _retval);

Return the result of CallQueryInterface.

>+    if (eventType.EqualsLiteral("load") && mTemplateBuilder){

Add space before {

>+        if(target){

Ditto.

>+        return rv;
>+    }else{

No need for else after return.

>+        return NS_OK;

>+class nsXULTemplateQueryProcessorXML : public nsIXULTemplateQueryProcessor,

>+    nsCOMPtr<nsIXULTemplateBuilder> mTemplateBuilder;

This seems to add a cycle between the builder and the processor. You'll need to add cycle collection to nsXULTemplateQueryProcessorXML so the cycle gets broken or use a weak reference if that's possible.

sr=peterv with that taken care of.

Comment 26

[Laurent Jouanneau]

Attachment: Patch v4.2, ready for checkin

I fixed all things pointed by peterv in this new patch.

Comment 27

[Laurent Jouanneau]

Checkin needed. I don’t have commit access.

Comment 28

[Laurent Jouanneau]

Checkin made by smaug.

Thanks everybody.