Closed Bug 285631 Opened 19 years ago Closed 18 years ago

Improve XUL Templates

Categories

(Core :: XUL, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: enndeakin, Assigned: enndeakin)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 3 obsolete files)

This is a general bug about improving XUL templates as described at
http://wiki.mozilla.org/wiki/XUL:Templates_Plan

In particular, implementing phase 1 which involves breaking the template builder
into RDF and non-RDF parts. This is the most complicated part and will involve a
giant patch.

There isn't a templates component in buzilla, should there be one?
Status: NEW → ASSIGNED
This is the initial draft of the patch to break the template builder into RDF
and non-RDF parts. It doesn't actually implement any non-RDF generation
mechanisms.

It is not completely finished, there are some issues marked with XXXndeakin
still to fix. One known issue is that dragging bookmarks causes items to be
displayed incorrectly or duplicated, although the underlying data is ok.

I'm posting the patch so anyone who wants to can test for regressions, crashes,
etc. Especially on non-Linux. I have a bunch of testcases which I can post if
desired.

I also have a beginnings of a connection to mozStorage, though I'm not sure
where it would live (storage or content/xul/templates)

(Guess I'll have to break the patch up to fit into the 500K limit. Oh well, a
zip for now since it's late).
Note that the patch size limit was recently changed to 1MB.
- fixes numerous bugs with updating
- fixes bookmark dragging
- allows other query processors to be used
- rumour has it, it even builds on Windows and Mac

Vlad offered to review, but other eyes are welcome.
Attachment #179356 - Attachment is obsolete: true
Attachment #185839 - Flags: review?(vladimir)
will this work make it into 1.1?
I've been running with this patch for a few days now, with no ill effects; still
working on skimming through the code.  The interfaces look good; it doesn't look
like there would be any problems in creating a query processor for an arbitrary
query engine.  One comment so far:

+  /**
+   * Translate a ref attribute string into a result. This is used in the first
+   * pass. Other passes may use the result object directly.
+   *
+   * XXXndeakin perhaps remove this and use result id's as the ref
+   *
+   * @param aDatasource datasource for the data
+   * @param aRefString the ref attribute string
+   *
+   * @return the translated ref
+   */
+  nsIXULTemplateResult translateRef(in nsISupports aDatasource,
+                                    in AString aRefString);

I'd keep translateRef; I think it would be useful to be able to specify the ref
in a more "user friendly" format than any ID may be.

I've also seen the following assertion a few times when dragging things to the
bookmarks toolbar where the toolbar is already overflowing (dragging to the
empty space before the chevron), but now I can't reproduce this.. it doesn't
happen with 1.1a1, but something not directly template related may have changed
in the bookmarks code in the meantime.  Nothing seems to break even with this
assertion, though, and like I said I can't figure out how to reproduce it reliably..

###!!! ASSERTION: couldn't find row: 'iter != mRows.Last()', file
../../../../../mozilla-firefox/content/xul/templates/src/nsXULTreeBuilder.cpp,
line 1131
Comment on attachment 185839 [details] [diff] [review]
More complete templates patch

I'm going to go ahead and r+ this -- I've been running Firefox with this patch
for a while now, and have noticed no problems.	I haven't had a chance to
exercise the new features yet, but hopefully I will in the next few weeks as I
poke at some bookmarks stuff.

I think this should land as soon as the trunk opens for 1.9.
Attachment #185839 - Flags: review?(vladimir) → review+
Did anyone do some performance analysis on this? Maybe not just for a menu with
10 entries, this should scale well, IMHO. At least not worse than the current 
code.
I haven't done any detailed performance testing, but some testcases I tried show
approximately the same amount of time to build a list with 1000 items in it with
and without the changes.
Comment on attachment 185839 [details] [diff] [review]
More complete templates patch

shaver suggested that bz should take a look too
Attachment #185839 - Flags: review?(bzbarsky)
I won't be able to review anything this size any time soon... and it doesn't
help that I don't really know how templates work at the moment.  :(
Blocks: 86435
OK.  So I know Neil wrote a long series of blog posts, but I have not had time
to read it yet.  Is there a description somewhere of what this patch is actually
doing?  Having that on hand would make reviewing a lot easier...
bz, there is info at the url for this bug. This patch does the 
first point (labeled 1 on the wiki page), breaking nsXULTemplateBuilder.cpp up
into two pieces, the first remaining in nsXULTemplateBuilder.cpp such that it is
not RDF-specific. The second part nsXULTemplateQueryProcessorRDF.cpp, contains
the RDF specific code. This way, other 'second parts' could be created to access
xml data or mozstorage data. The patch also includes some syntax changes to make
the querying less rdf specific. Let me know if you want details of specific changes.
Ah, excellent.  Let me read that; I'm hoping to be able to do this review in the
next several days.
Comment on attachment 185839 [details] [diff] [review]
More complete templates patch

Here's some review comments on these changes, this is all code I know virtually nothing about, so I'm trying to learn as I go here... Here's what I've got so far.

- In content/xul/templates/public/nsIXULTemplateQueryProcessor.idl

+interface nsIXULTemplateQueryProcessor : nsISupports
+{
+  /**
+   * Initialize for query generation. This will called before the rules are

Add 'be' after 'will'.

+  /**
+   * Generate the results of a query and return them in an enumeration. The
+   * enumeration must contain nsIXULTemplateResult objects. If there are no
+   * results, an empty enumerator must be returned. The arguments may be used

s/enumeration/enumerator/?

- In nsContentTestNode::Constrain():

     InstantiationSet::Iterator last = aInstantiations.Last();
     for (InstantiationSet::Iterator inst = aInstantiations.First(); inst != last; ++inst) {
[...]
+        aInstantiations.Erase(inst--);

I know you didn't write this, but if we make it to the end of this block in the first iteration this will ask the iterator to step before the first item in the iterator, which works with this iterator, but it's probably worth adding a comment to saying that it's ok if this happens here...

- In nsRDFConInstanceTestNode::FilterInstantiations():

     InstantiationSet::Iterator last = aInstantiations.Last();
     for (InstantiationSet::Iterator inst = aInstantiations.First(); inst != last; ++inst) {
[...]
+        if (! valueres) {
+            aInstantiations.Erase(inst--);
+            continue;

Same thing.

+                nsResourceSet& containmentProps = mProcessor->GetContainmentProperties();
[...]
-                        if (mMembershipProperties.Contains(property)) {
+                        if (mProcessor->GetContainmentProperties().Contains(property)) {

Any reason not to use the containmentProps local here?

- In nsRDFConMemberTestNode::nsRDFConMemberTestNode(

+        nsAutoString cvar(NS_LITERAL_STRING("(none)"));
+        if (mContainerVariable)
+            mContainerVariable->ToString(cvar);
+
+        nsAutoString mvar(NS_LITERAL_STRING("(none)"));
+        if (mMemberVariable)
+            mMemberVariable->ToString(mvar);

Maybe flip these around so you don't waste time copying the string "(none)" if you have a mMemberVariable or mContainerVariable, i.e. add an else and loose the nsAutoString ctro arg? Same thing in nsRDFPropertyTestNode::nsRDFPropertyTestNode(), both versions.

Note to self, got through to nsRuleNetwork.h
Comment on attachment 185839 [details] [diff] [review]
More complete templates patch

I'm still trying to understand how this stuff fits together; what follows are some questions based on reading the interfaces.  First of all, it would really help if each interface had a short description of the overall interface and the way it interacts with the world, not just of the methods and properties.  For example, see <http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIChannel.idl#45>.

>Index: content/xul/templates/public/nsIXULTemplateBuilder.idl
>+#include "nsIAtom.idl"

You should be able to just forward-declare this, right?  Please do so, unless there's a good reason I'm missing for the include?

>+     * The query processor to use to generate results
>+     * XXX there wouldn't be any real value of getting this outside the
>+     *     template builder, but there needs to be means to set this.
>+     */
>+    attribute nsIXULTemplateQueryProcessor queryProcessor;

So just have a |void setQueryProcessor(in nsIXULTemplateQueryProcessor);| here, please.  I also see no obvious reason for the getter, and we don't really want outside stuff messing with the query processor while our template builder is using it.

Is the intent to allow setting this an arbitrary number of times?  I'm assuming it is; if not, this should be in some sort of init() method.

I also assume that changing the query processor will not rebuild or refresh or anything like that, and that to have it take effect usefully the caller will need to do so?  In either case, the behavior should be documented.

Is setting the query processor to null ok?  If so, what does it mean (that is, how will the template builder behave thereafter)?  Please document that.

Is anyone responsible for setting this to null?  Or is the query processor not allowed to hold a reference to the builder?

>+     * Inform the template builder about a result that has been added, changed
>+     * or removed. This method is expected to be called by a query processor
>+     * when the datasource has been changed.

"the datasource" would be that query processor's datasource?  If so, perhaps "query processor when its datasource has been changed"?

Don't query processors need to hold refs to the template builder to be able to call methods on it?  Still trying to sort out the ownership model.

>+     * If both the old result (aOldResult) and the new result (aNewResult) are
>+     * set, then the result is one that has changed. The results are expected
>+     * to have the same id.

"In this case the results are expected ..."

Please assert that they have the same id in the implementation?  Or throw an exception if they do not (in the latter case, document that)?

>+     * If aIsBindingUpdate is true, then only
>+     * a rule's bindings have changed. In this case, the rules are not
>+     * reapplied as it is expected that the same rule will still apply.

So the value aIsBindingUpdate is relevant only if both aOldResult and aNewResult are set?  That should probably be documented more clearly.  It also raises the question of why we don't have three methods here:  updateResult, resultAdded, resultRemoved.

>+     * If aOldResult is null, a new result aNewResult is available which
>+     * should be added to the set of results. The query node that the new
>+     * result applies to must be specified using the aQueryNode parameter.

Does that mean that aQueryNode is not relevant if both results are non-null?

> The
>+     * new result will be applied to the rules and the result's RuleMatched
>+     * method will be called for the matching rule.

Hmm... Is the result being applied to the rules, or are the rules being applied to the result?  My real problem here is having only a very vague understanding of what a "result" is in this context, perhaps...

What exception, if any, is thrown if both results are null?  Should document it.

>+    /**
>+     * Return the root result
>+     */
>+    nsIXULTemplateResult getRootResult();

In what sense is this a "root result"?  Results don't obviously form a tree, that I can see.

Why is this not a |readonly attribute nsIXULTemplateResult rootResult;| ?

>+     * Return the result for a given id. Only one such result is returned and
>+     * is always the currently active match.

What does it mean to be a currently active match?  Is that documented somewhere?
>+     * @param aId the id to return the result of

"of" or "for"?  I'm guessing "for".

>+     * Returns true if the node has content generated for it. This method is
>+     * intended to be called only by the query processor.
..
>+     * @param aTag tag that must match

Match what?

>+    PRBool hasGeneratedContent(in nsIRDFResource aNode, in nsIAtom aTag);

|boolean|, not PRBool.  If the builder is supposed to not be RDF-specific, what's this method doing here?

>+     * Adds a rule filter for a given rule, which may used for specialized

"may be used".

>+     * rule filtering. Any existing filter on a rule is removed.

"on the rule", right?

> The default
>+     * conditions specified inside the <rule> tag are applied before the
>+     * rule filter is applied.

Which means that the filter can cause things that would have matched the rule to not match, but not the other way around?  Or something else?

>Index: content/xul/templates/public/nsIXULTemplateQueryProcessor.idl
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

That's not consistent with the actual indentation in your code.  Either make the tab-width and c-basic-offset be 2 here (I would prefer that), or actually use 4-space indent in this file.

>+#include "nsIAtom.idl"
>+#include "nsISupports.idl"
>+#include "nsISimpleEnumerator.idl"
>+#include "nsIXULTemplateRuleFilter.idl"

Do you actually need all those included?  Or can you forward-declare some of them?  I suspect the latter.

>+   * Initialize for query generation. This will called before the rules are
>+   * processed and whenever the rules are rebuilt.
>+   *
>+   * @param aDatasource datasource for the data

Is the idea that different implementations of this interface will QI this to different interfaces?  Might want to say so explicitly.

>+   * @param aBuilder the template builder

Is that guaranteed to be the builder this is a processor for?  Document, please.

>+   * @param aTemplateNode the <template> node

You mean the <template> node that aBuilder is planning to build?  Or something else?  This should be more clearly documented.

>+                             in nsIDOMNode aRootNode);

aTemplateNode?  Or is it the docs that are wrong?

>+   * The reference variable may be used within the query as a placeholder for
>+   * the reference point, if it was set as the container attribute on the
>+   * template.

I'm not sure I follow this sentence....

>+   * The member variable is determined from the member attribute on the
>+   * template, or from the uri in the first action's rule if that attribute is
>+   * not present.

So the member variable is the value of the member attribute if it's set?  And if it's not it's gotten from the URI.... how?

Also "first action" where?  In aQuery?  In the document?  In the Gecko process?

> A rule processor may use the member variable as a hint to
>+   * indicate what variable is expected to contain the results.

What is a "rule processor"?  What does it mean for a variable to contain anything?  For this function, a "variable" is an atom.  Or do you mean that the atom holds the name of the variable and the value of the variable is a set of results?  Or something?

>+   * If necessary, the query builder may use other means to determine these
>+   * variables.

I'm not sure what this means.....

>+   * @param aBuilder the template builder

Which is using us as its query builder?

>+  nsISupports compileQuery(in nsIXULTemplateBuilder aBuilder,
>+                           in nsIDOMNode aQuery,
>+                           in nsIAtom aRefVariable,
>+                           in nsIAtom aMemberVariable);

So we're not really expecting script to call this, right?  Note that nsIAtom is not really scriptable (eg you can't create atoms from script).  Given that, is there a reason this method is scriptable?  Similar for other places that use atoms.

>+   * results, an empty enumerator must be returned. The arguments may be used
>+   * to establish the query and reference point.

What does it mean to "establish the query and reference point"?  Aren't we just passing those in?

>+   * The context reference (aRef) is a reference point used when calculating
>+   * results. It is calculated from the ref attribute on the first pass or the
>+   * parent node when calculating inner children.

Again, aren't we just passing it in here?

> It may be ignored if it is
>+   * not deemed relevant.

What happens then?  That is, how does the output of this method depend on the "reference point" at all?

> The reference variable specified during query
>+   * compilation is expected to be a placeholder for the reference point.

I'm not sure what this means.

>+   * This method is only called when compileQuery returns a non-null value for
>+   * a particular query.

Can one call compileQuery a bunch of times in a row, then call generateResults a bunch of times in a row?

>+   * @param aDatasource datasource for the data

Is this the same as the datasource that was passed to initializeForBuilding?

>+   * @param aQuery the compiled query returned from query compilation

So I assume this has to be a return value from compileQuery on this same query processor, right?  If so, please document that?  What will happen if it's not, or if null is passed (assuming we want to document that, as opposed to just saying "don't do that")?

>+   * Add a binding for a particular rule. A binding calculates a result for
....
>+   * @param aRef variable that holds reference value

What does that mean?

>+   * @param aExpr expression used to compute the value to assign

What sort of expressions are valid?  Where is the syntax described?  For example, can I use:

  "e^{2x + 3} \sin{5y}"

?  Based on what this documentation says, I don't see why not.

>+   * Translate a ref attribute string into a result. This is used in the first
>+   * pass.

First pass of what?

> Other passes may use the result object directly.

Meaning what?

>+   * @param aDatasource datasource for the data

Is this guaranteed to be the same as that passed to initializeForBuilding?

>+   * Compare two results to determine their order, used when sorting results.
>+   * This method should return -1 when the left result is less than the right,
>+   * 0 if both are equivalent, and 1 if the left is greater than the right.
>+   * The sort should only consider the specified field.

s/sort/comparison/ in that last sentence?

>+   * @param aLeft the left result to compare
>+   * @param aRight the right result to compare
>+   * @param aVar field to compare to

You mean field to compare on?

What does it mean to have a given field?  I see nothing on nsIXULTemplateResult talking about fields...

>Index: content/xul/templates/public/nsIXULTemplateResult.idl
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Again, make this match the actual indent.

>+#include "nsISupports.idl"
>+#include "nsIAtom.idl"
>+#include "nsIDOMNode.idl"
>+#include "nsIRDFResource.idl"

Again, you should be able to forward-declare some (most?) of these.

>+   * True if the template builder may process the children and generate child
>+   * content. If false, child content is not processed. This may be overriden
>+   * by syntax used in the template rule.

What's a template rule?  How is it related to an nsIXULTemplateResult?  Sounds like there is only one per any given result, but past that, it's not clear.

>+   * ID of the result. The content will be given this id.

What does that mean?  It'll have attributes of type ID (in the SGML/XML sense) set yo this string?

> The id should be
>+   * unique for a query.

What does that mean?  This id is an id for a result... right?

>+   * Resource for the result, which may be null. If set, the uri should be the
>+   * same as the ID property.
>+   */
>+  readonly attribute nsIRDFResource resource;

I thought we were going for RDF-independence in the interfaces?

>+   * Get a binding for a variable such as ?name for this result

In other words, return the value of the variable named "name"?  It might be clearer said that way...

>+  nsISupports getBindingFor(in nsIAtom aVar);

I'm not sure what this is useful for.  There is no way to pass this nsISupports back to this result, so I assume it will be used elsewhere.  But then the elsewhere will need to know something about what the actual implementation of nsIXULTemplateResult is returning here?  Or what?  I see the "RDF" impl of nsIXULTemplateQueryProcessor messing around with the return value of this method; is he idea that an nsIXULTemplateQueryProcessor might need to access the bindings for the nsIXULTemplateResults it creates?


>+   * Get a binding for a variable as a string such as ?name for this result

I don't follow that sentence.  Is the prepositional phrase "as a string" misplaced?

>+   * Indicate that a particular rule has matched and that output will be
>+   * generated for it.
>+   *
>+   * @param aQuery the query that matched

So was it a rule or a query that matched?  Are the two the same?  If so, why two different terms?

>+   * @param aRuleNode the rule node that matched

What is a "rule node" in this context?  Note that in Gecko in general we already have an nsRuleNode class that's very much unrelated to what's happening here...  I assume this is the <xul:rule> node involved, right?  I'd probably say it that way.

>+   * Indicate that the output for a result has been removed and that the
>+   * result is no longer being used by the builder.
>+   */
>+  void remove();

Should this be called hasBeenRemoved(), then?

>Index: content/xul/templates/public/nsIXULTemplateRuleFilter.idl
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Again, make that match reality.

>+   * Evaluate a result for a match.

Is that "evaluate whether a result matches a rule", or "evaluate the result of a matching process elsewhere"?  I assume you mean the former?

> The match method should return true if the
>+   * rule matches and the result should be accepted, or false if the rule does
>+   * not match and the result should be dropped.

Can it happen that the rule matches and the result should be dropped?  I assume no?
One other thing.  The various methods that take nsIDOMNodes for <rule>, etc should probably assert that the nodes are the right thing (XUL namespace and right tag, I'd guess).
Also, in various places where we have variable names, is the leading '?' always part of the name?  Or is the name the part after the '?'?
Also, at http://wiki.mozilla.org/XUL:Templates_Plan I'm not sure I follow what the value of the "uri" attribute really is....
OK, I'll try to update the idls with more detailed comments that answer the questions in the next few days. I'll try to write it as if the reader knows nothing or little about templates.

The template builder holds the reference to the query processor. The intent is that the query processor be implementable in JS but that the query processor can't be retrieved in any other way, and only the template builder should be calling its methods. I'm not sure if there's a way to specify that in idl or elsewhere.

I kept some nsIRDFResource references to avoid maintaining multiple copies of each id string in both the builder and query processor. Let me know if there's a better way to do that.
> I'll try to write it as if the reader knows nothing or little about templates.

That would be perfect!

> I'm not sure if there's a way to specify that in idl or elsewhere.

That sounds like something that could just be documented in the generic description of the nsIXULTemplateQueryProcessor interface.

> Let me know if there's a better way to do that.

I'm going to need to understand this code a little better first.  ;)

I'm sorry this has taken so long; now that I've actually started looking at it, it should go faster...
Is there any good in exposing the queryprocessor to web content? AFAICT,
the (nsIXULTemplateBuilder) builder is exposed, but can one implement a query 
processor without XPCOM (it's using atoms, for example, and has nsISupports for 
the data source, so you need QI).
I haven't convinced myself that the query processor needs to be implementable outside of a component. That would eliminate the need to have the queryProcessor member on the template builder. Instead, it would create a component using an attribute querytype="rdf" which maps to a component <contractid>?name=rdf

On the other hand, someone might have some privileged web content that needs to access some special data with a custom processor. Unprivileged code however, should be ok as long as a processor exists for xml and javascript objects.

I've tried to answer all of the questions above in the comments.

>+    attribute nsIXULTemplateQueryProcessor queryProcessor;

I decided to just remove this and not allow access to it outside of the builder.

> What does that mean?  It'll have attributes of type ID (in the SGML/XML sense)
> set yo this string?

Currently, it just sets the id attribute, it should probably set the attribute with the right ID type though.
Attachment #202189 - Flags: review?(bzbarsky)
You probably want to set GetIDAttributeName(), if it returns non-null.

I'll look on Wed night and Thursday.  Hopefully should be able to get through this then.
Comment on attachment 202189 [details] [diff] [review]
IDLs with more detailed comments

>Index: content/xul/templates/public/nsIXULTemplateBuilder.idl
>+ * results as the reference point. As an example for an XML datasource, the

Comma after "example", and no comma after "datasource", please. 

>+ * The <queryset> element contains a single query and one or more <rule>
>+ * elements. There may be more than one <queryset> if multiple queries are
>+ * desired, and is optional if only one is needed.

"and this element is optional if only one query is needed -- in that case the
<query> and <rule>s are allowed to be children of the <template> node".

>The template builder
>+ * then uses each result and generates output based on the <rule> elements.

Perhaps "the template builder then generates output for each result based on the <rule> elements"?  Or is that not correct?  I'd rather have correct than sounding smoother... ;)

>+ * input data or query syntax, while the template builder remains independant

"independent"

>+ Due to this, the query processor will be
>+ * supplied with the datasource and query which the template builder handles
>+ * in an opaque way, while the query processor handles these more
>+ * specifically.

I'm not quite sure what this paragraph means... Want to email me so we can work it out?

>+ * Each query may be accompanied by one or more <rule> elements. These rules
>+ * are evaulated

"evaluated"

>+ * If a result passes a rule's conditions, this is considered a match, and the
>+ * content within the rule's <action> body is inserted as a sibling of the
>+ * <template>

Can more than one rule match?  Or do we stop looking once a <rule> matches?

>+ * define additional variables to be used within an action body. Each of these
>+ * declared bindings should be supplied to the query processor

"should"?  Or "must"?  I assume the latter is what we mean...

>+ * addBinding method. The bindings are evaulated

"evaluated"

>+ * When using multiple queries, each may generate results with the same id.
>+ * These multiple results may all match the conditions, however only the
>+ * result for the earliest matching query in the template becomes the active
>+ * match and generates output.

Hmm... But each query has a separate set of rules, no?  So this is saying that if any two queries produce results with the same id and if both of those results match rules from their respective sets of rules, only the earlier one will be used, right?

>+     * Force the template builder to rebuild its content. All existing content
>+     * should be removed first. The query processor's done method will be

Perhaps put "done" in quotes?  Or make it "done()"?  Or something?

Also, it's not clear what "all existing content should be removed first" means.  Is that the caller's responsibility, somehow, or will the builder handle that?  The wording makes it sound like the former, but mention of "done()" being called during cleanup makes it sound like the latter.  If it's the latter, then s/should/will/, otherwise s/should/must/, please.


>+     * Inform the template builder that a new result is available. The builder
>+     * should add this result to the set of results.

"should", or "must"?  Or "will"?

>+     * The builder will apply the new result to the rules associated with the
>+     * query

I'd think we're applying the rules to the result, not the result to the rules...

>+     * Inform the template builder that a result should be replaced with
>+     * another.

s/a result/one result/

>+     * @throws NS_ERROR_INVALID_ARG if the ids don't match

Presumably also NS_ERROR_NULL_POINTER as needed?

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

>+ * The reference variable may be specified in a template by setting the
>+ * container attribute

"container" in quotes, please.

>+ * member variable may be specified in a similar way using the member
>+ * attribute

"member" in quotes.

>+ * The template builder must call initializeWithBuilding

initializeForBuilding?

> The
>+ * initializeForBuilding, compileQuery and addBinding methods may not be
>+ * called after generateResults has been called until the builder indicates
>+ * that the generated output is being removed by calling the done method.

But it's ok to call initializeForBuilding multiple times before calling generateResults()?

>+ * Currently, the datasource supplied to the methods will always be an
>+ * nsIRDFDataSource or a DOM node, and will always be the same one in between
>+ * calls to initializeForBuilding and done.

Is the latter something we want (or can?) assert in the implementation?  At least without leaking (eg hold a debug-only weak ref between initializeForBuilding and done() or something)?  If so, please do so?

>+   * Add an variable binding

"a variable"

>Index: content/xul/templates/public/nsIXULTemplateResult.idl

>+   * Resource for the result, which may be null. If set, the resource uri
>+   * should be the same as the ID property.

"should" or "must"?

r=bzbarsky on the IDL with those nits picked.  Thank you for the extra comments!  The setup is much much clearer now!
Attachment #202189 - Flags: review?(bzbarsky) → review+
I'm part way through the code itself; aiming to have that done by early next week (I'm away for part of the weekend, unfortunately).
Comment on attachment 185839 [details] [diff] [review]
More complete templates patch

For future reference, using the -p option to diff (in addition to the other options you used) makes the patch a good bit easier to review...

>Index: content/xul/templates/src/nsContentTestNode.cpp

>+nsContentTestNode::nsContentTestNode(TestNode* aParent,

>         PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
>+               ("nsContentTestNode[%p]: parent=%p ref-var=%d tag=%s",
>+                this, aParent, NS_ConvertUCS2toUTF8(refvar).get(),

You want "ref-var=%s" in the format string.  And UTF16, not UCS2, in the NS_Convert stuff.

>Index: content/xul/templates/src/nsContentTestNode.h

>+ * The nsContentTestNode is always the top node a query's rule network. It

"top node in a query's" ?  

>+ * exists so that Constrain can be filter out resources aren't part of a result.

Remove the "be"?

> class nsContentTestNode : public TestNode
>+    nsContentTestNode(TestNode* aParent,

If this is always a root, why even allow an aParent arg?  I'd think you would just pass nsnull to the TestNode() constructor in our constructor, instead of manually passing nsnull at all nsContentTestNode constructor callsites.

>Index: content/xul/templates/src/nsInstantiationNode.cpp

MOZ_COUNT_CTOR in the constructor, and MOZ_COUNT_DTOR in the destructor, please.

>+nsInstantiationNode::nsInstantiationNode(nsXULTemplateQueryProcessorRDF* aProcessor,
>+           ("nsInstantiationNode[%p] rule=%p", this, aQuery));

s/rule/query/ there?

>+nsInstantiationNode::Propagate(InstantiationSet& aInstantiations,
>+                               PRBool aIsUpdate, PRBool& aMatched)

>+        // XXXwaterson Unfortunately, this could also lead to retractions;
>+        // e.g., (contaner ?a ^empty false) could become "unmatched". How

Should that be "container" instead of "contaner"?

>+                    mProcessor->Builder()->UpdateResult(nsnull, nextresult,

I assume this has been updated to match the IDL?

>Index: content/xul/templates/src/nsRDFBinding.cpp

>+RDFBindingSet::AddBinding(nsIAtom* aVar, nsIAtom* aRef, nsIRDFResource* aPredicate)

>+    newbinding->mSubjectVariable = aRef;
...
>+    newbinding->mNext = nsnull;

Is there a reason those aren't arguments to the RDFBinding constructor?  I guess it's OK either way, but see comments on RDFBinding below.

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

That leaks newbinding.  Please fix that.

>+RDFBindingSet::SyncAssignments(nsIRDFResource* aSubject,

This method should have a |NS_PRECONDITION(aReslt, "Must have result")| or something like that...

>+    // iterate through the bindings looking ones that would match the RDF

"looking for ones"?

I'm not that happy with the equality comparison between nsIRDFResource* and nsIRDFNode* we have here...  If this is not perf-sensitive, I'd prefer SameCOMIdentity be used; if it is, comment why this is ok.

>+RDFBindingSet::AddDependencies(nsIRDFResource* aSubject,
>+                               nsXULTemplateResultRDF* aResult)

Is there a reason this doesn't add a dep on aSubject while RemoveDependencies removes said ref?  If yes, please document clearly; the asymmetry is quite confusing to me.

>+nsBindingValues::~nsBindingValues()

This should have a MOZ_COUNT_DTOR (and the constructor should have MOZ_COUNT_CTOR).

>+nsBindingValues::SetBindingSet(RDFBindingSet* aBindings)

Are we guaranteed that mBindings is null whenever this is called?  I don't see anything obvious guaranteeing this; if this is a real invariant, please add a corresponding assertion here and document it in the header.  If this is NOT a guaranteed invariant, this code leaks.

>+        mValues = new nsIRDFNode*[count];
>+        if (mValues) {

So on OOM this will just silently do nothing?  And then crash later in SyncAssignments where we assume that the binding set linked list length is equal to the array length here?  It looks to me like this method should return nsresult and callers should handle OOM.

>+nsBindingValues::GetAssignmentFor(nsXULTemplateResultRDF* aResult,
>+    // everytime.

 "every time"

>+                if (subjectValue) {
>+                    nsCOMPtr<nsIRDFResource> subject = do_QueryInterface(subjectValue);

This probably wants to assert |subject| is non-null.

>Index: content/xul/templates/src/nsRDFBinding.h

>+struct RDFBinding {

Should this have a private constructor (and have the one class that creates these beasties be a friend class)?

In any case, this should have MOZ_COUNT_CTOR and MOZ_COUNT_DTOR.

>+class RDFBindingSet
>+    PRInt32 AddRef() { return ++mRefCnt; }
>+    PRInt32 Release()

NS_LOG_ADDREF and NS_LOG_RELEASE, please.

>+    PRInt32 Count() { return mCount; }

That should be a const method.

>+     * Return true if the binding set contains a binding which would cause
>+     * the result to need resynchronizing for the given source and property.
>+     */
>+    PRBool
>+    SyncAssignments(nsIRDFResource* aSubject,
>+                    nsIRDFResource* aPredicate,
>+                    nsIRDFNode* aTarget,
>+                    nsIAtom* aMemberVariable,

Which of those are "source" and "property"?  Please document the arguments?

>+                    nsXULTemplateResultRDF* aResult,
>+                    nsBindingValues& aBindingValues);

Is it guaranteed that aBindingValues->BindingSet() == this?  If so, can you assert that in the method?  In either case, document what nsBindingValues this is.

>+class nsBindingValues
>+    RDFBindingSet* mBindings;

Is there a reason this is not nsRefPtr<RDFBindingSet>?

>+    nsIRDFNode** mValues;

Is there a reason why this isn't just nsCOMArray<nsIRDFNode>?  Are we trying to save the 4 bytes by not storing the length both here and in the RDFBindingSet?  But if so, would it not make more sense to just store the length in this array?  The only places I see Count() being used on the set are in SetBindingSet() (where we wouldn't need it if we just used the dynamically-sizing nsCOMArray) and in the destructor (where we would also not need it).

If there is a good reason to keep this as-is, please let me know and I'll review the code that uses it more carefully.

>+    RDFBindingSet* BindingSet() { return mBindings; }

If it doesn't guarantee a non-null return, please call it GetBindingSet().

>+    void SetBindingSet(RDFBindingSet* aBindings);

Please document when this should/must/must not be called.  I assume there's a good reason that's not just a constructor argument?

>Index: content/xul/templates/src/nsRDFConInstanceTestNode.cpp
>+nsRDFConInstanceTestNode::nsRDFConInstanceTestNode(TestNode* aParent,

>         PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
>                ("nsRDFConInstanceTestNode[%p]: parent=%p member-props=(%s) container-var=%d container=%s empty=%s",

container-var should be %s, no?

>+                NS_ConvertUCS2toUTF8(cvar).get(),

s/UCS2/UTF16/

>+        if (! valueres) {
>+            aInstantiations.Erase(inst--);

I assume that doing inst-- on an iterator that is already at First() is valid?  Seems to be, but I'm not quite sure.

>Index: content/xul/templates/src/nsRDFPropertyTestNode.cpp

>+nsRDFPropertyTestNode::nsRDFPropertyTestNode(TestNode* aParent,
>         PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
>                ("nsRDFPropertyTestNode[%p]: parent=%p source=%d property=%s target=%d",
>-                this, aParent, aSourceVariable, prop, aTargetVariable));
>+                this, aParent, NS_ConvertUCS2toUTF8(svar).get(), prop, NS_ConvertUCS2toUTF8(tvar).get()));

source and target should be %s.  Please make sure to actually test logging in this code; I'm trying to catch all instances of this problem, but I could always miss some...

And s/UCS2/UTF16/.

>+nsRDFPropertyTestNode::nsRDFPropertyTestNode(TestNode* aParent,
>         PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
>                ("nsRDFPropertyTestNode[%p]: parent=%p source=%s property=%s target=%d",
>-                this, source, prop, aTargetVariable));
>+                this, aParent, source, prop, NS_ConvertUCS2toUTF8(tvar).get()));

target should be %s, and s/UCS2/UTF16/.

>+nsRDFPropertyTestNode::nsRDFPropertyTestNode(TestNode* aParent,
>         PR_LOG(gXULTemplateLog, PR_LOG_DEBUG,
>                ("nsRDFPropertyTestNode[%p]: parent=%p source=%s property=%s target=%d",
>-                this, aSourceVariable, prop, NS_ConvertUCS2toUTF8(target).get()));
>+                this, aParent, NS_ConvertUCS2toUTF8(svar).get(), prop, NS_ConvertUCS2toUTF8(target).get()));

target should be %s.   There was some clear copy/paste fun here.  :(  And UTF16, not UCS2.

>Index: content/xul/templates/src/nsRDFQuery.cpp

>+NS_IMPL_ISUPPORTS2(nsRDFQuery, nsITemplateRDFQuery, nsISupports)

That should be NS_IMPL_ISUPPORTS1(nsRDFQuery, nsITemplateRDFQuery).  QI to nsISupports is done automatically, via casting to the first thing in the list for NS_IMPL_ISUPPORTSn.

>+nsRDFQuery::Finish()
>+    if (mCachedResults)
>+        delete mCachedResults;

delete is null-safe; no need for the null-check here.

>+nsRDFQuery::SetCachedResults(nsXULTemplateQueryProcessorRDF* aProcessor,
>+    if (mCachedResults)
>+        delete mCachedResults;

And here.

>Index: content/xul/templates/src/nsRDFQuery.h
>+class nsITemplateRDFQuery : public nsISupports
>+    NS_DEFINE_STATIC_IID_ACCESSOR(NS_ITEMPLATERDFQUERY_IID)

This needs to be updated to trunk's DECLARE/DEFINE pair stuff.

>+    virtual nsXULTemplateQueryProcessorRDF* Processor() = 0;  // not addrefed

Is that guaranteed to return non-null for all implementations?  If not, it should be GetProcessor.

>+    virtual nsIAtom* GetMemberVariable() = 0; // not addrefed

How is this atom related to this object?

>+    virtual void GetQueryNode(nsIDOMNode** aQueryNode) = 0;

Is that the node that this query was compiled from?  Document, please.

>+    virtual void ClearCachedResults() = 0;

What does that do?  There's no mention of cached results anywhere else on this interface.

>+class nsRDFQuery : public nsITemplateRDFQuery
>+    nsresult SetCachedResults(nsXULTemplateQueryProcessorRDF* aProcessor,
>+                              const InstantiationSet& aInstantiations);

Document that this takes ownership of aInstantiations, assuming this method succeeds?

It's not obvious to me how we're avoiding caching the same aInstantiations on multiple nsRDFQuerys.... Does every nsInstantiationNode see its own unique aInstantiations?  If so, how does that work when aIsUpdate is true?  I don't see the nsInstantiationNode taking ownership of aInstantiations there; do they just leak?

>+    nsXULTemplateResultSetRDF* UseCachedResults();

Document that the caller is thereafter responsible for deleting the object (and that we no longer have a reference to it after this call)?

>+    void ClearCachedResults() { mCachedResults = nsnull; }

Doesn't this leak if it's ever called?  That is, shouldn't this delete or something?

>+    nsXULTemplateQueryProcessorRDF* Processor() { return mProcessor; }

Never null, right?

>Index: content/xul/templates/src/nsRuleNetwork.cpp
>+nsAssignmentSet::GetAssignmentFor(nsIAtom* aVariable, nsIRDFNode** aValue) const
>+    *aValue = nsnull;

Could do that right before the |return PR_FALSE|, no?  I think that would make more sense.

>+TestNode::Propagate(InstantiationSet& aInstantiations,

>+    // if there is more than one child, each will need to be supplied with the
>+    // original set of instantiations from this node

That's not obvious from the documentation for Propagate() in nsRuleNetwork.h.  If I understand correctly, Propagate() is effectively responsible for taking ownership of the aInstantiations passed to it, right?   Hence the need for copying here and the reason that SetCachedResults is ok?

>+    if (! aInstantiations.Empty()) {

And if it is, do we leak it?

>+            // create a copy of the instantiations
>+            if (shouldCopy) {

Don't we leak the original in this case?  Also, is there a reason this case does not update aMatched?

>+                InstantiationSet* instantiations =
>+                    new InstantiationSet(aInstantiations);
>+                kid->Propagate(*instantiations, aIsUpdate, matched);

This needs to handle out-of-memory, no?

> PRBool
> TestNode::HasAncestor(const ReteNode* aNode) const
> {
>+    if (! mParent)
>+        return PR_FALSE;
>+
>     return aNode == this ? PR_TRUE : mParent->HasAncestor(aNode);

So for a node with no mParent, calling HasAncestor() on itself will return false, while for all other nodes it will return true?  That seems somewhat inconsistent, and given the recursive nature of this method will always return false if aNode has no parent.  Don't you want to return something more like:

  return aNode == this || (mParent && mParent->HasAncestor(aNode));

and skip that if check?

>Index: content/xul/templates/src/nsRuleNetwork.h
> class MemoryElement {

While you're here, add MOZ_COUNT_CTOR/MOZ_COUNT_DTOR to this?

>@@ -429,19 +283,19 @@
>+public:
>     List* mAssignments;

I assume there is a reason to make that public?  If so, might be worth documenting, and similar with other things in this file that are becoming public.

> /**
>+ * A collection of variable-to-value bindings, with the memory elements
...
>+ * children). When an instantiation that gets to the last node of the rule

s/that gets/gets/

>@@ -810,21 +668,22 @@
>+    virtual nsresult Propagate(InstantiationSet& aInstantiations,
>+                               PRBool aIsUpdate, PRBool& aMatched) = 0;

As commented above, there needs to be some discussion here about the ownership model for aInstantiations.

> @@ -902,313 +761,105 @@
>+    PRInt32 Count() { return mCount; }

Const method?

>+class TestNode : public ReteNode
>+    virtual PRBool HasAncestor(const ReteNode* aNode) const;

Does that need to be virtual?  That is, does anyone actually override it?

>Index: content/xul/templates/src/nsTemplateMatch.h
>+    nsTemplateRule* mRule;
...
>+    nsTemplateQuerySet* mQuerySet;

We don't own these, right?  Worth documenting.

>+    nsTemplateMatch *mNext;

We don't own this either, right?

>Index: content/xul/templates/src/nsTemplateRule.cpp
>+nsTemplateCondition::nsTemplateCondition(nsIAtom* aSourceVariable,

MOZ_COUNT_CTOR (and same in the other constructors).  And MOZ_COUNT_DTOR in the destructor, please.

>+nsTemplateCondition::nsTemplateCondition(nsIAtom* aSourceVariable,
...
>+                                         PRBool aIsMultiple)

>+                nsAutoString str(Substring(aTargets,start,end - start));
>+                mTargetList.AppendString(str);

Why do you need |str|?  You should be able to just append the Substring to mTargetList.

>+            nsAutoString str(Substring(aTargets,start));
>+            mTargetList.AppendString(str);

Same here.

I guess you can't do much about OOM here, eh?  :(

>+nsTemplateCondition::SetRelation(const nsAString& aRelation)

Use EqualsLiteral() instead of Equals(NS_LITERAL_STRING()), please.

>+nsTemplateCondition::CheckMatchStrings(const nsAString& aLeftString,
>+            case eBefore:
>+            {
>+                // XXXndeakin is case sensitive not needed here?

I'm not sure I follow this comment.

>+                else {
>+                    match = (aLeftString < aRightString);

Hmm...  Does that actually work on nsAString?  I'm not seeing an obvious operator< for those...  I understand that this compiled; I just don't quite see why.

>+nsTemplateRule::GetAction(nsIContent** aAction) const
>+    *aAction = mAction.get();

Why the .get() here?  Shouldn't be needed.

>+nsTemplateRule::CheckMatch(nsIXULTemplateResult* aResult)
>+    // if there are no conditions, the result always matches

Even if there is a rule filter?  That doesn't match the interface comments, as far as I recall...

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

How about:

  return NS_FAILED(rv) || match;

?

>Index: content/xul/templates/src/nsTemplateRule.h
>+    // relations that may be used in a rule. They may be negated with the
>+    // negate flag. Less and Greater are used for numeric comparisons and
>+    // Before and After are used for string comparisons.
>+    enum ConditionRelation {

Document whether this is how source relates to target or the other way around?  That is, document that |eContains| means the source contains the target?

>+    PRBool              mIgnoreCase;
>+    PRBool              mNegate;

PRPackedBool for those two?

> class nsTemplateRule
>+    nsresult GetAction(nsIContent** aAction) const;

In a followup bug, might want to make this return nsIContent* (without addrefing).

>+    nsresult GetRuleNode(nsIDOMNode** aResult);

So is this the same as the result of GetAction() and then QI?  That's what the documentation sounds like... If that's not what it is, can the comments be clarified?  Also, why is this not a const method?

>+    nsTemplateQuerySet* GetQuerySet() const { return mQuerySet; }

This can be null, right (per naming of method)?  If this is never null, name this |QuerySet()|, please.

>+    PRBool
>+    CheckMatch(nsIXULTemplateResult* aResult);

Document?  And this is not a const method?  Why not?

>+    nsresult
>+    AddBindingsToQueryProcessor(nsIXULTemplateQueryProcessor* aProcessor);

Document?

>+    // indicates that the rule will only when generating content inside an
>+    // container with this tag

I don't follow that comment.  "will" what?

>     Binding* mBindings;

Do we own that?

>+public:
>+
>+    nsCOMPtr<nsIAtom> mRefVariable;
>+    nsCOMPtr<nsIAtom> mMemberVariable;
>+
>+    nsTemplateCondition* mConditions;

Why are these public?  And do we own mConditions?

>+class nsTemplateQuerySet
>+    nsTemplateQuerySet(PRInt32 aPriority)

MOZ_COUNT_CTOR.

>+    ~nsTemplateQuerySet()

MOZ_COUNT_DTOR.

>+    PRInt32 GetPriority()

I'd name this |Priority()|.  And make it const.

>+    PRInt32 GetRuleCount()

Again, no Get.  And const.

>+    nsTemplateRule* GetRuleAt(PRInt32 aIndex)
>+        return (nsTemplateRule* )mRules[aIndex];

NS_STATIC_CAST, please.

>+    void Clear()
>+            delete (nsTemplateRule *)mRules[r];

NS_STATIC_CAST, and store in a temporary before deleting -- some of our compilers don't like deleting rvalues (which is what a cast result is), and iirc they're correct per spec.

>Index: content/xul/templates/src/nsTreeRows.cpp
>+nsTreeRows::FindByResource(nsIRDFResource* aResource)
>+                const char *uri;
>+                aResource->GetValueConst(&uri);
>+                resourceid.Assign(NS_ConvertUTF8toUCS2(uri));

  CopyUTF8ToUTF16(uri, resourceid);

instead of the convert + assign.

>+                iter--;

Is this safe when iter == First()?  In this case that's _very_ nonobvious to me...  In fact, as far as I can tell this iterator cannot step before First().

>+
...
> // nsTreeRows::Subtree

Don't add that line of whitespace.

I'm up through the beginning of nsXULContentBuilder.cpp.  More tomorrow afternoon.
(In reply to comment #27)
>I'm not that happy with the equality comparison between nsIRDFResource* and
>nsIRDFNode* we have here...  If this is not perf-sensitive, I'd prefer
>SameCOMIdentity be used; if it is, comment why this is ok.
I suppose that in general you could implement nsIRDFResource through some weird tearoff scenario, even though it inherits from nsIRDFNode, but I believe parts of RDF require all nsIRDFNodes to statically cast to nsRDFNode anyway.
Sorry, that was inaccurate; I meant all nsIRDFResources are required to statically cast to nsRDFResource, which singly inherits from nsIRDFNode.
Comment on attachment 185839 [details] [diff] [review]
More complete templates patch

> Index: content/xul/templates/src/nsTemplateRule.h

>+    void AddRule(nsTemplateRule *aChild)
>+        mRules.AppendElement(aChild);

That can fail (return PR_FALSE) -- when it does, you probably want to return NS_ERROR_OUT_OF_MEMORY so the caller can delete aChild or something.

>Index: content/xul/templates/src/nsXULContentBuilder.cpp
>     SynchronizeUsingTemplate(nsIContent *aTemplateNode,

Want to document this while you're here?  Followup patch ok for this.  Same applies to various other existing methods that you touched.

>+    CreateContainerContentsForQuerySet(nsIContent* aElement,

Please document the new methods you add (in this patch).  Especially what aContainer and aNewIndexInContainer will be, since we've had trouble with that before...

>+    GetElementsForResult(nsIXULTemplateResult* aResult,
>+                         nsISupportsArray* aElements);

In a followup, probably worth moving this to an nsCOMArray.

>+    GetInsertionLocation(nsIXULTemplateResult* aOldResult,

Document, please.  What does the return value mean?  What can be done with the out param?

>+    ReplaceMatch(nsIXULTemplateResult* aOldResult,

Document, please.  If this is documented elsewhere, point to those docs.

>+    SynchronizeResult(nsIXULTemplateResult* aResult);

Document, please.  If this is documented elsewhere, point to those docs.

> nsXULContentBuilder::BuildContentFromTemplate(nsIContent *aTemplateNode,
>+#ifdef DEBUG_ndeakin
>+    printf("nsXULContentBuilder::BuildContentFromTemplate (is unique: %d) ", aIsUnique);

Is there a reason this is not using gXULTemplateLog?  I'd think it should.

>+        // Check whether this element is the generation element. The
>+        // element

Second sentence should start "The generation element", right?

>+        // are descnendants of the generation element in the conte

"descendants", and "content" (and yes, I know it was mis-spelled before...)

>+        // Note that |isGenerationElement| and |isUnique| are mutually
>         // exclusive.
>-        PRBool isResourceElement = PR_FALSE;
>+        PRBool isGenerationElement = PR_FALSE;
>         PRBool isUnique = aIsUnique;

So if aIsUnique is false... then what?  I realize this code and comment were this way already, but if the comment doesn't match reality we should fix it.

>             // We identify the resource element by presence of a
>             // "uri='rdf:*'" attribute. (We also support the older
>             // "uri='...'" syntax.)
>             nsAutoString uri;
>             tmplKid->GetAttr(kNameSpaceID_None, nsXULAtoms::uri, uri);

Do we still support "uri='...'"?  Is that all subsumed into the existence of |aMatch->mRule| now?

If so, do we still need to check for !uri.IsEmpty(), or can we just check that tmplKid->HasAttr(kNameSpaceID_None, nsXULAtoms::uri)?

>@@ -576,85 +569,61 @@
>+        else if (isGenerationElement) {
...
>-            if (iscontainer) {
>-                realKid->SetAttr(kNameSpaceID_None, nsXULAtoms::container,
>-                                 NS_LITERAL_STRING("true"), PR_FALSE);
...
>+            // Set up the element's 'container' and 'empty' attributes.
>+            SetContainerAttrs(realKid, aChild);

So now we set the "container" attr even if it's not a container (set it to false in that case).  And we pass PR_TRUE for aNotify in SetContainerAttrs.  We didn't use to notify here; why do we want to start now?

We're also now notifying for the "empty" attr; again, do we want to be?

Perhaps SetContainerAttrs should take an aNotify arg?

>+nsXULContentBuilder::RemoveMember(nsIContent* aContent,
>+    PRInt32 pos = parent->IndexOf(aContent);

I assume there's a reason why |parent| must be non-null here?  Please document it?

>+    NS_ASSERTION(pos >= 0, "parent doesn't think this child has an index");
>+    if (pos < 0) return NS_OK;

That doesn't play so well with XBL... might be worth a separate bug, since neither did the original code.

>+    nsresult rv = parent->RemoveChildAt(pos, PR_TRUE);

Is it OK that we now always notify?  I guess we only ever passed PR_TRUE to RemoveMember, eh?

> nsXULContentBuilder::CreateTemplateAndContainerContents(nsIContent* aElement,
>+#ifdef DEBUG_ndeakin
>+    printf("nsXULContentBuilder::CreateTemplateAndContainerContents start ***\n");

Again, why not NSPR logging?

You might want to log mFlags here.

>+    if (aElement == mRoot) {
...
>+                if (NS_FAILED(rv)) return rv;

Please put that on two separate lines.

...
>+    else {

Why not |else if (!(mFlags & eDontRecurse))| ?  That way you can avoid calling Get() on the hashtable when you plan to ignore the result anyway.

>+            if (NS_FAILED(rv) || ! mayProcessChildren) return rv;

Please remove the space after '!'.  That's true in general, but here it makes the code particularly hard to read.  Also, put the |return rv;| part on a separate line.

>+#ifdef DEBUG_ndeakin
>+    printf("nsXULContentBuilder::CreateTemplateAndContainerContents end ***\n");

NSPR logging.

> nsXULContentBuilder::CreateContainerContents(nsIContent* aElement,
>-        if (xulcontent->GetLazyState(nsXULElement::eContainerContentsBuilt))
>-            return NS_OK;
>+         if (xulcontent->GetLazyState(nsXULElement::eContainerContentsBuilt))
>+             return NS_OK;

Please don't make that indentation change.

>+    PRInt32 rulecount = mQuerySets.Count();

Shouldn't that be |querySetCount|?  At least if I understand right, there are several rules per queryset, and this is a list of querysets...

>+    for (PRInt32 r = 0; r < rulecount; r++) {
>+        CreateContainerContentsForQuerySet(aElement, aResult, aNotify, queryset,
>+                                          aContainer, aNewIndexInContainer);

So this is ok because CreateContainerContentsForQuerySet only writes to *aContainer if it's null?  Might be worth documenting that....


>+nsXULContentBuilder::CreateContainerContentsForQuerySet(nsIContent* aElement,
>+#ifdef DEBUG_ndeakin

NSPR logging?

>+    if (NS_FAILED(rv) || ! results) return rv;

No space after '!' and return on separate line, please.

>+    PRBool hasMoreResults;
>+    results->HasMoreElements(&hasMoreResults);
>+    while (hasMoreResults){

You don't need to check the rv of HasMoreElements?  I don't really see why not..

>+        rv = results->GetNext(getter_AddRefs(nextresult));
>+        if (NS_FAILED(rv)) return rv;

Return on separate line, please.

>+        rv = GetResultResource(nextresult, getter_AddRefs(resultid));
>+        if (NS_FAILED(rv)) return rv;

And here.

>+        if (! resultid) {
>+            results->HasMoreElements(&hasMoreResults);
>             continue;

This might be easier if you use a for() loop with HasMoreElements() in the third statement rather than a while loop...  In either case, why no check of rv here?

>+        nsTemplateMatch *newmatch =
>+            nsTemplateMatch::Create(mPool, aQuerySet, nextresult);

Throw NS_ERROR_OUT_OF_MEMORY if this returns null?

>+                rv = newmatch->RuleMatched(aQuerySet, matchedrule, nextresult);
>+                if (NS_FAILED(rv)) return rv;

Return on separate line.

>+            mMatchMap.Put(resultid, newmatch);

What if that fails?

>+        results->HasMoreElements(&hasMoreResults);

Why no check of rv?

>+nsXULContentBuilder::HasGeneratedContent(nsIRDFResource* aResource,
>+    nsresult rv = mRootResult->GetResource(getter_AddRefs(rootresource));
>+    if (NS_FAILED(rv)) return rv;

Return on separate line, please.

>+        if (!aTag || (mRoot->Tag() == aTag))

No need for the extra parens here.

>+        nsAutoString refID;
>+        refID.Assign(NS_ConvertUTF8toUCS2(uri));

Why not just:

  NS_ConvertUTF8toUCS2 refID(uri);

?

>+        nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(mRoot->GetDocument());
>+        NS_ASSERTION(xuldoc, "expected a XUL document");

Should this really be asserting if the XUL elements in question happen to be in a non-XUL document?

>+        rv = NS_NewISupportsArray(getter_AddRefs(elements));
>+        if (NS_FAILED(rv)) return rv;

Return on separate line.

>+        rv = elements->Count(&cnt);
>+        if (NS_FAILED(rv)) return rv;

And here.

>+            nsISupports* isupports = elements->ElementAt(i);
>+            nsCOMPtr<nsIContent> content(do_QueryInterface(isupports));
>+            NS_IF_RELEASE(isupports);

How about:

  nsCOMPtr<nsIContent> content = do_QueryElementAt(elements, i);

instead?

>+                if ((content == mRoot) || mContentSupportMap.Get(content, &match)) {

No need for the extra parens.

>+                    if (! aTag || (content->Tag() == aTag)) {

And here.

>+nsXULContentBuilder::GetInsertionLocation(nsIXULTemplateResult* aResult,
>+    rv = NS_NewISupportsArray(getter_AddRefs(elements));
>+    if (NS_FAILED(rv)) return PR_FALSE;

Return on separate line.

>+    if (! xuldoc) return PR_FALSE;

And here.

>+    xuldoc->GetElementsForID(ref, elements);

Please file a followup bug to move GetElementsForID over to using nsCOMArray?  That would let us clean up a lot of these callers a bit..

>+        nsISupports* isupports = elements->ElementAt(t);
>+        nsCOMPtr<nsIContent> content = do_QueryInterface(isupports);
>+        NS_IF_RELEASE(isupports);

do_QueryElementAt, please.

>+        if ((content == mRoot) || mContentSupportMap.Get(content, &refmatch)) {

Remove the extra parens.

>+            // happens, for example, if we recieve an assertion on a

receive.

>+            if (xulcontent &&
>+                xulcontent->GetLazyState(nsXULElement::eContainerContentsBuilt))
>+                *aLocation = content;

Curly braces around that body, please?  It'd be much clearer, imo.

>+nsXULContentBuilder::ReplaceMatch(nsIXULTemplateResult* aOldResult,
>+        nsresult rv = GetElementsForResult(aOldResult, &elements);
>+        if (NS_FAILED(rv)) return rv;

Return on separate line.

>+            nsISupports* isupports = elements.ElementAt(e);
>+            nsCOMPtr<nsIContent> content = do_QueryInterface(isupports);
>+            NS_IF_RELEASE(isupports);

do_QueryElementAt.

> nsresult
> nsXULContentBuilder::OpenContainer(nsIContent* aElement)
>+        if (! xuldoc)
>+            return PR_FALSE;

That's not an nsresult.

>+        // See if we're responsible for this element
>+        nsCOMPtr<nsIContent> content = aElement;

This can just be an nsIContent*, no?

>+        if (mFlags & eDontRecurse)
>+            return NS_OK;

Can't we just check that before walking aElement's parent chain, etc?  Seems to me like we should.

>+        nsresult rv = result->GetMayProcessChildren(&mayProcessChildren);
>+        if (NS_FAILED(rv) || ! mayProcessChildren) return rv;

Remove space after '!', and return on separate line.

>@@ -1864,33 +1819,34 @@
>+    // XXXndeakin not sure if commenting this out is a good thing or not.
>+    // Leaving it in causes templates where the ref is set dynamically to
>+    // not work due to the order in which these state bits are set.

Please file a followup bug on this -- we should make sure to resolve it before 1.9.

>Index: content/xul/templates/src/nsXULContentUtils.cpp

>+nsICollation *nsXULContentUtils::gCollation = nsnull;

It's a static -- it'll be null by default.


>@@ -131,16 +134,32 @@
>+        nsCOMPtr<nsILocaleService> localeService = do_GetService(NS_LOCALESERVICE_CONTRACTID, &rv);

I know you just copy/pasted this whole chunk of code, but would you mind making it not stick out past 80 columns?

>+            if (NS_SUCCEEDED(rv = localeService->GetApplicationLocale(getter_AddRefs(locale))) && (locale)) {

Remove extra parens from around |locale| at the end there?

>Index: content/xul/templates/src/nsXULContentUtils.h
>+#include "nsICollation.h"
>+#include "nsCollationCID.h"
>+#include "nsILocale.h"
>+#include "nsILocaleService.h"

Why do you need all these includes here?  You shouldn't need any of them -- just forward-declaring nsICollation and then including these in the .cpp should work fine.

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

> nsXULTemplateBuilder::nsXULTemplateBuilder(void)
>+    mMatchMap.Init();

What if that fails?

>+    mPool.Init("nsXULTemplateBuilder", bucketsizes, 1, 256);

Or this?

Those should both live in some sort of Init() method.

>+nsXULTemplateBuilder::Uninit(PRBool aIsFinal)
>+        delete (nsTemplateQuerySet *)mQuerySets[q];

That needs to be stored in a local to be deleted in a reasonable way, last I checked (some compilers don't like calling delete on rvalues).

>+    return NS_OK;

So why is this an nsresult method, it it always returns NS_OK?

>+nsXULTemplateBuilder::AddRuleFilter(nsIDOMNode* aRule, nsIXULTemplateRuleFilter* aFilter)
>+    if (! aRule || !aFilter)

No space after '!', please.

>+    // added, it replaces the old one. Look for the right rule and set it's

"its".

>+            nsTemplateRule* rule = (nsTemplateRule *)queryset->GetRuleAt(r);

Why the cast?

>@@ -289,415 +357,460 @@

>+    // create the query processor if it isn't already set. The type attribute

We seem to create it even if it's set.  Is that desirable?

>+    if (! tmpl)
>+        return NS_OK;

This loses the rv from LoadDataSources().  On purpose?  Or should there be an early return if LoadDataSources() fails?


>+    if (type.IsEmpty() || type.Equals(NS_LITERAL_STRING("rdf"))) {

type.EqualsLiteral("rdf")

>+        nsXULTemplateQueryProcessorRDF *processor =
>+            new nsXULTemplateQueryProcessorRDF();
...
>+        NS_ADDREF(processor);
>+        mQueryProcessor = processor;

That leaks the processor, no?  Just assign the result of |new| to mQueryProcessor directly, perhaps?

>+        cid.Append(NS_ConvertUCS2toUTF8(type));

AppendUTF16toUTF8(type, cid);

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

Should also perhaps return a useful rv (the only way mQueryProcessor is null there is if the do_CreateInstance failed, and it might be worth knowing why it failed).

>+nsXULTemplateBuilder::UpdateResult(nsIXULTemplateResult* aOldResult,
>+#ifdef DEBUG_ndeakin
>+    printf("nsXULTemplateBuilder::UpdateResult %p %p %p %d\n",

NSPR logging?

>+    if (! aOldResult && ! aNewResult)

No space after '!', please.

Note to self: UpdateResult got split into three methods; need to review the actual code once it's in the bug.

>+nsXULTemplateBuilder::GetResultForId(const nsAString& aId,

>+            if (match->mRule) {
>+                *aResult = match->mResult;
>+                NS_IF_ADDREF(*aResult);
>             }

Don't you want to break at the end of the if body?

>+nsXULTemplateBuilder::AddListener(nsIXULBuilderListener* aListener)
>+    mListeners.AppendObject(aListener);

AppendObject can fail, no?  You want to throw NS_ERROR_OUT_OF_MEMORY in that case.

>+nsXULTemplateBuilder::AttributeChanged(nsIDocument *aDocument,
>+        if (aAttribute == nsXULAtoms::ref)

and aNameSpaceID == kNameSpaceID_None, no?  In fact, that should probably be part of the aContent == mRoot test, since it applies to datasources too.

>+nsXULTemplateBuilder::DocumentWillBeDestroyed(nsIDocument *aDocument)
>+    // Break circular references
>+    if (mDB) {

Why do we need that test?

> nsXULTemplateBuilder::LoadDataSources(nsIDocument* doc)
> 	// check for magical attributes. XXX move to ``flags''?
>+    nsAutoString coalesce;

Fix comment indent too?

>+nsXULTemplateBuilder::DetermineMatchedRule(nsIContent* aContainer,
...
>+                                           nsTemplateQuerySet* queryset,

aQuerySet, please.  And why does this return nsresult?  It always returns NS_OK, and no caller checks the retval.

>+            if (! tag || MatchTagForResult(aContainer, aResult, tag)) {

Remove space after '!'.

> nsXULTemplateBuilder::SubstituteTextReplaceVariable(nsXULTemplateBuilder* aThis,
>+        nsIAtom* var = NS_NewAtom(aVariable);
...
>+        NS_RELEASE(var);

  nsCOMPtr<nsIAtom> var = do_GetAtom(aVariable);

and then no need to manually release.

>+nsXULTemplateBuilder::CompileQueries()
>+    if (flags.Find(NS_LITERAL_STRING("dont-test-empty")) >= 0)
>+        mFlags |= eDontTestEmpty;

That's really not a very robust method of parsing a string...  I guess that's what we had before, but please file a followup bug on making this a little stricter?

>+    nsresult rv = mQueryProcessor->InitializeForBuilding(mDB, this, rootnode);
>+    if (NS_FAILED(rv)) return rv;

Return on separate line.

>+            delete (nsTemplateQuerySet *)mQuerySets[q];

Again, I'm not sure that'll compile on all our platforms.

>+nsXULTemplateBuilder::CompileTemplate(nsIContent* aTemplate,
>+        nsINodeInfo *ni = rulenode->GetNodeInfo();
>+        if (! ni)
>+            continue;

On tip NodeInfo() never returns null.

>+        if (! aIsQuerySet && ni->Equals(nsXULAtoms::queryset, kNameSpaceID_XUL)) {

No space after '!', please.

>+            // only create a queryset for those after the second since the
>+            // first one is always created

Should this assert that?  That is, assert that |hasQuerySet || aQuerySet| or something?  And this comment should say "after the first", no?

>+                mQuerySets.AppendElement(aQuerySet);

What if that fails?


>+                        // create a new queryset is one hasn't been created already

s/is/if/

>+                            mQuerySets.AppendElement(aQuerySet);

What if that fails?

>+                    mQuerySets.AppendElement(aQuerySet);

And here.

>+nsXULTemplateBuilder::CompileExtendedQuery(nsIContent* aRuleElement,
>+                    rule->SetTag(NS_NewAtom(tag));

That leaks the atom.  do_GetAtom into a local, and pass that to SetTag, please.

>+            // If the rule compilation failed, then we have to bail.
>+            if (NS_FAILED(rv)) {
>+                delete rule;

But aQuerySet still has a pointer to |rule|, no?  That looks wrong to me

>+nsXULTemplateBuilder::DetermineMemberVariable(nsIContent* aActionElement,
>+    // If the member variable hasn't already been specified, then
>+    // grovel over <action> to find it. We'll use the first one
>+    // that we find in a breadth-first search.

Why breadth-first instead of document order?

>+        nsVoidArray unvisited;

I'd suggest nsCOMArray<nsIContent> or nsTArray<nsIContent*> if you want to avoid refcounting.  Both avoid the casting you ahve to do as it is.

>+            unvisited.RemoveElementAt(0);

This makes things kinda slow if there are a lot of elements (since you have to keep memmoving), but I don't see an obvious way to deal with that.  :(

>+            if (! uri.IsEmpty() && uri[0] == PRUnichar('?')) {

Remove space after '!', please.

>+                *aMemberVariable = NS_NewAtom(uri);

This addrefs...

>+    NS_IF_ADDREF(*aMemberVariable);

And so does that.  So you leak.  I'd just addref in the mMemberVariable not null case, and rely on NS_NewAtom to addref in the other case, and remove this NS_IF_ADDREF.

>+nsXULTemplateBuilder::CompileSimpleQuery(nsIContent* aRuleElement,
>+    nsTemplateRule* rule = new nsTemplateRule(aRuleElement, aRuleElement, aQuerySet);
...
>+    rv = aRuleElement->GetAttr(kNameSpaceID_None, nsXULAtoms::parent, tag);
>+    if (NS_FAILED(rv)) return rv;

That leaks |rule| (if rv is failure).

>+        rule->SetTag(NS_NewAtom(tag));

That leaks the atom.

>+nsXULTemplateBuilder::CompileConditions(nsTemplateRule* aRule,

>+        nsINodeInfo *ni = node->GetNodeInfo();
>+        if (ni->Equals(nsXULAtoms::where, kNameSpaceID_XUL)) {

I doubt you need the |ni| local here:

  if (node->NodeInfo()->Equals(nsXULAtoms::where, kNameSpaceID_XUL)) {

>+nsXULTemplateBuilder::CompileWhereCondition(nsTemplateRule* aRule,

>+    nsAutoString multiple;
>+    aCondition->GetAttr(kNameSpaceID_None, nsXULAtoms::multiple, multiple);
>+    PRBool shouldMultiple = multiple.Equals(NS_LITERAL_STRING("true"));

  PRBool shouldMultiple =
    aCondition->AttrValueIs(kNameSpaceID_None, nsXULAtoms::multiple,
                            nsXULAtoms::_true, eCaseMatters);

>+    if (! shouldMultiple && (value[0] == PRUnichar('?'))) {

Remove space after '!', please.

>+    nsAutoString ignorecase;
>+    aCondition->GetAttr(kNameSpaceID_None, nsXULAtoms::ignorecase, ignorecase);
>+    PRBool shouldIgnoreCase = ignorecase.Equals(NS_LITERAL_STRING("true"));

Again, use AttrValueIs, please.

>+    nsAutoString negate;
>+    aCondition->GetAttr(kNameSpaceID_None, nsXULAtoms::negate, negate);
>+    PRBool shouldNegate = negate.Equals(NS_LITERAL_STRING("true"));

And here.

>+    else if (svar && ! value.IsEmpty()) {

Remove space after '!', please.

>@@ -2259,79 +2041,104 @@

>+            if (attr != nsXULAtoms::id && attr != nsXULAtoms::uri) {


What about the namespace?  Do you really want to skip "id" attributes in random namespaces here?

> nsXULTemplateBuilder::IsActivated(nsIRDFResource *aResource)
>-         entry != nsnull;
>-         entry = entry->mPrevious) {
>+        entry != nsnull;
>+        entry = entry->mPrevious) {

This was indented correctly before...

>-            return PR_TRUE;
>+        return PR_TRUE;

As was this.  Please undo both those changes?

>+nsXULTemplateBuilder::GetResultResource(nsIXULTemplateResult* aResult,
>+    nsCOMPtr<nsIRDFResource> resource;
>+    nsresult rv = aResult->GetResource(getter_AddRefs(resource));
...
>+    *aResource = resource;
>+    NS_IF_ADDREF(*aResource);

How about:

  nsresult rv = aResult->GetResource(aResource);
  if (NS_FAILED(rv)
    return rv;

That is, I see no reason for the local here...

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

>+    CompileTemplate(nsIContent* aTemplate,
>+                    nsTemplateQuerySet* aQuerySet,
>+                    PRBool aIsQuerySet,
>+                    PRInt32* aPriority,
>+                    PRBool* aCanUseTemplate);

Please document this?  Especially what aIsQuerySet and aCanUseTemplate mean?

>+    CompileExtendedQuery(nsIContent* aRuleElement,
>+                         nsIContent* aActionElement,
>+                         nsIAtom* aMemberVariable,
>+                         PRBool aIgnoreCondition,
>+                         nsTemplateQuerySet* aQuerySet);

Document this too?

>+    nsresult 
>+    CompileSimpleQuery(nsIContent* aRuleElement,
>+                       nsTemplateQuerySet* aQuerySet,
>+                       PRBool* aCanUseTemplate);

And this?

>+    CompileWhereCondition(nsTemplateRule* aRule,
>+                          nsIContent* aCondition,
>+                          nsTemplateCondition** aCurrentCondition);

Please document what this does, and what the ownership model for aCurrentCondition is.

>+    DetermineMatchedRule(nsIContent* aContainer,
>+                         nsIXULTemplateResult* aResult,
>+                         nsTemplateQuerySet* queryset,
>+                         nsTemplateRule** aMatchedRule);

Ownership model for aMatchedRule?  I assume this must be a rule from |queryset| (which should be aQuerySet)?  Document that, if so, please.

>+    nsresult GetResultResource(nsIXULTemplateResult* aResult,
>+                               nsIRDFResource** aResource);

Document?

>+    /**
>+     * For the rule network
>+     */
>+    nsVoidArray   mQuerySets;

Could you make this nsTArray<nsTemplateQuerySet> now that we have nsTArray?  Or would that be too much code bloat for little gain?  I just don't like the casting we have to do as things stand....

> public:
>+    nsCOMPtr<nsIAtom> mRefVariable;
>+    nsCOMPtr<nsIAtom> mMemberVariable;
>+    nsDataHashtable<nsISupportsHashKey, nsTemplateMatch*> mMatchMap;

Is there a reason these are public?

>@@ -376,27 +338,41 @@
>+    GetInsertionLocation(nsIXULTemplateResult* aOldResult,
>+                         void** aLocation) = 0;

So what's aLocation usable for?  Should it be documented that this is what you pass to ReplaceMatch's aContext (assuming that's what you use it for)?

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

>+nsXULTemplateQueryProcessorRDF::nsXULTemplateQueryProcessorRDF(void)

>+        CallGetService(kRDFServiceCID, &gRDFService);
>+        CallGetService(kRDFContainerUtilsCID, &gRDFContainerUtils);

What if one of those fails?  Also, I suggest asserting that those pointers are null before doing that, just in case.

>+    mPool.Init("nsXULTemplateQueryProcessorRDF", bucketsizes, 2, 256);
>+
>+    mMemoryElementToResultMap.Init();
>+    mBindingDependencies.Init();
>+    mRuleToBindingsMap.Init();

Or one of these?  It seems to me that this class wants an Init() method...

>+nsXULTemplateQueryProcessorRDF::InitializeForBuilding(nsISupports* aDatasource,
>+    // don't do anything if generation has already been done
>+    if (mGenerationStarted)
>+        return NS_OK;

The API doesn't document that this is what happens.  I think it would make more sense to throw if this is called after generateResults, really, but either way the API should document what the behavior is, I think.

>+static PLDHashOperator
>+DestroyBinding(nsISupports* aKey, RDFBindingSet* aBindingSet, void* aContext)

Why not just make mRuleToBindingsMap be a nsRefPtrHashtable<nsISupportsHashKey, RDFBindingSet>?  Then you could also use GetWeak() on it in a few places, etc.  And you wouldn't need this; just clearing the hashtable would work right.

>+nsXULTemplateQueryProcessorRDF::Done()

Shouldn't this also clear mQueries?  I'm not sure under what conditions the template builder resets mQueriesCompiled to false, but it does happen, and I never see us clearing mQueries.  This method seems like the place to do that to me.

>+nsXULTemplateQueryProcessorRDF::CompileQuery(nsIXULTemplateBuilder* aBuilder,
>+    if (! mSimpleRuleMemberTest && tagname.Equals(NS_LITERAL_STRING("template"))) {

Remove space after '!'?  And use EqualsLiteral, or better yet atom comparison on |content->Tag()|.  If you know for sure that aQueryNode is a XUL content, please assert that; if not, you need to have namespace checks too.

>+        // simplified syntax with no rules
>+        rv = AddDefaultSimpleRules(query, &lastnode);
>+        if (NS_FAILED(rv)) return rv;
>+
>+        query->SetSimple();

So if mSimpleRuleMemberTest is non-null we won't call query->SetSimple() (and in fact will call CompileExtendedQuery)?   That seems wrong to me.

>+    else if (tagname.Equals(NS_LITERAL_STRING("rule"))) {

Again, EqualsLiteral or better yet Tag() compare.

>+        rv = CompileSimpleQuery(query, content, &lastnode);
>+        query->SetSimple();

Why do we early return in the no rules case but not here?

>+    if (NS_FAILED(rv) || ! lastnode) {
>+        delete query;
>+        return rv;

Can it happen that rv is success and there is no lastnode?

>+    nsInstantiationNode* instnode = new nsInstantiationNode(this, query);
>+    if (! instnode) {
>+        delete query;

Do we not need to clean up lastnode here?  I guess random stuff hanging out in mAllTests is ok on error...  Also, I'd make |query| an nsRefPtr.  That way you don't have to worry about cleaning it up.

>+    lastnode->AddChild(instnode);
>+    mAllTests.Add(instnode);
>+
>+    mQueries.AppendObject(query);

All three of those can fail...

>+nsXULTemplateQueryProcessorRDF::GenerateResults(nsISupports* aDatasource,
>+    nsXULTemplateResultSetRDF* results = nsnull;

Hmm...  So nsXULTemplateResultSetRDF is a refcounted class.  But earlier on I recall there being explicit |delete| calls on this class... Not a good idea, in my opinion -- there is a good risk of deleting something someone else holds a ref to.  Please store an nsRefPtr<nsXULTemplateResultSetRDF> in nsRDFQuery, and just null it out in UseCachedResults() and Finish().  If you also use an nsRefPtr here, that would all work happily, and you could remove all the manual deleting and actually make use of this nice refcounting business.  ;)

>+#ifdef DEBUG_ndeakin

NSPR logging?  Or is this really really something no one but you would ever care about?

>+                   NS_ConvertUCS2toUTF8(id).get());

UTF16toUTF8.  This applies to a lot of this debug code.

>+nsXULTemplateQueryProcessorRDF::AddBinding(nsIDOMNode* aRuleNode,
>+    // bindings can't be added once result generation has started, otherwise
>+    // the array sizes will get out of sync
>+    if (mGenerationStarted)
>+        return NS_ERROR_FAILURE;

The API docs say "only fatal errors should be thrown".  I think that this restriction here should just be documented in the API.  And NS_ERROR_UNEXPECTED might be a better error.

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

Return on separate line?

>+        bindings->AddRef();
>+
>+        mRuleToBindingsMap.Put(aRuleNode, bindings);

Put() can fail, in which case you've leaked....  Using that nsRefPtrHashtable would help with that, I think.

>+nsXULTemplateQueryProcessorRDF::Propagate(nsIRDFResource* aSource,
>+#ifdef DEBUG_ndeakin
>+        printf("***************************************************\nStart Propagation\n");

NSPR logging?

>+            if (rdftestnode->CanPropagate(aSource, aProperty, aTarget, seed)){

Space before '{'.

>+                livenodes.Add(rdftestnode);


What if Add fails?

>+                InstantiationSet* instantiations = new InstantiationSet();
>+                instantiations->Append(seed);
>+
>+                rv = rdftestnode->Constrain(*instantiations);
>+                if (NS_FAILED(rv)) return rv;

Doesn't that leak on failure?  And please return on a separate line.

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

Return on separate line.

>+nsXULTemplateQueryProcessorRDF::Retract(nsIRDFResource* aSource,
>+#ifdef DEBUG_ndeakin

NSPR logging?

>+nsXULTemplateQueryProcessorRDF::SynchronizeAll(nsIRDFResource* aSource,
>+    nsCOMPtr<nsIMutableArray> results;
>+    if (! mBindingDependencies.Get(aSource, getter_AddRefs(results)))

Why not use GetWeak() here?

That said, I'm not sure why mBindingDependencies is not a nsClassHashtable<nsISupportsHashKey, nsCOMArray<nsXULTemplateResultRDF>>.  It seems like that would fit he use you're making of it better -- you never need anyone but the hashtable to own the array, and you will be storing one and only one type of stuff in the array.  And since nsXULTemplateResultRDF has unique inheritance from nsISupports, nsCOMArray should work with it...

>+        results->QueryElementAt(r, NS_GET_IID(nsIXULTemplateResult),
>+                                getter_AddRefs(result));

do_QueryElementAt if the nsCOMArray thing doesn't work out for some odd reason.

>+nsXULTemplateQueryProcessorRDF::CheckContainer(nsIRDFResource* aResource,

>+    if (aIsContainer)
>+        *aIsContainer = isContainer;

Is that ever null?  Shouldn't this method just bail early if it is?  I'd check whether it ever is, though (in a followup bug, perhaps).

>+nsXULTemplateQueryProcessorRDF::CompileExtendedQuery(nsRDFQuery* aQuery,

>+    mAllTests.Add(idnode);

This can fail...

>+        if ((condition->Tag() == nsXULAtoms::content) && ! i) {

Remove the extra parens and space after '!'?  And document why for i == 0 this is not desireable?

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

Return on separate line?

>+            prevnode->AddChild(testnode);

That can fail.

>+nsXULTemplateQueryProcessorRDF::CompileTripleCondition(nsRDFQuery* aQuery,
>+    aCondition->GetAttr(kNameSpaceID_None, nsXULAtoms::subject, subject);
...
>+    if (subject[0] == PRUnichar('?'))

Are we guaranteed by something elsewhere in this code that |subject| is nonempty?  If not, this looks bad...  I know you just copied this code, but still.

>+    aCondition->GetAttr(kNameSpaceID_None, nsXULAtoms::predicate, predicate);
...
>+    if (predicate[0] == PRUnichar('?')) {

Same.

>+    aCondition->GetAttr(kNameSpaceID_None, nsXULAtoms::object, object);
...
>+    if (object[0] == PRUnichar('?')) {

And here.

>+    mRDFTests.Add(testnode);
>+    mAllTests.Add(testnode);

Those can fail and then we leak, no?

>+nsXULTemplateQueryProcessorRDF::CompileMemberCondition(nsRDFQuery* aQuery,

>+    aCondition->GetAttr(kNameSpaceID_None, nsXULAtoms::container, container);
>+    if (container[0] != PRUnichar('?')) {

Again, what if it's empty?

>+    aCondition->GetAttr(kNameSpaceID_None, nsXULAtoms::child, child);
>+    if (child[0] != PRUnichar('?')) {

And here.

>+    mRDFTests.Add(testnode);
>+    mAllTests.Add(testnode);

And these can fail

>+nsXULTemplateQueryProcessorRDF::AddDefaultSimpleRules(nsRDFQuery* aQuery,
>+    if (! membernode)
>+        return NS_ERROR_OUT_OF_MEMORY;

This leaks idnode.

>+    idnode->AddChild(membernode);
>+    mRDFTests.Add(membernode);
>+
>+    mAllTests.Add(idnode);
>+    mAllTests.Add(membernode);

Add() can fail (out of memory).  This code should probably try to deal.  Same for AddChild().

Would the error handling in this function and the other functions that create TestNodes be any easier with some nsAutoPtr usage?  It seems like a pain as things stand...

>+nsXULTemplateQueryProcessorRDF::CompileSimpleQuery(nsRDFQuery* aQuery,

>+        rv = AddDefaultSimpleRules(aQuery, &parentNode);
>+        if (NS_FAILED(rv)) return rv;

Return on separate line?

>+        if ((attr.get() == nsXULAtoms::property) && (attrNameSpaceID == kNameSpaceID_RDF))

I know you just moved this code, but ditch the extra parens, and there's no need to call .get() on nsCOMPtr<nsIAtom> to compare it to nsIAtom* anymore.  Same elsewhere in this function.

>+            rv = aQueryElement->GetAttr(kNameSpaceID_None, nsXULAtoms::iscontainer, value);

Note that GetAttr() no longer returns nsresult (as you probably noticed when you had to merge your other changes to sicking's GetAttr() changes).  Please check over the GetAttr calls in this patch to make sure they're ok?  I'll do the same once you post an updated patch.

>+            mRDFTests.Add(testnode);
>+            mAllTests.Add(testnode);

Those can fail.

>+            mRDFTests.Add(testnode);
>+            mAllTests.Add(testnode);

And those...

>+                prevnode->AddChild(testnode);

And that.

>+nsXULTemplateQueryProcessorRDF::GetBindingsForRule(nsIDOMNode* aRuleNode)
>+    RDFBindingSet* bindings = nsnull;
>+    mRuleToBindingsMap.Get(aRuleNode, &bindings);
>+    return bindings;

This could be a singleline:

   return mRuleToBindingsMap.GetWeak(aRuleNode);

if mRuleToBindingsMap were an nsRefPtrHashtable.

>+nsXULTemplateQueryProcessorRDF::AddMemoryElements(const Instantiation& aInst,
>+        nsCOMPtr<nsIMutableArray> arr;
>+        mMemoryElementToResultMap.Get(hash, getter_AddRefs(arr));

I think mMemoryElementToResultMap should also be an nsClassHashtable<nsISupportsHashKey, nsCOMArray<nsXULTemplateResultRDF>> or maybe nsIXULTemplateResult depending on whether you can change the signature of AddMemoryElements and RemoveMemoryElements to take nsXULTemplateResultRDF (which I think you can).  It'd make this code a lot cleaner, I suspect.

>+nsXULTemplateQueryProcessorRDF::RetractElement(const MemoryElement& aMemoryElement)
>+            arr->QueryElementAt(r, NS_GET_IID(nsIXULTemplateResult),
>+                                getter_AddRefs(result));

do_QueryElementAt if you can't switch to nsCOMArray.

>+                // because the memory elements are hashed by an integer,
>+                // sometimes two different memory elements will have the same
>+                // hash code. In this case we check the result to make sure
>+                // and only remove those that refer to that memory element.

Might be worth hashing on something unique in a followup bug...

>Index: content/xul/templates/src/nsXULTemplateQueryProcessorRDF.h
>+     * Compute the containment properties which are additional arcs which
>+     * indicate that a node is a container, in additional to the RDF container
>+     * tests.

Might be nice to indicate what happens to the result of the computation.

>+     * Compile a query that uses the extended template syntax.

Document who/what owns aLastNode?  Same for other methods in this class that have a TestNode** out param.

>+    nsresult
>+    AddMemoryElements(const Instantiation& aInst,
>+                      nsIXULTemplateResult* aResult);
>+    nsresult
>+    RemoveMemoryElements(const Instantiation& aInst,
>+                         nsIXULTemplateResult* aResult);
>+    void RetractElement(const MemoryElement& aMemoryElement);

Document?

>+    nsIRDFDataSource* DataSource() { return mDB; }
>+    nsIXULTemplateBuilder* Builder() { return mBuilder; }

Are those guaranteed non-null?  If not, use the Get prefix, please.

>+    nsResourceSet& GetContainmentProperties() { return mContainmentProperties; }

And this, on the other hand, should just be ContainmentProperties(), imo.

>+     * All of the RDF tests in the rule network, which are checked when a new
>+     * assertion is added to the graph

I think it's worth explicitly saying that this is a subset of mAllTests.

>Index: content/xul/templates/src/nsXULTemplateResultRDF.cpp

>+// XXXndeakin for some reason, making this class have classinfo breaks trees.
>+//#include "nsIDOMClassInfo.h"

File a followup, please (at least if we _do_ want to give it classinfo).

>+NS_IMPL_ADDREF(nsXULTemplateResultRDF)
>+NS_IMPL_RELEASE(nsXULTemplateResultRDF)
>+
>+NS_INTERFACE_MAP_BEGIN(nsXULTemplateResultRDF)
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)
>+  NS_INTERFACE_MAP_ENTRY(nsIXULTemplateResult)
>+NS_INTERFACE_MAP_END

Why not just NS_IMPL_ISUPPORTS1(nsXULTemplateResultRDF, nsIXULTemplateResult) ?

>+nsXULTemplateResultRDF::GetIsContainer(PRBool* aIsContainer)

>+        nsXULTemplateQueryProcessorRDF* processor = Processor();
>+        if (processor)

If it can return null, please name it GetProcessor ?

>+    aId.Assign(NS_ConvertUTF8toUCS2(uri));

  CopyUTF8ToUTF16(uri, aId);


>Index: content/xul/templates/src/nsXULTemplateResultRDF.h

>+ * An single result of an query on an RDF graph

"A single"

>Index: content/xul/templates/src/nsXULTemplateResultSetRDF.cpp

>+nsXULTemplateResultSetRDF::HasMoreElements(PRBool *aResult)
>+    if (! mInstantiations || ! mQuery) {

Remove space after '!'?

>+        if (mCurrent) {
>+            mCurrent = mCurrent->mNext;

Er... HasMoreElements() needs to be idempotent.  This isn't, unless I'm badly misunderstanding something.

>+            *aResult = ! mInstantiations->Empty();

Remove space after '!'?

>+nsXULTemplateResultSetRDF::GetNext(nsISupports **aResult)
>+    nsXULTemplateResultRDF* nextresult =
>+        new nsXULTemplateResultRDF(mQuery, mCurrent->mInstantiation, mResource);
...
>+    mProcessor->AddMemoryElements(mCurrent->mInstantiation, nextresult);

Passing an XPCOM object with zero refcount around is a really bad idea.  Please use an nsRefPtr for nextresult!

>Index: content/xul/templates/src/nsXULTreeBuilder.cpp

> nsXULTreeBuilder::ToggleOpenState(PRInt32 aIndex)

>+    if (result && (result != mRootResult)) {

Remove the extra parens, please.

>+        nsresult rv = result->GetMayProcessChildren(&mayProcessChildren);
>+        if (NS_FAILED(rv) || ! mayProcessChildren) return rv;

Remove space after '!', and return on a separate line, please.

>+nsXULTreeBuilder::HasGeneratedContent(nsIRDFResource* aResource,

>+    if ((aResource == rootresource) ||
>+        (mRows.FindByResource(aResource) != mRows.Last()))

Please remove the extraneous parens.

>+nsXULTreeBuilder::GetInsertionLocation(nsIXULTemplateResult* aResult,
>+    if (NS_FAILED(rv) || ref.IsEmpty()) return PR_FALSE;

Return on new line.

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

And here.

>+nsXULTreeBuilder::ReplaceMatch(nsIXULTemplateResult* aOldResult,
>+        nsresult rv = result->GetBindingAsStringFor(mRefVariable, ref);
>+        if (NS_FAILED(rv) || ref.IsEmpty()) return rv;

And here.

>+        rv = gRDFService->GetUnicodeResource(ref, getter_AddRefs(container));
>+        if (NS_FAILED(rv)) return rv;

And here.

> nsXULTreeBuilder::RebuildAll()
>+        rv = mQueryProcessor->TranslateRef(mDB, ref, getter_AddRefs(mRootResult));
>+        if (NS_FAILED(rv)) return rv;

And here.

> nsXULTreeBuilder::GetTemplateActionRowFor(PRInt32 aRow, nsIContent** aResult)
>+        nsXULContentUtils::FindChildByTag(action, kNameSpaceID_XUL, nsXULAtoms::treechildren, getter_AddRefs(children));

Line-break at 80 chars, please?  And same for the next couple of lines?

>+nsXULTreeBuilder::OpenSubtreeForQuerySet(nsTreeRows::Subtree* aSubtree,

A lot of the comments from nsXULContentBuilder::CreateContainerContentsForQuerySet apply here... I'll probably repeat some, but might miss some too.

>+    nsresult rv = mQueryProcessor->GenerateResults(mDB, aResult,
>+                                                   aQuerySet->mCompiledQuery,
>+                                                   getter_AddRefs(results));
>+    if (NS_FAILED(rv)) return rv;

Return on new line.

> +    results->HasMoreElements(&hasMoreResults);

Check rv?

>+        nsTemplateMatch *newmatch =
>+            nsTemplateMatch::Create(mPool, aQuerySet, nextresult);

What if this fails?

>+        rv = GetResultResource(nextresult, getter_AddRefs(resultid));
>+        if (NS_FAILED(rv)) return rv;

Return on new line.

>+        if (! resultid) {
>+            results->HasMoreElements(&hasMoreResults);
>+            continue;

This loop might be better off as a for() loop....  And doesn't that leak newmatch?

>+            if (cyclic) {
>+                NS_WARNING("tree cannot handle cyclic graphs");
>+                continue;

And doesn't this also leak newmatch?

>+                rv = newmatch->RuleMatched(aQuerySet, matchedrule, nextresult);
>+                if (NS_FAILED(rv)) return rv;

And this also, on failure?  And please return on separate line.

>+                if (isOpen)
>+                    open.AppendElement((void*) count);

NS_INT32_TO_PTR?

>+            mMatchMap.Put(resultid, newmatch);

What if that fails?

>+        results->HasMoreElements(&hasMoreResults);

Check rv?

>+nsXULTreeBuilder::CloseContainer(PRInt32 aIndex)
>+nsXULTreeBuilder::IsContainerOpen(nsIXULTemplateResult *aResult, PRBool* aOpen)
>+    if ((mFlags & eDontRecurse) && (aResult != mRootResult)) {

No need for parens around the != test.

This is really good stuff!  I'd like to see an updated patch with the comments addressed, but if we get to mid-December (so I'm gone) and you've addressed the comments, go ahead and land -- we can sort out any remaining issues when I get back.
Attachment #185839 - Flags: review?(bzbarsky) → review+
Blocks: 305981
Blocks: 321169
Blocks: 321170
Blocks: 321174
Blocks: 321175
Blocks: 321176
Blocks: 321177
Blocks: 321178
Blocks: 321179
Blocks: 321180
Blocks: 321182
Blocks: 321184
Blocks: 321091
Updated patch addressing previous comments. bz, feel free to review this, or I can check in if desired. Follow up bugs are filed for additional issues that can be addressed next.

I would appreciate if someone would test on Windows at least building since I can't seem to get a build going there for some reason. And on Mac of course. There's some testcases at http://xulplanet.com/ndeakin/xul/xultemplatetests.xpi
Attachment #207467 - Flags: review?(bzbarsky)
Will try to get this done by end of week.

Martijn, I hate piling more stuff on you, but could you test on Windows?  Or do you know someone who can?
I'll try to fire off a windows build tomorrow, and may be able to cover the mac as well.
CC-ing some people who also might be interested in trying out this special windows build.
I am building on Windows right now. Will post the outcome.

If there are any particular test cases you want me to try out, please post here.
Attachment 207467 [details] [diff] applies and builds fine on Windows. Trunk build updated today.
"Me too!". Attachment 207467 [details] [diff] builds fine on Windows, using msvc7.1.
I've rebuild my debug build, (after a distclean) with the latest patch here ("templates patch with comments addressed"). The build also contains the "frame display lists" patch (bug 317375).
At starting up, I get this assertion 6 times:
###!!! ASSERTION: nsTHashtable was not initialized properly.: 'mTable.entrySize'
, file ../../../../dist/include/xpcom/nsTHashtable.h, line 246
Every time I open a new window, or open the extensions or themes window, I get this assertion.

When trying to install an extension, I crash.
When running with gdb, I crash on startup:
http://wargers.org/bt.txt
http://www.croczilla.com/svg/samples/xulsvg1/xulsvg1.xul
This doesn't show anything with my debug build with the patch, while regular trunk builds show various triangles.
Blocks: 307858
Comment on attachment 207467 [details] [diff] [review]
templates patch with comments addressed

OK, there some issues with this patch so I'll fix them first
Attachment #207467 - Attachment is obsolete: true
Attachment #207467 - Flags: review?(bzbarsky)
The extension manager crash is actually a bug in the extension manager and/or RDF - bug 295927 and a followup bug to that which I can't remember if I filed or not. I'll also change the template builder to not expose this bug.
This patch is similar to the one from two weeks ago except it also has the following:

- fixes leak of InstantiationSets
- fix leak of RDFBindingSet
- change nsXULContentBuilder::SetContainerAttrs to not check empty state for non-containers, avoiding crash
- fix crash in nsXULTemplateQueryProcessorRDF::RetractElement
- fix hashtable assertions (add extra if-clause in Rebuild functions)
- fix issue with generating non-XUL in nsXULContentBuilder::MayGenerateResult
- add back code for bug 242467 which was inadvertantly removed
- allow <content uri="?blah"> with variables other than ?uri to work. Not fixing this would cause too many regressions. Mainly involves adding the function nsXULTemplateBuilder::DetermineRDFQueryRef. This should fix the SVG testcase.

I never saw the crashes on my Linux build, but this patch should fix the crashes others were seeing.
Attachment #208595 - Flags: review?(bzbarsky)
Ok, I've tried the latest patch and everything seems to work fine with it, I can install extensions without crashing and the SVG testcase works again with it.
I think I found two bugs:
- When dragging a bookmark in somewhere, all the bookmark folders beneath it are doubled, and they all become empty (restart of Firefox fixes it).
http://wargers.org/mozilla/bug285631/285631_moving_bookmark_bug.htm
Adding a bookmark in a folder makes the bookmark folder icon disappear, instead you get the 'blank' icon.
And in the Bookmarks Manager, the View->Sorted by.. (sorted by name, description, etc) does not work anymore.
I get far too much to see in the bookmarks manager, it seems like I see all the bookmarks in the root folder, including those I only should see in the subfolders.

When I drag a link from one place to another I get this assertion (not sure if it is related to the patch, though):
###!!! ASSERTION: row count changed unexpectedly: 'mRowCount == rowCount', file
c:/mozilla/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 2254

When I add a search engine to Firefox, it gets added under "Add Engines" menu-item. When I close the search popup and open it again, the newly added engine disappeares and instead I see an extra separator at the bottom. Restarting Firefox again fixes it.
(In reply to comment #47)
> When I add a search engine to Firefox, it gets added under "Add Engines"
> menu-item. When I close the search popup and open it again, the newly added
> engine disappeares and instead I see an extra separator at the bottom.
> Restarting Firefox again fixes it.

This is actually the current behaviour, assuming you manage to open the menu fast enough after adding another search engine (hard, but possible).  The reason is that the separator and the Add Engines item are added programatically in the on-popup-showing handler for the popup menu; when the menu is closed, they are removed by the opposite menu handler.  The reason why you see an extra separator is because the popup hiding handler just removes the last two items -- which, in this case, will be "Add Engines" and the new search engine.  The separator stays in the menu, and the next time the menu is shown, the separator and Add Engines are added (but not the new engine).

The template builder doesn't know anything about the separator and Add Engins, and assumes (quite rightly) that it's generating the whole menu... hence it sticks the new item at the end, which happens to be after the two inserted items at the end.  What's really needed is some sort of element that can be placed inside a menu and that can contain a bunch of <menuitem>s that the menu will just pretend are in it (without the intervening element).  That way it would be possible to have something like:

<menuitem label="Foo"/>
<menuseparator/>
<menuitem-container-thing template"...."/>
<menuseparator/>
<menuitem label="Add Engines..."/>

A lot of code does the adding/removing of items in popup showing and hiding, and it sort of sucks; we need a more robust solution for that.  It sounds like the template builder fixes here just tickle that broken situation more often.
(In reply to comment #47)
> When I drag a link from one place to another I get this assertion (not sure if
> it is related to the patch, though):
> ###!!! ASSERTION: row count changed unexpectedly: 'mRowCount == rowCount', file
> c:/mozilla/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 2254
Ok, this also doesn't seem to happen because of this patch, it is "normal" behavior for a debug build.

(In reply to comment #49)
>(In reply to comment #47)
>>###!!! ASSERTION: row count changed unexpectedly: 'mRowCount == rowCount', file
>>c:/mozilla/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp, line 2254
>is "normal" behavior for a debug build.
This is actually a real assertion. It means that someone did something wrong somewhere. In particular a release build will show an incorrect number of rows.
Blocks: 325213
Comment on attachment 208595 [details] [diff] [review]
Fix crashes and assertions

>Index: content/xul/content/src/nsXULAtomList.h

The changes here will need to move to nsGkAtomList.h, right?

>Index: content/xul/templates/src/nsInstantiationNode.cpp
>+nsInstantiationNode::Propagate(InstantiationSet& aInstantiations,

>+                    nsXULTemplateResultRDF* nextresult =
>+                        new nsXULTemplateResultRDF(mQuery, *inst, resource);

I'd use an nsRefPtr<nsXULTemplateResultRDF> here, just in case AddMemoryElements fails.  Speaking of which, shouldn't you handle it failing?

>Index: content/xul/templates/src/nsRDFBinding.cpp
>+nsBindingValues::SetBindingSet(RDFBindingSet* aBindings)

>+        for (--count; count >= 0; count--)
>+            mValues[count] = nsnull;

Why not just memset?  Though note my comment on the type of mValues later on.

>Index: content/xul/templates/src/nsRDFBinding.h

The constructor and destructor of RDFBinding never ended up with MOZ_COUNT_CTOR and MOZ_COUNT_DTOR.  Please add those.

Also RDFBindingSetL::AddRef and RDFBindingSet::Release never ended up with the NS_LOG_ADDREF and NS_LOG_RELEASE calls I asked for.  Please add those too.

>+class nsBindingValues

>+    nsIRDFNode** mValues;

OK.  So given that we do want to keep the count in mBindings, why not make this an |nsCOMPtr<nsIRDFNode>* mValues|?

That way you don't have to do a lot of that manual refcounting (or manual setting to zero, for that matter) that you're doing now -- the nsCOMPtr constructors and destructors will handle it for you.

>Index: content/xul/templates/src/nsRDFQuery.h
>+    NS_DECLARE_STATIC_IID_ACCESSOR(NS_ITEMPLATERDFQUERY_IID)

You also need a NS_DEFINE_STATIC_IID_ACCESSOR outside the class decl.  Please add that.

>+    virtual void GetQueryNode(nsIDOMNode** aQueryNode) = 0;

Might want to make this return already_AddRefed<nsIDOMNode> or even a non-addrefed nsIDOMNode*.  Separate bug, though.

>+    // If successful, this method takes ownership of aInstantiations.
>+    nsresult SetCachedResults(nsXULTemplateQueryProcessorRDF* aProcessor,
>+                              const InstantiationSet& aInstantiations);

So I just realized we're passing in a const ref here... Generally speaking, that makes the ownership thing pretty weird.  If we're passing around owning references to InstantiationSet, then we should be passing around pointers.  Again, followup bug, since this would be a pretty pervasive change.  On the other hand, I think it would make the ownership model for instantiation sets easier to follow.

>Index: content/xul/templates/src/nsRuleNetwork.cpp
>+TestNode::Propagate(InstantiationSet& aInstantiations,

>+    if (! aInstantiations.Empty()) {

And if it is?  For example, if shouldCopy is false and aInstantiations.IsEmpty() we will never set aMatched.  Is that correct?  Seems wrong to me.

>+                if (!instantiations)
>+                    return NS_ERROR_OUT_OF_MEMORY;

Note that it's not like callers check the return value of Propagate(); even this method doesn't.  Does everything still work right with the ownership model for instantiation sets given that assumption?

The more I think about it, the more I think that the right approach is to pass around an nsAutoPtr<InstantiationSet>& or something and have callees explicitly call forget() on it when they take ownership.  I guess this should be a separate bug.

>+    // if a copy of the instantiations was made, then pretend it didn't match
>+    // even if it did, since the match would have taken ownership over the
>+    // copy instead of the original and we want to make sure that the caller
>+    // deletes the instantiations properly.
>+    if (shouldCopy)
>+        aMatched = PR_FALSE;

So is aMatched used ONLY to determine whether to delete the InstantiationSet?  If so, it needs a better name.  If not, then this overloading looks wrong to me and we need to split out control over InstantiationSet ownership into a separate boolean or something.

>Index: content/xul/templates/src/nsTemplateRule.h
>+class nsTemplateCondition
>+{
>+public:
>+    // relations that may be used in a rule. They may be negated with the
>+    // negate flag. Less and Greater are used for numeric comparisons and
>+    // Before and After are used for string comparisons. For Less, Greater,
>+    // Before, After and Contains, comparisons are done relative to the
>+    // source, that is match if the source contains the target.
>+    enum ConditionRelation {

What about StartsWith/EndsWith?  Shouldn't those be listed in that comment too?

I'd maybe say something more like:

    // For Less, Greater, Before, After, Startswith, Endswith, and Contains,
    // the source is conceptually on the left of the relation and the target
    // is on the right.  For example, if the relation is Contains, that means
    // Match if the source contains the target.

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

> nsXULContentBuilder::CreateTemplateAndContainerContents(nsIContent* aElement,

>+#ifdef PR_LOGGING
>+    if (PR_LOG_TEST(gXULTemplateLog, PR_LOG_DEBUG)) {
>+        PR_LOG(gXULTemplateLog, PR_LOG_ALWAYS,
>+               ("nsXULContentBuilder::CreateTemplateAndContainerContents start - flags: mFlags",
>+                mFlags));
>+    }
>+#endif

You don't need the #ifdef or the if() check here.  just pass PR_LOG_DEBUG as the log level to PR_LOG and it'll do the right thing.  Also, this doesn't actually log mFlags.  It logs the string "mFlags".  There should be a %d there or something?

>+#ifdef PR_LOGGING
>+    if (PR_LOG_TEST(gXULTemplateLog, PR_LOG_DEBUG)) {
>+        PR_LOG(gXULTemplateLog, PR_LOG_ALWAYS,
>+               ("nsXULContentBuilder::CreateTemplateAndContainerContents end"));
>+    }
>+#endif

Same as at the start of the method.

>@@ -1175,154 +1237,218 @@ nsXULContentBuilder::CreateContainerCont

>+    for (PRInt32 r = 0; r < querySetCount; r++) {
>+        nsTemplateQuerySet* queryset = mQuerySets[r];
>+        CreateContainerContentsForQuerySet(aElement, aResult, aNotify, queryset,
>+                                          aContainer, aNewIndexInContainer);

Again, it's not obvious to me why this is OK -- that is, why we won't leak the *aContainer from earlier calls in the loop.  The reasons should be documented either here or where we declare CreateContainerContentsForQuerySet.

>+nsXULContentBuilder::MayGenerateResult(nsIXULTemplateResult* aResult,

>+            if (xulcontent) {
>+                if (xulcontent->GetLazyState(nsXULElement::eContainerContentsBuilt))
>+                    *aLocation = content;
>+            }
>+            else {
>+                // non-XUL is never built lazily
>+                *aLocation = content;

How about:

   if (!xulcontent || xulcontent->GetLazyState(nsXULElement::eContainerContentsBuilt)) {
     // non-XUL content is never built lazily, nor is content that's
     // already been built
     *aLocation = content;
   }

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

>+nsXULTemplateBuilder::UpdateResult(nsIXULTemplateResult* aOldResult,

>+#ifdef PR_LOGGING
>+    if (PR_LOG_TEST(gXULTemplateLog, PR_LOG_DEBUG)) {
>+        PR_LOG(gXULTemplateLog, PR_LOG_ALWAYS,
>+               ("nsXULTemplateBuilder::UpdateResult %p %p %p",
>+               aOldResult, aNewResult, aQueryNode));
>+    }
>+#endif

Again, no need for the two outer tests -- just pass PR_LOG_DEBUG to PR_LOG as the level.

>+    // get the container where content should be inserted. If
>+    // GetInsertionLocation returns false, the container hasn't generated

I don't see a GetInsertionLocation anywhere in this patch....  Should this be talking about MayGenerateResult?

>+    if (aOldResult || acceptedmatch)
>+        rv = ReplaceMatch(aOldResult, acceptedmatch, matchedrule, insertionPoint);
>+
>+    if (removedmatch) {
...
>+    }
>+
>     return NS_OK;

Is there a reason we're ignoring the rv from ReplaceMatch?

>+nsXULTemplateBuilder::AddListener(nsIXULBuilderListener* aListener)

>+    nsresult rv = mListeners.AppendObject(aListener);
>+    if (!rv)

AppendObject returns a PRBool, not an nsresult.

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

>+     * Compile the a <where> tag in a condition. The caller should set
>+     * aCurrentCondition to null for the first condition.
...
>+    CompileWhereCondition(nsTemplateRule* aRule,

You mean *aCurrentCondition in the comment, right?

>+     * @param aMatchedRule rule that has matched, or null if any.
>+     */
>+    nsresult
>+    DetermineMatchedRule(nsIContent* aContainer,

Perhaps:

  @param aMatchedRule [out] ...

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

>+nsXULTemplateQueryProcessorRDF::InitializeForBuilding(nsISupports* aDatasource,

>+        rv = mPool.Init("nsXULTemplateQueryProcessorRDF", bucketsizes, 2, 256);

What if it's already inited (eg if we got past here on our last InitializeForBuilding and then failed to init one of the hashtables)?  Make sure this code deals with that ok?

>+        if (!mMemoryElementToResultMap.Init())
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        if (!mBindingDependencies.Init())
>+            return NS_ERROR_OUT_OF_MEMORY;
>+        if (!mRuleToBindingsMap.Init())
>+            return NS_ERROR_OUT_OF_MEMORY;

Same for all of these; I suspect you want something like:

  if (!mMemoryElementToResultMap.IsInitialized() &&
      !mMemoryElementToResultMap.Init())
      return NS_ERROR_OUT_OF_MEMORY;

and same for the others.

>+nsXULTemplateQueryProcessorRDF::CompileQuery(nsIXULTemplateBuilder* aBuilder,

>+    rv = mAllTests.Add(instnode);
>+    if (NS_FAILED(rv))
>+        return rv;

Doesn't that leak instnode?

>+nsXULTemplateQueryProcessorRDF::GenerateResults(nsISupports* aDatasource,

>+    *aResults = results;
>+    NS_IF_ADDREF(*aResults);

Just do results.swap(*aResults) here (since you know *aResults is null).  No need for manual refcounting then, and saves you an addref/release pair.

>+nsXULTemplateQueryProcessorRDF::AddBinding(nsIDOMNode* aRuleNode,
>+    RDFBindingSet* bindings = mRuleToBindingsMap.GetWeak(aRuleNode);
...
>+        bindings = new RDFBindingSet();
...
>+        if (!mRuleToBindingsMap.Put(aRuleNode, bindings))
>+            return NS_ERROR_OUT_OF_MEMORY;

That leaks bindings.  Why not make bindings a nsRefPtr<RDFBindingSet> here and not have to worry about leaks?

Then you could even combine the two out-of-memory checks easily:

   if (!bindings || !mRuleToBindingsMap.Put(aRuleNode, bindings))
       return NS_ERROR_OUT_OF_MEMORY;

>+nsXULTemplateQueryProcessorRDF::Propagate(nsIRDFResource* aSource,

>+                InstantiationSet* instantiations = new InstantiationSet();
>+                instantiations->Append(seed);

This needs to handle out of memory.

>+nsXULTemplateQueryProcessorRDF::AddDefaultSimpleRules(nsRDFQuery* aQuery,

>+    // add nodes to mAllTests first. If later calls fail, just leave them in
>+    // the list so that they can be deleted later.
>+    nsresult rv = mAllTests.Add(idnode);
>+    if (NS_FAILED(rv)) {
>+        delete idnode;

You need to delete membernode here too, don't you?

With those fixed, r+sr=bzbarsky.  No need for more reviews  from me here -- just fix those issues up and land this, please.

Thanks again for your patience, and I'm sorry this took so long _again_.  :(
Attachment #208595 - Flags: superreview+
Attachment #208595 - Flags: review?(bzbarsky)
Attachment #208595 - Flags: review+
Blocks: 326712
Bug 326712 is for the bookmarks window regressions that Martijn mentions in comments 45, 46 and 47. I will have a patch for that ready not long after the patch for this bug is checked in
>+        for (PRInt32 count = mBindings->Count() - 1; count >= 0; count--) {
>+            mValues[count]= nsnull;
>+        }
>+
>+        delete [] mValues;

Why bother with the loop?  Using delete[] will call t he nsCOMPtr destructors, right?  Which will do the right thing here, I think.
checked in
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
This checkin led to a small Ts/Txul hit (on comet, luna, monkey, which are the tinderboxen that actually have useful measurements).

Might be worth filing a bug to profile window-open and see whether something jumps out at us...
Could this have caused the assert that makes balsa orange?
And in fact, it probably did, as follows (based on assert stack):

1)  Layout module init inits nsXULContentUtils
2)  This gets the application locale, etc
3)  That causes a read from a properties file to get the locale
4)  That requires resolution of a chrome URI
5)  That looks for the XUL prototype cache
6)  That lives in the layout module, so we reenter layout module init

The real issue, imho, is that reading random stuff from chrome channels depends on layout....  Would it make sense to move the prototype cache out of layout and into the chrome registry or something?
I've never really understood why the chrome channel needs to know anything about the prototype cache at all... why can't that be handled internally in nsXULDoc? To be more specific, why are we opening a chrome channel that we never intend to use?
For now I can move the collation initialization into nsXULContentUtils::GetCollation and see if the issue goes away.
> To be more specific, why are we opening a chrome channel that we never intend to
> use?

Because we don't know we have a XUL doc till we have a MIME type, which we get off an opened channel.

Basically, the XUL cache setup is fundamentally incompatible with the way all other documents work...
Attached patch fix build bustage (obsolete) — Splinter Review
The checkin caused my 64-bit build to fail (gcc4):

../../../../../mozilla/content/xul/templates/src/nsRuleNetwork.h: In member function ‘PLHashNumber nsAssignment::Hash() const’:
../../../../../mozilla/content/xul/templates/src/nsRuleNetwork.h:255: error: cast from ‘nsDerivedSafe<nsIAtom>*’ to ‘PRInt32’ loses precision

The attached patch fixes it.
Attachment #211767 - Flags: review?(enndeakin)
roc: wrong patch?
Attachment #211767 - Attachment is obsolete: true
Attachment #211771 - Flags: review?(enndeakin)
Attachment #211767 - Flags: review?(enndeakin)
Why not use NS_PTR_TO_INT32 there too?
Attachment #211771 - Flags: review?(enndeakin) → review+
Checked in with NS_PTR_TO_INT32.

BTW I don't think that's a very good hash function ... using | tends to lose entropy.
Neil, I suspect this checkin caused a regression, bug 327473 - I'll double check that the build just before this checkin didn't have bug 327473, but I'm pretty sure the builds with it do have the bug.
the 2/13 build is ok, the 2/14 build has bug 327473. 

The account settings UI uses RDF XUL templates - since TB and SM share this code, I would bet it breaks the SM account settings UI as well.
Depends on: 327473
No longer depends on: 327473
Depends on: 327504
In addition to the Account Manager patch David cited, I'm also getting spurious crashes in nsTemplateRule::GetAction when manipulating the folder pane. 

See Bug 327504 for details.
Depends on: 327508
Another thing :)

I'm also getting several assertions on startup and shut down that were introduced with this patch:

NS_ASSERTION(xuldoc, "expected a XUL document");

See Bug 327508
I'm suspecting that this patch has broken the zap [1] ui.

We are using templates with several rules:
<rule>
 <conditions>
   ...
   <triple subject="?node" predicate="foo" object="a"/>
 </conditions>
 ...
</rule>
<rule>
  <conditions>
    ...
    <triple subject="?node" predicate="foo" object="b"/>
  </conditions>
  ...
</rule>

When we have a change in the datasource like
 (node, foo, a) -> (node, foo, b),
the template used to dynamically change from the first rule to the second.
This is now broken. Could this patch be the cause?

[1] http://www.croczilla.com/zap
I think this might have caused bug 327885.
Blocks: 327895
No longer blocks: 326712
Depends on: 326712
No longer blocks: 327895
Depends on: 327895
Depends on: 329040
Depends on: 329335
Depends on: 367310
Flags: in-testsuite-
Blocks: 76525
Can anyone confirm whether or not the numerous documentation updates to the XUL Template Guide the last few days have included coverages of the changes here?
(In reply to comment #72)
> Can anyone confirm whether or not the numerous documentation updates to the XUL
> Template Guide the last few days have included coverages of the changes here?
> 

Yes, and mfinkle offered to update the element reference to match. The only other task is to update the example files in the template guide to match the changes.

Depends on: 417509
mfinkle, are you done with this documentation work?  If not, what still needs doing?
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.widgets
Depends on: 460961
Still waiting to hear from mfinkle on whether this doc is updated. :)
You need to log in before you can comment on or make changes to this bug.