Closed Bug 177543 Opened 21 years ago Closed 21 years ago

[FIXr]Fix up some array stuff in nsDocument

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.3alpha

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 2 obsolete files)

mChildren and mStyleSheets do much better as nsCOMArray
Attached patch Pretty much like this (obsolete) — Splinter Review
reviews please? 
Blocks: 171830
Priority: -- → P1
Summary: Fix up some array stuff in nsDocument → [FIX]Fix up some array stuff in nsDocument
Target Milestone: --- → mozilla1.3alpha
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 :(
Attachment #104655 - Flags: superreview+
or maybe nsCOMArray should just use nsAutoVoidArray instead? nsSupportsArray is
also kind of an "auto" array with space for I think 8 elements.
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.  ;)
bz: I'm with you.. lets just use nsCOMArray right now
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!
Attachment #104655 - Flags: review+
> ++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.
Attached patch make the cosmetic changes (obsolete) — Splinter Review
This does not touch the issue of the owner stuff... hyatt?  Comments?
Attachment #104655 - Attachment is obsolete: true
Attachment #104703 - Attachment is obsolete: true
Summary: [FIX]Fix up some array stuff in nsDocument → [FIXr]Fix up some array stuff in nsDocument
Yes, that was a mispaste (that doesn't seem to cause any trouble amusingly 
enough).
fixed for 1.3a
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
As usual, this broke the Os/2 build. Any ideas?
Would explicit casts to nsISupports help?
Apparently they do (and have been checked in).
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.
You need to log in before you can comment on or make changes to this bug.