321174 - move GetElementsForResult and GetElementsForID to an nsCOMArray

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

These functions in nsXULContentBuilder should use an nsCOMArray

Comment 1

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

Attachment: possible patch

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 2

[Neil Deakin]

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.

Comment 3

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

Attachment: v2

I changed the comment for GetInsertionLocations a bit.

Comment 4

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

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.