Bug 285631 - Improve XUL Templates
 Summary: Improve XUL Templates
 Status: RESOLVED FIXED dev-doc-complete Core Components XUL (show other bugs) Trunk x86 Linux -- normal with 5 votes (vote) --- Neil Deakin 326712 327504 327508 327895 329040 329335 367310 417509 460961 305981 321178 321182 76525 86435 263166 307858 321091 321169 321170 321174 321175 321176 321177 321179 321180 321184 325213 553807 Show dependency tree / graph

Reported: 2005-03-10 12:15 PST by Neil Deakin
Modified: 2014-04-25 15:16 PDT (History)
45 users (show)
enndeakin: in‑testsuite-
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Attachments
Break template builder into RDF and non-RDF parts (128.68 KB, application/zip)
2005-04-01 20:21 PST, Neil Deakin
no flags Details
More complete templates patch (691.08 KB, patch)
2005-06-09 19:57 PDT, Neil Deakin
bzbarsky: review+
Details | Diff | Splinter Review
IDLs with more detailed comments (35.55 KB, patch)
2005-11-07 19:44 PST, Neil Deakin
bzbarsky: review+
Details | Diff | Splinter Review
2006-01-03 19:30 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Fix crashes and assertions (743.96 KB, patch)
2006-01-15 14:13 PST, Neil Deakin
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
For reference, this is the patch that I will be checking in, no need for additional review (744.89 KB, patch)
2006-02-10 12:33 PST, Neil Deakin
no flags Details | Diff | Splinter Review
fix build bustage (1.72 KB, patch)
2006-02-13 13:23 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
errr.... yeah! this is the right patch! (1.40 KB, patch)
2006-02-13 13:32 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
enndeakin: review+
Details | Diff | Splinter Review

 Neil Deakin 2005-03-10 12:15:20 PST 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? Neil Deakin 2005-04-01 20:21:39 PST Created attachment 179356 [details] Break template builder into RDF and non-RDF parts 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). Myk Melez [:myk] [@mykmelez] 2005-04-27 09:28:45 PDT Note that the patch size limit was recently changed to 1MB. Neil Deakin 2005-06-09 19:57:34 PDT Created attachment 185839 [details] [diff] [review] More complete templates patch - 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. conor 2005-06-09 22:13:45 PDT will this work make it into 1.1? Vladimir Vukicevic [:vlad] [:vladv] 2005-06-15 13:09:41 PDT 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 Vladimir Vukicevic [:vlad] [:vladv] 2005-07-18 16:05:53 PDT 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. Axel Hecht 2005-07-19 01:53:34 PDT 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. Neil Deakin 2005-08-06 11:54:55 PDT 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.  Neil Deakin 2005-08-27 07:04:32 PDT Comment on attachment 185839 [details] [diff] [review] More complete templates patch shaver suggested that bz should take a look too Boris Zbarsky [:bz] 2005-08-28 07:10:14 PDT 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. :( Boris Zbarsky [:bz] 2005-10-09 19:30:18 PDT 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... Neil Deakin 2005-10-09 19:42:31 PDT 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.  Boris Zbarsky [:bz] 2005-10-09 20:03:59 PDT Ah, excellent. Let me read that; I'm hoping to be able to do this review in the next several days. Johnny Stenback (:jst, jst@mozilla.com) 2005-10-28 17:03:13 PDT 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 Boris Zbarsky [:bz] 2005-10-30 16:18:58 PST `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 . >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 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

 Note You need to log in before you can comment on or make changes to this bug.