Closed
Bug 321171
Opened 19 years ago
Closed 18 years ago
Template Query Processor for XML
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha6
People
(Reporter: enndeakin, Assigned: enndeakin)
References
()
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
peterv
:
superreview+
|
Details | Diff | Splinter Review |
Support using XML datasources. Initial code to come soon.
Assignee | ||
Comment 1•19 years ago
|
||
I have this mostly implemented but not going to do anything until bug 335122 is in.
Depends on: 335122
Assignee | ||
Comment 2•19 years ago
|
||
Implements simple support for building template content from XML sources. More functionality can be added later.
bz, let me know if someone else should review this. Also I added a comment where I call CreateDocument as I don't know if that part is the correct way to initialize a document.
Documentation is at http://wiki.mozilla.org/XUL:Templates_with_XML
Testcases are at http://xulplanet.com/ndeakin/tests/xts/templates/xml/
Attachment #229709 -
Flags: superreview?(bzbarsky)
Attachment #229709 -
Flags: review?(bzbarsky)
Comment 3•19 years ago
|
||
I probably won't get to this until at least sometime next week...
Comment 4•18 years ago
|
||
That looks like xforms repeat element :). Neil, do you suppose to provide content generating by using for example @template attribute instead <template/> element? It allows to easy generate content of xul:grid for example.
Assignee | ||
Comment 5•18 years ago
|
||
Comment on attachment 229709 [details] [diff] [review]
Support for XML in templates
let's all reduce bz's review queue
Attachment #229709 -
Flags: superreview?(bzbarsky)
Attachment #229709 -
Flags: superreview?(bugmail)
Attachment #229709 -
Flags: review?(bzbarsky)
Attachment #229709 -
Flags: review?(bugmail)
I'm not really the right person to review template stuff. I can sr though.
Assignee | ||
Comment 7•18 years ago
|
||
(In reply to comment #6)
> I'm not really the right person to review template stuff. I can sr though.
>
Fell free to suggest another person more suitable who doesn't exist then.
Attachment #229709 -
Flags: review?(bugmail)
Assignee | ||
Updated•18 years ago
|
Attachment #229709 -
Flags: review?(Olli.Pettay)
Comment 8•18 years ago
|
||
Could you perhaps update the patch.
3 out of 14 hunks FAILED -- saving rejects to file content/xul/templates/src/nsXULTemplateBuilder.cpp.rej
And some fuzz with other files.
Assignee | ||
Comment 9•18 years ago
|
||
Attachment #229709 -
Attachment is obsolete: true
Attachment #262906 -
Flags: review?(Olli.Pettay)
Attachment #229709 -
Flags: superreview?(jonas)
Attachment #229709 -
Flags: review?(Olli.Pettay)
Comment 10•18 years ago
|
||
Comment on attachment 262906 [details] [diff] [review]
updated patch
Is there a reason to use 4 space indent? IMO we should use 2 spaces in all new files.
>+nsresult
>+XMLBindingSet::AddBinding(nsIAtom* aVar, nsIDOMXPathExpression* aExpr)
>+{
>+ XMLBinding* newbinding = new XMLBinding(aVar, aExpr);
>+ if (! newbinding)
>+ return NS_ERROR_OUT_OF_MEMORY;
I'd prefer |if (!newbinding)| or in this case even
NS_ENSURE_TRUE(newBinding, NS_ERROR_OUT_OF_MEMORY);
>+
>+ if (mFirst) {
>+ XMLBinding* binding = mFirst;
>+
>+ while (binding) {
>+ // if the target variable is already used in a binding, ignore it
>+ // since it won't be useful for anything
>+ if (binding->mVar == aVar)
>+ return NS_OK;
So here you leak newBinding.
>+class XMLBindingSet
...
>+ PRInt32 AddRef() { return ++mRefCnt; }
Could you add NS_LOG_ADDREF here
>+
>+ PRInt32 Release()
>+ {
>+ PRInt32 refcnt = --mRefCnt;
>+ if (refcnt == 0) delete this;
>+ return refcnt;
>+ }
and NS_LOG_RELEASE here. And also un-inline Release() and
use the normal stabilization thing, so something like
--mRefCnt;
NS_LOG_RELEASE(this, mRefCnt, "XMLBindingSet");
if (mRefCnt == 0) {
mRefCnt = 1;
delete this;
return 0;
}
return mRefCnt;
> nsresult
>-nsXULTemplateBuilder::LoadDataSources(nsIDocument* doc)
>+nsXULTemplateBuilder::LoadDataSources(nsIDocument* aDocument, PRBool* aShouldDelayBuilding)
> {
...
>+ if (querytype.IsEmpty())
>+ querytype.AssignLiteral("xml");
use {} here. Makes the code easier to read.
>+ mQueryProcessor = new nsXULTemplateQueryProcessorRDF();
>+ if (! mQueryProcessor)
>+ return NS_ERROR_OUT_OF_MEMORY;
NS_ENSURE_TRUE(mQueryProcessor, NS_ERROR_OUT_OF_MEMORY);
same also elsewhere
>+ if (!mQueryProcessor) {
>+ // XXXndeakin should log an error here
>+ return rv;
>+ }
I think it might be useful to see some error message at least in debug builds, so
maybe using NS_ENSURE_TRUE(mQueryProcessor, rv) would be better here. And then
if better logging is needed, open a new bug for that and put a comment here
// Fix logging, bug xyzpqj.
Or something like that.
>+ // check for magical attributes. XXX move to ``flags''?
>+ nsAutoString coalesce;
>+ mRoot->GetAttr(kNameSpaceID_None, nsGkAtoms::coalesceduplicatearcs, coalesce);
>+ if (coalesce.EqualsLiteral("false"))
>+ mCompDB->SetCoalesceDuplicateArcs(PR_FALSE);
You could use nsIContent::AttrValueIs(...) method here, and also elsewhere.
>+ else {
>+ nsCOMPtr<nsIDOMDOMImplementation> implementation =
>+ do_CreateInstance(kDOMDOMImplementationCID, &rv);
>+ if (NS_FAILED(rv))
>+ return rv;
>+
>+ nsCOMPtr<nsIPrivateDOMImplementation> privImpl =
>+ do_QueryInterface(implementation);
>+ // XXXndeakin is this right?
>+ if (privImpl)
>+ privImpl->Init(docurl, docurl, principal);
>+
>+ nsCOMPtr<nsIDOMDocument> domDocument;
>+ nsAutoString emptyStr;
>+ rv = implementation->CreateDocument(emptyStr,
>+ emptyStr,
>+ nsnull,
>+ getter_AddRefs(domDocument));
Do you really need to do this. Couldn't you get the nsIDOMDOMImplementation from some
element's ownerDocument and use that to create a new document? Or just use nsContentUtils::CreateDocument which nsIDOMDOMImplementation::CreateDocument uses anyway.
>@@ -1880,21 +1996,23 @@ nsXULTemplateBuilder::CompileExtendedQue
...
>+ // allow bindings to be placed directly inside rule
>+ if (!bindings)
>+ bindings = aRuleElement;
check indentation
>+NS_IMETHODIMP
>+nsXULTemplateResultSetXML::GetNext(nsISupports **aResult)
>+{
>+ nsCOMPtr<nsIDOMNode> node;
>+ nsresult rv = mResults->SnapshotItem(mPosition++, getter_AddRefs(node));
So mPosition is always the index of the next item, not the current one?
>+NS_IMETHODIMP
>+nsXULTemplateQueryProcessorXML::InitializeForBuilding(nsISupports* aDatasource,
>+ nsIXULTemplateBuilder* aBuilder,
>+ nsIDOMNode* aRootNode)
>+{
>+ // the datasource is either a document or a DOM element
>+ nsCOMPtr<nsIDOMDocument> doc = do_QueryInterface(aDatasource);
>+ if (doc)
>+ doc->GetDocumentElement(getter_AddRefs(mRoot));
>+ else
>+ mRoot = do_QueryInterface(aDatasource);
Shouldn't you make sure here that mRoot != nsnull ?
So maybe NS_ENSURE_STATE(mRoot);
>+
>+ mEvaluator = do_CreateInstance("@mozilla.org/dom/xpath-evaluator;1");
Check OOM here.
>+nsXULTemplateQueryProcessorXML::GenerateResults(nsISupports* aDatasource,
>+ nsIXULTemplateResult* aRef,
>+ nsISupports* aQuery,
>+ nsISimpleEnumerator** aResults)
>+{
>+ if (!aQuery)
>+ return NS_ERROR_INVALID_ARG;
>+
>+ mGenerationStarted = PR_TRUE;
>+
>+ nsXULTemplateResultXML* refxml = NS_STATIC_CAST(nsXULTemplateResultXML*, aRef);
>+ nsXMLQuery* query = NS_REINTERPRET_CAST(nsXMLQuery*, aQuery);
Can content access nsIXULTemplateQueryProcessor objects?
If yes, then this would create a security bug. I think you need to QI aRef and aQuery to
some internal interface(-like) thing, not cast. And make sure QIing succeeded.
>+NS_IMETHODIMP nsXULTemplateQueryProcessorXML::TranslateRef(nsISupports* aDatasource,
>+ const nsAString& aRefString,
>+ nsIXULTemplateResult** aRef)
>+{
...
>+ if (doc)
>+ doc->GetDocumentElement(getter_AddRefs(rootElement));
>+ else
>+ rootElement = do_QueryInterface(aDatasource);
indentation
>+
>+NS_IMETHODIMP nsXULTemplateQueryProcessorXML::CompareResults(nsIXULTemplateResult* aLeft,
...
>+ *aResult = ::Compare(nsDependentString(leftVal),
>+ nsDependentString(rightVal),
>+ nsCaseInsensitiveStringComparator());
Really CaseInsensitive ? Could you add a comment why.
>+ nsresult
>+ AddBinding(nsIAtom* aVar, nsIDOMXPathExpression* aExpr)
>+ {
>+ if (! mRequiredBindings)
>+ mRequiredBindings = new XMLBindingSet();
>+ if (! mRequiredBindings)
>+ return NS_ERROR_OUT_OF_MEMORY;
>+
if (!mRequiredBindings) {
mRequiredBindings = new XMLBindingSet();
NS_ENSURE_TRUE(mRequiredBindings, NS_ERROR_OUT_OF_MEMORY);
}
>+NS_IMETHODIMP
>+nsXULTemplateResultXML::GetId(nsAString& aId)
>+{
>+ char id[15];
>+ sprintf(id, "row:%d", mId);
>+
>+ aId.Assign(NS_ConvertUTF8toUTF16(id));
>+
>+ return NS_OK;
>+}
Why not
aId.AssignLiteral("row:");
aId.AppendInt(mId);
return NS_OK;
Attachment #262906 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 11•18 years ago
|
||
>
> Can content access nsIXULTemplateQueryProcessor objects?
No, but a separate interface is probably better anyway.
> ...
> >+ *aResult = ::Compare(nsDependentString(leftVal),
> >+ nsDependentString(rightVal),
> >+ nsCaseInsensitiveStringComparator());
>
> Really CaseInsensitive ? Could you add a comment why.
The RDF sorting is done case-insensitive. There was a patch somewhere that added the capability to specify whether case was relevant, but currently sorting is always done case-insensitive.
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•18 years ago
|
||
Attachment #262906 -
Attachment is obsolete: true
Attachment #263628 -
Flags: review?(Olli.Pettay)
Comment 13•18 years ago
|
||
Could you combine classes nsITemplateXMLQuery and nsXMLQuery, the latter
one looks anyway almost like a struct. That way the interface/class
wouldn't have to have virtual methods. Or maybe
nsITemplateXMLQuery should contain all the member variables and
getters/setters and nsXMLQuery would be there only to implement
AddRef/Release and QueryInterface. In any case, it is better to try
to avoid virtual methods when possible.
Maybe same thing with some other interfaces/classes, didn't look at it yet.
Comment 14•18 years ago
|
||
Comment on attachment 263628 [details] [diff] [review]
address comments, convert query to implement an internal interface
>+void
>+XMLBindingValues::GetAssignmentFor(nsXULTemplateResultXML* aResult,
>+ XMLBinding* aBinding,
>+ PRInt32 aIndex,
>+ PRUint16 aType,
>+ nsIDOMXPathResult** aValue)
Indentation, also elsewhere in the same file.
>+void
>+XMLBindingValues::GetNodeAssignmentFor(nsXULTemplateResultXML* aResult,
...
>+ GetAssignmentFor(aResult, aBinding, aIndex,
>+ nsIDOMXPathResult::FIRST_ORDERED_NODE_TYPE, getter_AddRefs(result));
Too long line
>+struct XMLBinding {
btw, would nsXMLBinding be a better name.
>+class XMLBindingSet
and here nsXMLBindingSet
>+ PRInt32
>+ LookupTargetIndex(nsIAtom* aTargetVariable, XMLBinding** aBinding)
>+ {
>+ if (! mBindings)
>+ return -1;
>+
>+ return mBindings->LookupTargetIndex(aTargetVariable, aBinding);
>+ }
Maybe
return mBindings ?
mBindings->LookupTargetIndex(aTargetVariable, aBinding) : -1;
> nsresult
>-nsXULTemplateBuilder::LoadDataSources(nsIDocument* doc)
>+nsXULTemplateBuilder::LoadDataSources(nsIDocument* aDocument, PRBool* aShouldDelayBuilding)
Put the second parameter to a new line.
>+ if (shouldLoadUrls) {
>+ rv = LoadDataSourceUrls(aDocument, datasources, isRDFQuery, aShouldDelayBuilding);
Too long line.
Long lines also elsewhere. Try to fix them when fixable.
http://beaufour.dk/jst-review/?patch=263628&file=
>+ // Parse datasources: they are assumed to be a whitespace
>+ // separated list of URIs; e.g.,
>+ //
>+ // rdf:bookmarks rdf:history http://foo.bar.com/blah.cgi?baz=9
>+ //
>+ nsIURI *docurl = aDocument->GetDocumentURI();
Too many spaces before // -comments
>+ if (NS_FAILED(rv)) {
>+ // This is only a warning because the data source may not
>+ // be accessible for any number of reasons, including
>+ // security, a bad URL, etc.
>+ #ifdef DEBUG
>+ nsCAutoString msg;
>+ msg.Append("unable to load datasource '");
>+ msg.AppendWithConversion(uriStr);
>+ msg.Append('\'');
>+ NS_WARNING(msg.get());
>+ #endif
This code just moved , but would it make sense log this to error console.
>+ nsCOMPtr<nsIDOMEventTarget> xmlel = do_QueryInterface(domDocument);
>+ xmlel->AddEventListener(NS_LITERAL_STRING("load"), this, PR_FALSE);
Why the variable is called xmlel?
>+NS_IMETHODIMP
>+nsXULTemplateResultSetXML::GetNext(nsISupports **aResult)
>+{
>+ nsCOMPtr<nsIDOMNode> node;
>+ nsresult rv = mResults->SnapshotItem(mPosition, getter_AddRefs(node));
>+ if (NS_FAILED(rv))
>+ return rv;
>+
>+ nsXULTemplateResultXML* result =
>+ new nsXULTemplateResultXML(mQuery, node, mBindingSet);
>+ NS_ENSURE_TRUE(result, NS_ERROR_OUT_OF_MEMORY);
>+
>+ mPosition++;
Nit, I prefer prefix over postfix. Prefix is after all a bit faster in some cases
(though, ofc compilers may optimize postfix to prefix in this case).
>+nsXULTemplateQueryProcessorXML::GenerateResults(nsISupports* aDatasource,
...
>+ if (!aQuery)
...
>+ if (! xmlquery)
...
>+ if (!context)
Nit, try to be consistent with if () syntax, either
if (! value) or if (!value)
I prefer the latter one.
>+nsXULTemplateQueryProcessorXML::AddBinding(nsIDOMNode* aRuleNode,
>+ nsIAtom* aVar,
>+ nsIAtom* aRef,
>+ const nsAString& aExpr)
...
>+ if (NS_FAILED(rv)) return rv;
You're changing these elsewhere so that return rv is in its own line.
>+NS_IMETHODIMP nsXULTemplateQueryProcessorXML::CompareResults(nsIXULTemplateResult* aLeft,
...
>+ // XXXndeakin handle other types
So what all other types. Could you perhaps file a bug and put the bug number here.
Fix also Comment #13
I'd still like to look at the patch at least once more. It is after all too small one. So r-.
Attachment #263628 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 15•18 years ago
|
||
Attachment #263628 -
Attachment is obsolete: true
Attachment #263752 -
Flags: review?(Olli.Pettay)
Comment 16•18 years ago
|
||
Comment on attachment 263752 [details] [diff] [review]
Update patch
> nsXULContentBuilder::AddPersistentAttributes(nsIContent* aTemplateNode,
...
>+ if (!mRoot)
>+ return NS_OK;
indentation
>+ ~nsXMLQuery() { mProcessor = nsnull; }
I think this is not needed. And because it is not virtual, it probably doesn't even get called
always.
>+ ~nsXULTemplateResultSetXML()
>+ {}
No need for this.
>+ ~nsXULTemplateQueryProcessorXML() {};
Nor this.
Attachment #263752 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 17•18 years ago
|
||
Attachment #263858 -
Flags: superreview?(jonas)
Assignee | ||
Updated•18 years ago
|
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha6
Comment 18•18 years ago
|
||
The patch now contains only the query processor itself. Changes have been made on this query processor because of changes on the nsIXULTemplateQueryProcessor interface. Most of new things in the xul template builder have been moved into an other patch (see bug 321170).
Updated•18 years ago
|
Attachment #267583 -
Attachment description: updated patch with new template builder → updated patch for new template builder
Comment 19•18 years ago
|
||
I suggest to change the superreviewer flag to Neil Rashbrook since Jonas didn't respond to the superreview request.
Assignee | ||
Updated•18 years ago
|
Attachment #263858 -
Flags: superreview?(jonas) → superreview?(peterv)
Comment 20•18 years ago
|
||
Comment on attachment 263858 [details] [diff] [review]
remove extra constructors as well
There's some long lines and you've added useless whitespace on some empty lines.
>Index: content/xul/templates/src/nsXMLBinding.cpp
>===================================================================
>+ MOZ_COUNT_DTOR(nsXMLBindingSet);
No need for MOZ_COUNT_CTOR/MOZ_COUNT_DTOR if you use NS_LOG_RELEASE.
>+ while (mFirst) {
>+ nsXMLBinding* doomed = mFirst;
>+ mFirst = mFirst->mNext;
>+ delete doomed;
If you make mFirst and mNext nsAutoPtr's you don't need this.
>+PRInt32
>+nsXMLBindingSet::Release()
>+{
>+ --mRefCnt;
>+ NS_LOG_RELEASE(this, mRefCnt, "nsXMLBindingSet");
>+ if (mRefCnt == 0) {
>+ mRefCnt = 1;
>+ delete this;
>+ return 0;
>+ }
>+ return mRefCnt;
>+}
NS_IMPL_RELEASE
>+nsXMLBindingSet::AddBinding(nsIAtom* aVar, nsIDOMXPathExpression* aExpr)
>+ nsXMLBinding* newbinding = new nsXMLBinding(aVar, aExpr);
I'd make this an nsAutoPtr so you don't need to worry about early returns.
>+nsXMLBindingValues::GetAssignmentFor(nsXULTemplateResultXML* aResult,
>+ *aValue = mValues.SafeObjectAt(aIndex);
>+
>+ if (!*aValue) {
>+ if (result && mValues.ReplaceObjectAt(result, aIndex))
Why replace? SafeObjectAt(aIndex) returned null.
>Index: content/xul/templates/src/nsXMLBinding.h
>===================================================================
>+/*
Use /**
>+struct nsXMLBinding {
>+ nsXMLBinding* mNext;
nsAutoPtr
>+class nsXMLBindingSet
>+ PRInt32 mRefCnt;
nsAutoRefCnt
>+ nsXMLBinding* mFirst;
nsAutoPtr
>+ nsXMLBindingSet()
>+ MOZ_COUNT_CTOR(nsXMLBindingSet);
No need for this.
>+ PRInt32 AddRef() {
>+ mRefCnt++;
>+ NS_LOG_ADDREF(this, mRefCnt, "nsXMLBindingSet", sizeof(*this));
>+ return mRefCnt;
>+ }
NS_IMPL_ADDREF
>Index: content/xul/templates/src/nsXULContentBuilder.cpp
>===================================================================
> nsXULContentBuilder::AddPersistentAttributes(nsIContent* aTemplateNode,
>+ if (NS_FAILED(rv))
>+ return rv;
NS_ENSURE_SUCCESS(rv, rv);
(here and in other places)
>Index: content/xul/templates/src/nsXULTemplateBuilder.cpp
>===================================================================
>+#include "nsIDOMDOMImplementation.h"
>+#include "nsIPrivateDOMImplementation.h"
>+static NS_DEFINE_CID(kDOMDOMImplementationCID, NS_DOM_IMPLEMENTATION_CID);
You don't seem to be using this.
>@@ -123,17 +128,18 @@ PRLogModuleInfo* gXULTemplateLog;
> nsXULTemplateBuilder::nsXULTemplateBuilder(void)
>- : mDB(nsnull),
>+ : mDataSource(nsnull),
>+ mDB(nsnull),
> mCompDB(nsnull),
> mRoot(nsnull),
> mQueriesCompiled(PR_FALSE),
> mFlags(0),
> mTop(nsnull)
There's no need to set nsCOMPtr's to null here.
>+nsXULTemplateBuilder::LoadDataSources(nsIDocument* aDocument,
>+ domdoc->GetElementById(Substring(datasources, 1),
>+ getter_AddRefs(dsnode));
>+ if (dsnode) {
>+ if (querytype.IsEmpty()) {
>+ querytype.AssignLiteral("xml");
So if there is no element with the specified id we'll treat it as querytype=rdf?
>+ nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(aDocument);
>+ if (xuldoc)
>+ xuldoc->SetTemplateBuilderFor(mRoot, this);
>
>+ // Now set the database on the element, so that script writers can
>+ // access it.
That comment belongs before the previous lines, no?
>+ nsXULElement *xulcontent = nsXULElement::FromContent(mRoot);
>+ if (! xulcontent) {
if (!mRoot->IsNodeOfType(nsINode::eXUL))
>Index: content/xul/templates/src/nsXULTemplateBuilder.h
>===================================================================
>@@ -325,16 +351,17 @@ public:
>+ nsCOMPtr<nsISupports> mDataSource;
You should add mDataSource to cycle collection (since it seems like it could hold a node).
>Index: content/xul/templates/src/nsXULTemplateQueryProcessorXML.cpp
>===================================================================
>+NS_IMPL_ISUPPORTS1(nsXULTemplateQueryProcessorXML, nsIXULTemplateQueryProcessor)
Since this seems to hold nodes it should probably participate in cycle collections. I'm ok with doing it in a follow-up bug, but it needs to happen.
>+nsXULTemplateQueryProcessorXML::CompileQuery(nsIXULTemplateBuilder* aBuilder,
>+ rv = CreateExpression(expr, aQueryNode, getter_AddRefs(compiledexpr));
>+ if (NS_FAILED(rv))
>+ return NS_OK;
Should this warn (invalid expression)?
>+nsXULTemplateQueryProcessorXML::CompareResults(nsIXULTemplateResult* aLeft,
>+ nsAutoString leftVal;
>+ aLeft->GetBindingFor(aVar, leftVal);
>+
>+ nsAutoString rightVal;
>+ aRight->GetBindingFor(aVar, rightVal);
>+
>+ // currently templates always sort case-insensitive
>+ *aResult = ::Compare(nsDependentString(leftVal),
>+ nsDependentString(rightVal),
>+ nsCaseInsensitiveStringComparator());
Why do you need the nsDependentString's?
>+nsXULTemplateQueryProcessorXML::CreateExpression(const nsAString& aExpr,
>+ if (mRoot) {
Why does this need to check for mRoot?
>+ nsCOMPtr<nsIDOMDocument> doc;
>+ aNode->GetOwnerDocument(getter_AddRefs(doc));
>+
>+ nsCOMPtr<nsIDOMXPathEvaluator> eval = do_QueryInterface(doc);
>+ if (eval) {
>+ nsresult rv =
>+ eval->CreateNSResolver(aNode, getter_AddRefs(nsResolver));
>+ if (NS_FAILED(rv))
>+ return NS_OK;
>+ }
>+ }
>+
>+ return mEvaluator->CreateExpression(aExpr, nsResolver, aCompiledExpr);
>Index: content/xul/templates/src/nsXULTemplateQueryProcessorXML.h
>===================================================================
>+class nsITemplateXMLQuery : public nsISupports
Do we really need a separate interface? Can't you just give nsXMLQuery an IID?
>Index: content/xul/templates/src/nsXULTemplateResultXML.cpp
>===================================================================
>+static PRUint32 mTemplateId = 0;
sTemplateId
>+nsXULTemplateResultXML::nsXULTemplateResultXML(nsITemplateXMLQuery* aQuery,
>+ nsIDOMNode* aNode,
>+ nsXMLBindingSet* aBindings)
>+ : mQuery(aQuery), mNode(aNode)
>+{
>+ mId = ++mTemplateId;
Put this in the initializer list too.
>Index: content/xul/templates/src/nsXULTemplateResultXML.h
>===================================================================
>+class nsXULTemplateResultXML : public nsIXULTemplateResult
>+ nsCOMPtr<nsIDOMNode> mNode;
This should participate in cycle collection too.
Looks good apart from that.
Attachment #263858 -
Flags: superreview?(peterv) → superreview-
Comment 21•18 years ago
|
||
(In reply to comment #20)
> >+ if (result && mValues.ReplaceObjectAt(result, aIndex))
>
> Why replace? SafeObjectAt(aIndex) returned null.
Hmmm, I guess mValues can have holes?
Assignee | ||
Comment 22•18 years ago
|
||
> Why replace? SafeObjectAt(aIndex) returned null.
> Hmmm, I guess mValues can have holes?
Yes, there is a slot with a specific index for each variable
> So if there is no element with the specified id we'll treat it as
> querytype=rdf?
No, I'll change that to use xml when the uri starts with #
> >+ rv = CreateExpression(expr, aQueryNode, getter_AddRefs(compiledexpr));
> >+ if (NS_FAILED(rv))
> >+ return NS_OK;
>
> Should this warn (invalid expression)?
Logging errors is bug 321169
> This should participate in cycle collection too.
I'll file a followup bug about adding cycle collection, which I had actually planned on doing anyway.
Assignee | ||
Comment 23•18 years ago
|
||
Attachment #263858 -
Attachment is obsolete: true
Attachment #268488 -
Flags: superreview?(peterv)
Comment 24•18 years ago
|
||
Comment on attachment 268488 [details] [diff] [review]
Update for comments
>Index: content/xul/templates/src/nsXMLBinding.h
>===================================================================
>+ nsXMLBindingSet()
>+ : mRefCnt(0)
>+ {
>+ }
>+
>+ ~nsXMLBindingSet()
>+ {
>+ }
You can drop these.
Attachment #268488 -
Flags: superreview?(peterv) → superreview+
Assignee | ||
Comment 25•18 years ago
|
||
I have a testcase that I will add to bug 378893.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Updated•18 years ago
|
Attachment #267583 -
Attachment is obsolete: true
Comment 26•18 years ago
|
||
Adding link to docs so they can move to devmo eventually.
Keywords: dev-doc-needed
Updated•17 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•