321171 - Template Query Processor for XML

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Support using XML datasources. Initial code to come soon.

Comment 1

[Neil Deakin]

I have this mostly implemented but not going to do anything until bug 335122 is in.

Comment 2

[Neil Deakin]

Attachment: Support for XML in templates

Implements simple support for building template content from XML sources. More functionality can be added later.

bz, let me know if someone else should review this. Also I added a comment where I call CreateDocument as I don't know if that part is the correct way to initialize a document.

Documentation is at http://wiki.mozilla.org/XUL:Templates_with_XML
Testcases are at http://xulplanet.com/ndeakin/tests/xts/templates/xml/

Comment 3

[Boris Zbarsky [:bzbarsky]]

I probably won't get to this until at least sometime next week...

Comment 4

[alexander :surkov (:asurkov)]

That looks like xforms repeat element :). Neil, do you suppose to provide content generating by using for example @template attribute instead <template/> element? It allows to easy generate content of xul:grid for example.

Comment 5

[Neil Deakin]

Comment on attachment 229709 [details] [diff] [review]
Support for XML in templates

let's all reduce bz's review queue

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not really the right person to review template stuff. I can sr though.

Comment 7

[Neil Deakin]

(In reply to comment #6)
> I'm not really the right person to review template stuff. I can sr though.
> 

Fell free to suggest another person more suitable who doesn't exist then.

Comment 8

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

Could you perhaps update the patch.
3 out of 14 hunks FAILED -- saving rejects to file content/xul/templates/src/nsXULTemplateBuilder.cpp.rej

And some fuzz with other files.

Comment 9

[Neil Deakin]

Attachment: updated patch

Comment 10

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

Comment on attachment 262906 [details] [diff] [review]
updated patch

Is there a reason to use 4 space indent? IMO we should use 2 spaces in all new files.


>+nsresult
>+XMLBindingSet::AddBinding(nsIAtom* aVar, nsIDOMXPathExpression* aExpr)
>+{
>+    XMLBinding* newbinding = new XMLBinding(aVar, aExpr);
>+    if (! newbinding)
>+        return NS_ERROR_OUT_OF_MEMORY;

I'd prefer |if (!newbinding)| or in this case even
NS_ENSURE_TRUE(newBinding, NS_ERROR_OUT_OF_MEMORY);

>+
>+    if (mFirst) {
>+        XMLBinding* binding = mFirst;
>+
>+        while (binding) { 
>+            // if the target variable is already used in a binding, ignore it
>+            // since it won't be useful for anything
>+            if (binding->mVar == aVar)
>+                return NS_OK;

So here you leak newBinding.

>+class XMLBindingSet
...
>+    PRInt32 AddRef() { return ++mRefCnt; }

Could you add NS_LOG_ADDREF here

>+
>+    PRInt32 Release()
>+    {
>+        PRInt32 refcnt = --mRefCnt;
>+        if (refcnt == 0) delete this;
>+        return refcnt;
>+    }

and NS_LOG_RELEASE here. And also un-inline Release() and 
use the normal stabilization thing, so something like
  --mRefCnt;
  NS_LOG_RELEASE(this, mRefCnt, "XMLBindingSet");
  if (mRefCnt == 0) {
    mRefCnt = 1;
    delete this;
    return 0;
  }
  return mRefCnt;


> nsresult
>-nsXULTemplateBuilder::LoadDataSources(nsIDocument* doc)
>+nsXULTemplateBuilder::LoadDataSources(nsIDocument* aDocument, PRBool* aShouldDelayBuilding)
> {
...
>+            if (querytype.IsEmpty())
>+                querytype.AssignLiteral("xml");

use {} here. Makes the code easier to read.

>+        mQueryProcessor = new nsXULTemplateQueryProcessorRDF();
>+        if (! mQueryProcessor)
>+            return NS_ERROR_OUT_OF_MEMORY;

NS_ENSURE_TRUE(mQueryProcessor, NS_ERROR_OUT_OF_MEMORY);
same also elsewhere

>+        if (!mQueryProcessor) {
>+            // XXXndeakin should log an error here
>+            return rv;
>+        }

I think it might be useful to see some error message at least in debug builds, so
maybe using NS_ENSURE_TRUE(mQueryProcessor, rv) would be better here. And then
if better logging is needed, open a new bug for that and put a comment here
// Fix logging, bug xyzpqj.
Or something like that.

>+        // check for magical attributes. XXX move to ``flags''?
>+        nsAutoString coalesce;
>+        mRoot->GetAttr(kNameSpaceID_None, nsGkAtoms::coalesceduplicatearcs, coalesce);
>+        if (coalesce.EqualsLiteral("false"))
>+            mCompDB->SetCoalesceDuplicateArcs(PR_FALSE);

You could use nsIContent::AttrValueIs(...) method here, and also elsewhere.


>+        else {
>+            nsCOMPtr<nsIDOMDOMImplementation> implementation =
>+                do_CreateInstance(kDOMDOMImplementationCID, &rv);
>+            if (NS_FAILED(rv))
>+                return rv;
>+  
>+            nsCOMPtr<nsIPrivateDOMImplementation> privImpl = 
>+                do_QueryInterface(implementation);
>+            // XXXndeakin is this right?
>+            if (privImpl)
>+                privImpl->Init(docurl, docurl, principal);
>+
>+            nsCOMPtr<nsIDOMDocument> domDocument;
>+            nsAutoString emptyStr;
>+            rv = implementation->CreateDocument(emptyStr, 
>+                                                emptyStr, 
>+                                                nsnull,
>+                                                getter_AddRefs(domDocument));

Do you really need to do this. Couldn't you get the nsIDOMDOMImplementation from some
element's ownerDocument and use that to create a new document? Or just use nsContentUtils::CreateDocument which nsIDOMDOMImplementation::CreateDocument uses anyway.


>@@ -1880,21 +1996,23 @@ nsXULTemplateBuilder::CompileExtendedQue
...
>+    // allow bindings to be placed directly inside rule
>+    if (!bindings)
>+      bindings = aRuleElement;

check indentation 

>+NS_IMETHODIMP
>+nsXULTemplateResultSetXML::GetNext(nsISupports **aResult)
>+{
>+    nsCOMPtr<nsIDOMNode> node;
>+    nsresult rv = mResults->SnapshotItem(mPosition++, getter_AddRefs(node));

So mPosition is always the index of the next item, not the current one?

>+NS_IMETHODIMP
>+nsXULTemplateQueryProcessorXML::InitializeForBuilding(nsISupports* aDatasource,
>+                                                      nsIXULTemplateBuilder* aBuilder,
>+                                                      nsIDOMNode* aRootNode)
>+{
>+    // the datasource is either a document or a DOM element
>+    nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(aDatasource);
>+    if (doc)
>+        doc->GetDocumentElement(getter_AddRefs(mRoot));
>+    else
>+      mRoot = do_QueryInterface(aDatasource);
Shouldn't you make sure here that mRoot != nsnull ?
So maybe NS_ENSURE_STATE(mRoot);

>+
>+    mEvaluator = do_CreateInstance("@mozilla.org/dom/xpath-evaluator;1");

Check OOM here.

>+nsXULTemplateQueryProcessorXML::GenerateResults(nsISupports* aDatasource,
>+                                                nsIXULTemplateResult* aRef,
>+                                                nsISupports* aQuery,
>+                                                nsISimpleEnumerator** aResults)
>+{
>+    if (!aQuery)
>+        return NS_ERROR_INVALID_ARG;
>+
>+    mGenerationStarted = PR_TRUE;
>+
>+    nsXULTemplateResultXML* refxml = NS_STATIC_CAST(nsXULTemplateResultXML*, aRef);
>+    nsXMLQuery* query = NS_REINTERPRET_CAST(nsXMLQuery*, aQuery);

Can content access nsIXULTemplateQueryProcessor objects?
If yes, then this would create a security bug. I think you need to QI aRef and aQuery to
some internal interface(-like) thing, not cast. And make sure QIing succeeded.

>+NS_IMETHODIMP nsXULTemplateQueryProcessorXML::TranslateRef(nsISupports* aDatasource,
>+                                                           const nsAString& aRefString,
>+                                                           nsIXULTemplateResult** aRef)
>+{
...
>+    if (doc)
>+        doc->GetDocumentElement(getter_AddRefs(rootElement));
>+    else
>+      rootElement = do_QueryInterface(aDatasource);

indentation

>+
>+NS_IMETHODIMP nsXULTemplateQueryProcessorXML::CompareResults(nsIXULTemplateResult* aLeft,
...
>+    *aResult = ::Compare(nsDependentString(leftVal),
>+                         nsDependentString(rightVal),
>+                         nsCaseInsensitiveStringComparator());

Really CaseInsensitive ? Could you add a comment why.


>+    nsresult
>+    AddBinding(nsIAtom* aVar, nsIDOMXPathExpression* aExpr)
>+    {
>+        if (! mRequiredBindings)
>+            mRequiredBindings = new XMLBindingSet();
>+        if (! mRequiredBindings)
>+            return NS_ERROR_OUT_OF_MEMORY;
>+
if (!mRequiredBindings) {
    mRequiredBindings = new XMLBindingSet();
    NS_ENSURE_TRUE(mRequiredBindings, NS_ERROR_OUT_OF_MEMORY);
}



>+NS_IMETHODIMP
>+nsXULTemplateResultXML::GetId(nsAString& aId)
>+{
>+    char id[15];
>+    sprintf(id, "row:%d", mId);
>+
>+    aId.Assign(NS_ConvertUTF8toUTF16(id));
>+
>+    return NS_OK;
>+}

Why not
aId.AssignLiteral("row:");
aId.AppendInt(mId);
return NS_OK;

Comment 11

[Neil Deakin]

> 
> Can content access nsIXULTemplateQueryProcessor objects?

No, but a separate interface is probably better anyway.

> ...
> >+    *aResult = ::Compare(nsDependentString(leftVal),
> >+                         nsDependentString(rightVal),
> >+                         nsCaseInsensitiveStringComparator());
> 
> Really CaseInsensitive ? Could you add a comment why.

The RDF sorting is done case-insensitive. There was a patch somewhere that added the capability to specify whether case was relevant, but currently sorting is always done case-insensitive.

Comment 12

[Neil Deakin]

Attachment: address comments, convert query to implement an internal interface

Comment 13

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

Could you combine classes nsITemplateXMLQuery and nsXMLQuery, the latter 
one looks anyway almost like a struct. That way the interface/class 
wouldn't have to have virtual methods. Or maybe 
nsITemplateXMLQuery should contain all the member variables and 
getters/setters and nsXMLQuery would be there only to implement 
AddRef/Release and QueryInterface. In any case, it is better to try
to avoid virtual methods when possible.

Maybe same thing with some other interfaces/classes, didn't look at it yet.

Comment 14

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

Comment on attachment 263628 [details] [diff] [review]
address comments, convert query to implement an internal interface


>+void
>+XMLBindingValues::GetAssignmentFor(nsXULTemplateResultXML* aResult,
>+                                     XMLBinding* aBinding,
>+                                     PRInt32 aIndex,
>+                                     PRUint16 aType,
>+                                     nsIDOMXPathResult** aValue)

Indentation, also elsewhere in the same file.


>+void
>+XMLBindingValues::GetNodeAssignmentFor(nsXULTemplateResultXML* aResult,
...
>+  GetAssignmentFor(aResult, aBinding, aIndex,
>+                   nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE, getter_AddRefs(result));

Too long line

>+struct XMLBinding {

btw, would nsXMLBinding be a better name.

>+class XMLBindingSet

and here nsXMLBindingSet

>+  PRInt32
>+  LookupTargetIndex(nsIAtom* aTargetVariable, XMLBinding** aBinding)
>+  {
>+    if (! mBindings)
>+      return -1;
>+
>+    return mBindings->LookupTargetIndex(aTargetVariable, aBinding);
>+  }

Maybe
return mBindings ? 
  mBindings->LookupTargetIndex(aTargetVariable, aBinding) : -1;


> nsresult
>-nsXULTemplateBuilder::LoadDataSources(nsIDocument* doc)
>+nsXULTemplateBuilder::LoadDataSources(nsIDocument* aDocument, PRBool* aShouldDelayBuilding)

Put the second parameter to a new line.

>+    if (shouldLoadUrls) {
>+        rv = LoadDataSourceUrls(aDocument, datasources, isRDFQuery, aShouldDelayBuilding);

Too long line.
Long lines also elsewhere. Try to fix them when fixable.
http://beaufour.dk/jst-review/?patch=263628&file=

>+      // Parse datasources: they are assumed to be a whitespace
>+      // separated list of URIs; e.g.,
>+      //
>+      //     rdf:bookmarks rdf:history http://foo.bar.com/blah.cgi?baz=9
>+      //
>+    nsIURI *docurl = aDocument->GetDocumentURI();

Too many spaces before // -comments

>+            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.AppendWithConversion(uriStr);
>+                msg.Append('\'');
>+                NS_WARNING(msg.get());
>+  #endif

This code just moved , but would it make sense log this to error console.

>+            nsCOMPtr<nsIDOMEventTarget> xmlel = do_QueryInterface(domDocument);
>+            xmlel->AddEventListener(NS_LITERAL_STRING("load"), this, PR_FALSE);

Why the variable is called xmlel?

>+NS_IMETHODIMP
>+nsXULTemplateResultSetXML::GetNext(nsISupports **aResult)
>+{
>+    nsCOMPtr<nsIDOMNode> node;
>+    nsresult rv = mResults->SnapshotItem(mPosition, getter_AddRefs(node));
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    nsXULTemplateResultXML* result =
>+        new nsXULTemplateResultXML(mQuery, node, mBindingSet);
>+    NS_ENSURE_TRUE(result, NS_ERROR_OUT_OF_MEMORY);
>+
>+    mPosition++;

Nit, I prefer prefix over postfix. Prefix is after all a bit faster in some cases
(though, ofc compilers may optimize postfix to prefix in this case).



>+nsXULTemplateQueryProcessorXML::GenerateResults(nsISupports* aDatasource,
...
>+    if (!aQuery)
...
>+    if (! xmlquery)
...
>+    if (!context)


Nit, try to be consistent with if () syntax, either
if (! value) or if (!value)
I prefer the latter one.

>+nsXULTemplateQueryProcessorXML::AddBinding(nsIDOMNode* aRuleNode,
>+                                           nsIAtom* aVar,
>+                                           nsIAtom* aRef,
>+                                           const nsAString& aExpr)
...
>+    if (NS_FAILED(rv)) return rv;
You're changing these elsewhere so that return rv is in its own line.


>+NS_IMETHODIMP nsXULTemplateQueryProcessorXML::CompareResults(nsIXULTemplateResult* aLeft,
...
>+    // XXXndeakin handle other types
So what all other types. Could you perhaps file a bug and put the bug number here.

Fix also Comment #13

I'd still like to look at the patch at least once more. It is after all too small one. So r-.

Comment 15

[Neil Deakin]

Attachment: Update patch

Comment 16

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

Comment on attachment 263752 [details] [diff] [review]
Update patch

> nsXULContentBuilder::AddPersistentAttributes(nsIContent* aTemplateNode,
...
>+    if (!mRoot)
>+      return NS_OK;
indentation

>+    ~nsXMLQuery() { mProcessor = nsnull; }
I think this is not needed. And because it is not virtual, it probably doesn't even get called 
always.


>+    ~nsXULTemplateResultSetXML()
>+    {}
No need for this.

>+    ~nsXULTemplateQueryProcessorXML() {};
Nor this.

Comment 17

[Neil Deakin]

Attachment: remove extra constructors as well

Comment 18

[Laurent Jouanneau]

Attachment: updated patch for new template builder

The patch now contains only the query processor itself. Changes have been made on this query processor because of changes on the nsIXULTemplateQueryProcessor interface. Most of new things in the xul template builder have been moved into an other patch (see bug 321170).

Comment 19

[Laurent Jouanneau]

I suggest to change the superreviewer flag to Neil Rashbrook since Jonas didn't respond to the superreview request.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 263858 [details] [diff] [review]
remove extra constructors as well

There's some long lines and you've added useless whitespace on some empty lines.

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

>+  MOZ_COUNT_DTOR(nsXMLBindingSet);

No need for MOZ_COUNT_CTOR/MOZ_COUNT_DTOR if you use NS_LOG_RELEASE.

>+  while (mFirst) {
>+    nsXMLBinding* doomed = mFirst;
>+    mFirst = mFirst->mNext;
>+    delete doomed;

If you make mFirst and mNext nsAutoPtr's you don't need this.

>+PRInt32
>+nsXMLBindingSet::Release()
>+{
>+  --mRefCnt;
>+  NS_LOG_RELEASE(this, mRefCnt, "nsXMLBindingSet");
>+  if (mRefCnt == 0) {
>+    mRefCnt = 1;
>+    delete this;
>+    return 0;
>+  }
>+  return mRefCnt;
>+}

NS_IMPL_RELEASE

>+nsXMLBindingSet::AddBinding(nsIAtom* aVar, nsIDOMXPathExpression* aExpr)

>+  nsXMLBinding* newbinding = new nsXMLBinding(aVar, aExpr);

I'd make this an nsAutoPtr so you don't need to worry about early returns.

>+nsXMLBindingValues::GetAssignmentFor(nsXULTemplateResultXML* aResult,

>+  *aValue = mValues.SafeObjectAt(aIndex);
>+
>+  if (!*aValue) {

>+      if (result && mValues.ReplaceObjectAt(result, aIndex))

Why replace? SafeObjectAt(aIndex) returned null.

>Index: content/xul/templates/src/nsXMLBinding.h
>===================================================================

>+/*

Use /**

>+struct nsXMLBinding {

>+  nsXMLBinding* mNext;

nsAutoPtr

>+class nsXMLBindingSet

>+  PRInt32 mRefCnt;

nsAutoRefCnt

>+  nsXMLBinding* mFirst;

nsAutoPtr

>+  nsXMLBindingSet()

>+    MOZ_COUNT_CTOR(nsXMLBindingSet);

No need for this.

>+  PRInt32 AddRef() {
>+    mRefCnt++;
>+    NS_LOG_ADDREF(this, mRefCnt, "nsXMLBindingSet", sizeof(*this));
>+    return mRefCnt;
>+  }

NS_IMPL_ADDREF

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

> nsXULContentBuilder::AddPersistentAttributes(nsIContent* aTemplateNode,

>+    if (NS_FAILED(rv))
>+        return rv;

NS_ENSURE_SUCCESS(rv, rv);

(here and in other places)

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

>+#include "nsIDOMDOMImplementation.h"
>+#include "nsIPrivateDOMImplementation.h"

>+static NS_DEFINE_CID(kDOMDOMImplementationCID,   NS_DOM_IMPLEMENTATION_CID);

You don't seem to be using this.

>@@ -123,17 +128,18 @@ PRLogModuleInfo* gXULTemplateLog;

> nsXULTemplateBuilder::nsXULTemplateBuilder(void)
>-    : mDB(nsnull),
>+    : mDataSource(nsnull),
>+      mDB(nsnull),
>       mCompDB(nsnull),
>       mRoot(nsnull),
>       mQueriesCompiled(PR_FALSE),
>       mFlags(0),
>       mTop(nsnull)

There's no need to set nsCOMPtr's to null here.

>+nsXULTemplateBuilder::LoadDataSources(nsIDocument* aDocument,

>+        domdoc->GetElementById(Substring(datasources, 1),
>+                               getter_AddRefs(dsnode));
>+        if (dsnode) {
>+            if (querytype.IsEmpty()) {
>+                querytype.AssignLiteral("xml");

So if there is no element with the specified id we'll treat it as querytype=rdf?

>+    nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(aDocument);
>+    if (xuldoc)
>+        xuldoc->SetTemplateBuilderFor(mRoot, this);
> 
>+    // Now set the database on the element, so that script writers can
>+    // access it.

That comment belongs before the previous lines, no?

>+    nsXULElement *xulcontent = nsXULElement::FromContent(mRoot);
>+    if (! xulcontent) {

    if (!mRoot->IsNodeOfType(nsINode::eXUL))

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

>@@ -325,16 +351,17 @@ public:

>+    nsCOMPtr<nsISupports> mDataSource;

You should add mDataSource to cycle collection (since it seems like it could hold a node).

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

>+NS_IMPL_ISUPPORTS1(nsXULTemplateQueryProcessorXML, nsIXULTemplateQueryProcessor)

Since this seems to hold nodes it should probably participate in cycle collections. I'm ok with doing it in a follow-up bug, but it needs to happen.

>+nsXULTemplateQueryProcessorXML::CompileQuery(nsIXULTemplateBuilder* aBuilder,

>+    rv = CreateExpression(expr, aQueryNode, getter_AddRefs(compiledexpr));
>+    if (NS_FAILED(rv))
>+        return NS_OK;

Should this warn (invalid expression)?

>+nsXULTemplateQueryProcessorXML::CompareResults(nsIXULTemplateResult* aLeft,

>+    nsAutoString leftVal;
>+    aLeft->GetBindingFor(aVar, leftVal);
>+
>+    nsAutoString rightVal;
>+    aRight->GetBindingFor(aVar, rightVal);
>+
>+    // currently templates always sort case-insensitive
>+    *aResult = ::Compare(nsDependentString(leftVal),
>+                         nsDependentString(rightVal),
>+                         nsCaseInsensitiveStringComparator());

Why do you need the nsDependentString's?

>+nsXULTemplateQueryProcessorXML::CreateExpression(const nsAString& aExpr,

>+    if (mRoot) {

Why does this need to check for mRoot?

>+        nsCOMPtr<nsIDOMDocument> doc;
>+        aNode->GetOwnerDocument(getter_AddRefs(doc));
>+
>+        nsCOMPtr<nsIDOMXPathEvaluator> eval = do_QueryInterface(doc);
>+        if (eval) {
>+            nsresult rv =
>+                eval->CreateNSResolver(aNode, getter_AddRefs(nsResolver));
>+            if (NS_FAILED(rv))
>+                return NS_OK;
>+        }
>+    }
>+
>+    return mEvaluator->CreateExpression(aExpr, nsResolver, aCompiledExpr);

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

>+class nsITemplateXMLQuery : public nsISupports

Do we really need a separate interface? Can't you just give nsXMLQuery an IID?

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

>+static PRUint32 mTemplateId = 0;

sTemplateId

>+nsXULTemplateResultXML::nsXULTemplateResultXML(nsITemplateXMLQuery* aQuery,
>+                                               nsIDOMNode* aNode,
>+                                               nsXMLBindingSet* aBindings)
>+    : mQuery(aQuery), mNode(aNode)
>+{
>+    mId = ++mTemplateId;

Put this in the initializer list too.

>Index: content/xul/templates/src/nsXULTemplateResultXML.h
>===================================================================

>+class nsXULTemplateResultXML : public nsIXULTemplateResult

>+    nsCOMPtr<nsIDOMNode> mNode;

This should participate in cycle collection too.

Looks good apart from that.

Comment 21

[Peter Van der Beken [:peterv]]

(In reply to comment #20)
> >+      if (result && mValues.ReplaceObjectAt(result, aIndex))
> 
> Why replace? SafeObjectAt(aIndex) returned null.

Hmmm, I guess mValues can have holes?

Comment 22

[Neil Deakin]

> Why replace? SafeObjectAt(aIndex) returned null.
> Hmmm, I guess mValues can have holes?

Yes, there is a slot with a specific index for each variable

> So if there is no element with the specified id we'll treat it as
> querytype=rdf?

No, I'll change that to use xml when the uri starts with #

> >+    rv = CreateExpression(expr, aQueryNode, getter_AddRefs(compiledexpr));
> >+    if (NS_FAILED(rv))
> >+        return NS_OK;
> 
> Should this warn (invalid expression)?

Logging errors is bug 321169

> This should participate in cycle collection too.

I'll file a followup bug about adding cycle collection, which I had actually planned on doing anyway.

Comment 23

[Neil Deakin]

Attachment: Update for comments

Comment 24

[Peter Van der Beken [:peterv]]

Comment on attachment 268488 [details] [diff] [review]
Update for comments

>Index: content/xul/templates/src/nsXMLBinding.h
>===================================================================

>+  nsXMLBindingSet()
>+    : mRefCnt(0)
>+  {
>+  }
>+
>+  ~nsXMLBindingSet()
>+  {
>+  }

You can drop these.

Comment 25

[Neil Deakin]

I have a testcase that I will add to bug 378893.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Adding link to docs so they can move to devmo eventually.