These functions in nsXULContentBuilder should use an nsCOMArray

Otherwise the code is clean and simple, but the 2nd parameter of GetInsertionLocations is a bit strange. null and empty arrays are handled a bit different way in nsXULTemplateBuilder::UpdateResult. So Neil, if you could comment whether using nsCOMArray<nsIContent>** is ok or do you want something else. And a review would be nice too :)

Comment on attachment 251277 [details] [diff] [review] possible patch nsXULDocument::ApplyPersistentAttributes >- PRUint32 cnt = 0; >- elements->Count(&cnt); >+ PRUint32 cnt = elements.Count(); > if (! cnt) > continue; The temporary variable isn't needed. nsXULContentBuilder::HasGeneratedContent >- PRUint32 cnt; >- rv = elements->Count(&cnt); >+ PRUint32 cnt = elements.Count(); > if (NS_FAILED(rv)) > return rv; > This error check should be removed. The tree builder sets the argument to null in GetInsertionLocations because there aren't any content nodes in a tree, yet it should still generate output. The comment for this method should be clearer about this.

I changed the comment for GetInsertionLocations a bit.

Comment on attachment 251292 [details] [diff] [review] v2 >@@ -2207,28 +2198,23 @@ nsXULDocument::ApplyPersistentAttributes > } > > const PRUnichar* value; > rv = literal->GetValueConst(&value); > if (NS_FAILED(rv)) return rv; > > nsDependentString wrapper(value); > >- PRUint32 cnt; >- rv = aElements->Count(&cnt); >- if (NS_FAILED(rv)) return rv; >+ PRUint32 cnt = aElements.Count(); > > for (PRInt32 i = PRInt32(cnt) - 1; i >= 0; --i) { >- nsISupports* isupports2 = aElements->ElementAt(i); >- if (! isupports2) >+ nsCOMPtr<nsIContent> element = aElements[i]; >+ if (!element) Use aElements.SafeObjectAt(i). Otherwise you'll get a buffer overrun rather than a null return. >@@ -1691,30 +1688,23 @@ nsXULContentBuilder::HasGeneratedContent > > NS_ConvertUTF8toUTF16 refID(uri); > > // just return if the node is no longer in a document > nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(mRoot->GetDocument()); > if (! xuldoc) > return NS_OK; > >- nsCOMPtr<nsISupportsArray> elements; >- rv = NS_NewISupportsArray(getter_AddRefs(elements)); >- if (NS_FAILED(rv)) >- return rv; >- >+ nsCOMArray<nsIContent> elements; > xuldoc->GetElementsForID(refID, elements); > >- PRUint32 cnt; >- rv = elements->Count(&cnt); >- if (NS_FAILED(rv)) >- return rv; >+ PRUint32 cnt = elements.Count(); > > for (PRInt32 i = PRInt32(cnt) - 1; i >= 0; --i) { >- nsCOMPtr<nsIContent> content = do_QueryElementAt(elements, i); >+ nsCOMPtr<nsIContent> content = elements[i]; Same thing here, use SafeObjectAt unless you're absolutely 100% sure that this can never ever go out of bounds (if the array is somehow mutated during the looping for example). Remember that it'll be a security issue if it happens so it's unlikely that the saved speed is woth it. > nsXULContentBuilder::GetInsertionLocations(nsIXULTemplateResult* aResult, >- nsISupportsArray** aLocations) >+ nsCOMArray<nsIContent>** aLocations) > { > *aLocations = nsnull; > > nsAutoString ref; > nsresult rv = aResult->GetBindingFor(mRefVariable, ref); > if (NS_FAILED(rv)) > return PR_FALSE; > >- nsCOMPtr<nsISupportsArray> elements; >- rv = NS_NewISupportsArray(getter_AddRefs(elements)); >- if (NS_FAILED(rv)) >- return PR_FALSE; >- > nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(mRoot->GetDocument()); > if (! xuldoc) > return PR_FALSE; > >- xuldoc->GetElementsForID(ref, elements); >+ *aLocations = new nsCOMArray<nsIContent>; >+ NS_ENSURE_TRUE(*aLocations, PR_FALSE); > >- PRUint32 count; >- elements->Count(&count); >+ xuldoc->GetElementsForID(ref, **aLocations); >+ PRUint32 count = (*aLocations)->Count(); > > PRBool found = PR_FALSE; > > for (PRUint32 t = 0; t < count; t++) { >- nsCOMPtr<nsIContent> content = do_QueryElementAt(elements, t); >+ nsCOMPtr<nsIContent> content = (*aLocations)->ObjectAt(t); SafeObjectAt > nsXULContentBuilder::SynchronizeResult(nsIXULTemplateResult* aResult) > { >- nsSupportsArray elements; >- GetElementsForResult(aResult, &elements); >+ nsCOMArray<nsIContent> elements; >+ GetElementsForResult(aResult, elements); > >- PRUint32 cnt = 0; >- elements.Count(&cnt); >+ PRUint32 cnt = elements.Count(); > > for (PRInt32 i = PRInt32(cnt) - 1; i >= 0; --i) { >- nsCOMPtr<nsIContent> element = do_QueryElementAt(&elements, i); >+ nsCOMPtr<nsIContent> element = elements[i]; And here. >Index: content/xul/templates/src/nsXULTemplateBuilder.cpp >@@ -446,19 +446,20 @@ nsXULTemplateBuilder::UpdateResult(nsIXU > aOldResult, aNewResult, aQueryNode)); > > // get the containers where content may be inserted. If > // GetInsertionLocations returns false, no container has generated > // any content yet so new content should not be generated either. This > // will be false if the result applies to content that is in a closed menu > // or treeitem for example. > >- nsCOMPtr<nsISupportsArray> insertionPoints; >+ nsCOMArray<nsIContent>* points = nsnull; > PRBool mayReplace = GetInsertionLocations(aOldResult ? aOldResult : aNewResult, >- getter_AddRefs(insertionPoints)); >+ &points); >+ nsAutoPtr<nsCOMArray<nsIContent> > insertionPoints(points); You can use the nsAutoPtr directly by using getter_Transfers(insertionPoints) >@@ -501,22 +502,19 @@ nsXULTemplateBuilder::UpdateResult(nsIXU > > if (! queryset) > return NS_OK; > } > > if (insertionPoints) { > // iterate over each insertion point and add or remove the result from > // that container >- PRUint32 count; >- insertionPoints->Count(&count); >- >+ PRUint32 count = insertionPoints->Count(); > for (PRUint32 t = 0; t < count; t++) { >- nsCOMPtr<nsIContent> insertionPoint = >- do_QueryElementAt(insertionPoints, t); >+ nsCOMPtr<nsIContent> insertionPoint = insertionPoints->ObjectAt(t); SafeObjectAt sr=sicking with that fixed.