177543 - [FIXr]Fix up some array stuff in nsDocument

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

mChildren and mStyleSheets do much better as nsCOMArray

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Pretty much like this

Comment 2

[Boris Zbarsky [:bzbarsky]]

reviews please? 

Comment 3

[Alec Flett]

Comment on attachment 104655 [details] [diff] [review]
Pretty much like this

cool. sr=alecf (and its nice to see the ownership on mStyleSheets cleaned up a
bit!)

I wonder though, with the switch from nsAutoVoidArray to nsCOMArray (which uses
an nsVoidArray internally) if we're hurt anyone with the extra
allocation...maybe we need an nsAutoCOMArray :(

Comment 4

[Alec Flett]

or maybe nsCOMArray should just use nsAutoVoidArray instead? nsSupportsArray is
also kind of an "auto" array with space for I think 8 elements.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm... Having an nsAutoCOMArray is probably more flexible... then again,
consumers who usually have an empty nsCOMArray can just go with nsCOMArray* and
we can make the main nsCOMArray auto...

I'd advise we land this as-is and see what the effect is on pageload, but I'd
like to know what peterv and jst think.  ;)

Comment 6

[Alec Flett]

bz: I'm with you.. lets just use nsCOMArray right now

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 104655 [details] [diff] [review]
Pretty much like this

>Index: content/base/src/nsDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsDocument.cpp,v
>retrieving revision 3.394
>diff -u -1 -0 -r3.394 nsDocument.cpp
>--- content/base/src/nsDocument.cpp	11 Oct 2002 04:22:42 -0000	3.394
>+++ content/base/src/nsDocument.cpp	30 Oct 2002 18:49:13 -0000
>@@ -582,42 +582,34 @@
...
>+      PRInt32 count = mChildren.Count();
>+      for (indx = 0; indx < count; indx++) {

++indx ?

> NS_IMETHODIMP
> nsDocument::UpdateStyleSheets(nsISupportsArray* aOldSheets, nsISupportsArray* aNewSheets)
> {
...
>       if (enabled) {
>         AddStyleSheetToStyleSets(sheet);
>-        sheet->SetOwningDocument(nsnull);

This can just be removed like that?

> NS_IMETHODIMP 
> nsDocument::SetScriptGlobalObject(nsIScriptGlobalObject *aScriptGlobalObject)
...
>-    for (indx = 0; indx < ucount; indx++) {
>-      nsCOMPtr<nsIContent> content(dont_AddRef(NS_STATIC_CAST(nsIContent*,mChildren->ElementAt(indx))));
>-      content->SetDocument(nsnull, PR_TRUE, PR_TRUE);
>+    for (indx = 0; indx < count; indx++) {

++indx ?

>@@ -3216,35 +3179,34 @@
...
>   if (!aRefChild) {
>-    PRUint32 count;
>-    mChildren->Count(&count);
>+    PRInt32 count = mChildren.Count();
>     indx = count;

Can't you just do indx = mChildren.Count(); ?

>-    mChildren->AppendElement(content);
>+    mChildren.AppendObject(content);
>   }

r=peterv. Nice cleanup!

Comment 8

[Boris Zbarsky [:bzbarsky]]

> ++indx ?

Um, sure.

> This can just be removed like that?

I asked myself that before removing it.  I can see no possible way that that
code is correct as-written.... We set the owner to "this" just a few lines
above... The whole thing looks like a mispaste to me, but ccing hyatt (author of
that code) to make sure.

> Can't you just do indx = mChildren.Count(); ?

yes, I can.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: make the cosmetic changes

This does not touch the issue of the owner stuff... hyatt?  Comments?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: let's try the real patch....

Comment 11

[David Hyatt]

Yes, that was a mispaste (that doesn't seem to cause any trouble amusingly 
enough).

Comment 12

[Boris Zbarsky [:bzbarsky]]

fixed for 1.3a

Comment 13

[Mike Kaply [:mkaply]]

As usual, this broke the Os/2 build. Any ideas?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Would explicit casts to nsISupports help?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Apparently they do (and have been checked in).

Comment 16

[Boris Zbarsky [:bzbarsky]]

As a note, I had to make two more changes to fix the leaks that ensued.  Both
AddStyleSheet and InsertStyleSheetAt had addrefs that were no longer needed and
caused leaks of the sheets.