94243 - nsVoidArray/nsSupportsArray: followup changes to users

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

This is a followup to bug 90545, incorporating the changes to files other than
nsVoidArray/nsSupportsArray.  Cleans up usage: use the right functions, avoid
extra allocations, use nsAutoVoidArray where it's totally non-controversial (see
bug 92573, bug 92575, and bug 92576 for member variable changes).  Modifies
xpcjsruntime.cpp to use SizeTo() to avoid allocations during js GC passes
(already has a=dbradley and jband in 90545).

I made a new bug to cover these modifications only, to simplify the work of
reviewers, reduce people's email load, etc.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

I'll take this.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Changes to use of nsVoidArray/nsSupportsArray (not member var changes)

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: updated patch for bit-rot

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

We need reviews on this. I'm willing to drop individual fixes from the patch if
there's objection, but I'd like to get this into the tree.  This fixes a bunch
of misuses of arrays, and includes a performance improvement for JS garbage
collection among others.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

BTW, the "putting ViewManager into an nsCOMPtr" stuff in
nsGfxListControlFrame.cpp wasn't originally intended to be in this patch; I
picked it up when updating for bit-rot.  I was cleaning up inconsistent uses of
ViewManager and one happened to be here.

Comment 6

[Pierre Saslawsky]

r=pierre for the changes in nsCSSDeclaration.cpp

Comment 7

[Brendan Eich [:brendan]]

Why the null vm check?  The old code was happy to count on the view manager
pointer being non-null.

Nits: Here and elsewhere, it looks like you're indenting the comments using tabs
that expand to 4-space stops.

 if (!gGlobalList) {
-    gGlobalList = new nsFontNodeArray();
+
// This may well expand further (families * sizes * styles?), but it's
+
// only created once.
+    gGlobalList = new nsFontNodeArray;
     if (!gGlobalList) {
       return NS_ERROR_OUT_OF_MEMORY;

Also, prevailing stylle for space-after-comma-in-arglist should be followed,
e.g. in this code:

-          mFontMetrics.ReplaceElementAt(metrics, 0);
+          mFontMetrics.MoveElement(0,cnt);

Pick these nits, and sr=[e-mail].  Let's keep this on the 0.9.4 radar.

/be

Comment 8

[Brendan Eich [:brendan]]

Yup, tabs -- see how pasting them into bugzilla's textarea caused them to become
magnified horizontally.  Please don't put tabs in files.  Use vim with |et| set
instead of vi, configure Emacs to respect the modeline, etc.  Thanks,

/be

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Ok.  Note that files with tab-width=4 (nsViewManager.cpp, the mailnews file,
nsSHEntry.cpp, etc) generally have tabs scattered through them already.  I
removed tabs from my patches.

Fixed ',' usage - some files are one way, some are the other (though
space-after-comma seems most prevalent), but I'd changed the style in a couple
of places.

As for viewmanager - 2/3 of the viewmanager references in the codebase use
nsCOMPtr's and check that it exists, 1/3 don't.  Also, we were crashing on
missing viewmanagers due to leaking presshells at one point, and in porkjockeys
it was stated that NS_RELEASE should only be being used where it's really
required - usually things should be in nsCOMPtr so they don't leak by mistake.

In fact, in that file (nsGFXListControlFrame.cpp), the place I changed it uses
nsCOMPtr's everywhere except for a large commented out block, which is where
most of those changes are (except for one "if (!viewmanager) return
NS_ERROR_FAILURE;" because of the aforementioned paranoia about leaking
presshells).

new patch to be posted momentarily.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: updated per comments. Also spurious existing tabs removed from nsFontMetricsGTK.cpp

Comment 11

[Brendan Eich [:brendan]]

Patches should eliminate tabs, per the indent-tabs-mode: nil part of the Emacs
modeline -- or we shall never be free of tabs.

Thanks for straightening out that view manager code.  Indeed, nsCOMPtr-izing is
almost always the right thing.  I just was wondering whether you knew of a bug
where the result of the getter could be null.

/be

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

From nsViewManager.cpp:
/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: true; c-basic-offset: 4 -*-

Shall I file a bug against all files with indent-tabs-mode: true?  (15 of them
from a quick lxr regexp, but I didn't search for modelines that were missing
indent-tabs-mode:).

http://lxr.mozilla.org/seamonkey/search?string=tabs-mode%3A+%5B%5En%5D&regexp=on

I have sr=brendan, and r=pierre for the CSS code (plus ok on the js stuff from
dbradley & jband).  I need r= for the rest and then we can ask drivers for a=.

Comment 13

[Chris Waterson]

All except the ones that I had to fight with rjc over ;-). (I think I also set
something like ``stroustrup'' in those...) I don't think you need to file a bug
-- fix 'em and call me the super-reviewer, and I'll take any heat for you.

Comment 14

[Chris Waterson]

How about |aNodes->IndexOf(node) >= 0|? That can usually be a single
test-and-branch instruction, as opposed to a load followed by a compare...

-      PRInt32 n = aNodes->Count();
-      for (PRInt32 j = 0; j < n; j++) {
-        if (aNodes->GetElement(j) == node) {
-          found = 1;
-        }
-      }
+      found = (aNodes->IndexOf(node) != -1);

Do you need to cast to |void*| here?

-          mSelectionCache->InsertElementAt((void*)selected, i);
+          mSelectionCache->AppendElement((void*)selected);

Do we actually have cases where we'd fail to get the view manager? Or is
over-paranoid COM disease creep?

 parentView->GetViewManager(*getter_AddRefs(viewManager));
+  if (!viewManager)
+      return NS_ERROR_FAILURE;
+    


sspitzer should look at the nsMsgFilterList.cpp changes, although they look fine
to me.

Do you need to cast to |void*|?

+
+    // This implicitly extends the array to include aOffset
+    mChildren.ReplaceElementAt((void *) aChild, aOffset);
+

So...r= or sr=waterson, modulo the above nits and getting sspitzer to bless
nsMsgFilterList.cpp changes.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

>How about |aNodes->IndexOf(node) >= 0|?

	Sure.

>nsGfxListControlFram.cpp:
>Do you need to cast to |void*| here?

	No, we don't; it was in the original code.  However, all the uses of
nsVoidArray in that file use it; it would be strange to remove it from this one
and leave all the others.

>Do we actually have cases where we'd fail to get the view manager? Or is
>over-paranoid COM disease creep?

	Both.  Normally we shouldn't ever fail to get it.  However, if a presshell
leaks (as they have in the past), it can happen (which was how I ran into this
issue).  Since 2/3 of the code uses nsCOMPtrs, and it allows us to get rid of
NS_RELEASE's, and the cost of a few "if (ptr) ptr->..." instancea is minimal
(should be simply a conditional branch after the pointer is loaded, maybe a test
but not too likely), it seems reasonable.  Leaking is one thing, but crashing
due to a leak is not good.

>nsSHEntry.cpp:
>Do you need to cast to |void*| here?

	No, it was in the original code.  In this case I'll remove it.

	I'll get sspitzer to review the mailnews stuff.  Thanks!

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

sspitzer is on vacation, asking mscott for r=

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

By email got:

looks good, thanks, r/sr =bienvenu
- david

Requesting a= from drivers

Comment 18

[Asa Dotzler [:asa]]

a=asa on behalf of drivers.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Fix checked in

Comment 20

[Roland Mainz]

Reopening bug... Xlib toolkit wasn't patched. I am going to file a GTK+ toolkit
--> Xlib toolkit patch ...

Comment 21

[Roland Mainz]

Attachment: Xlib toolkit patch (port from GTK+ toolkit)

Comment 22

[Roland Mainz]

Filed patch, requesting r=/sr=/a= ...

Comment 23

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

r=dbaron on attachment 47201 [details] [diff] [review]

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

I didn't touch xlib because I don't normally build it, and I was targetting
spots that showed up frequently when booting or loading a page in my stats.

r=[e-mail], for what it's worth

Comment 25

[Johnny Stenback (:jst)]

sr=jst

Comment 26

[Asa Dotzler [:asa]]

a=asa .

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

xlib changes committed