We have a bunch of nsAutoString explicitly allocated on the heap (in addition to the ones that are member variables, see bug 26622): http://lxr.mozilla.org/seamonkey/search?string=new+nsAutoString We should fix these. (They're probably also good places to look for other bad string usage.) Once we clean this up, we could also probably do a little funny math and make nsAutoString assert if it's not allocated on the stack.

scc: What's our policy here? Do we want to prevent *all* heap-allocated nsAutoStrings? There are a number of places where people use heap-allocated nsAutoStrings to fuse allocation (at least, I think that was the intent, although it could have been ignorance) in objects that I think are shortlived.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080819081634 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Comment on attachment 334455 [details] [diff] [review] (Bv1) 2 <ns*RuleProcessor.*> Given that this should share string buffers, auto strings aren't useful here anymore, so this is fine. However, you should add an: NS_ASSERTION(hasAttr || mLanguage.IsEmpty(), "GetAttr that returns false should not make string non-empty"); before the final "if (hasAttr)". r+sr=dbaron with that

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080828170525 Minefield/3.1a2pre] (home, debug default) (W2Ksp4) Seems to run without triggering the assertion... :-)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080828200904 Minefield/3.1a2pre] (home, debug default) (W2Ksp4) *|mDataFlavors| items: s/nsCAutoString/nsCString/g. *Some related code/comment rewrites. *s/hangled/handled/ Iiuc, this will add an allocation+deallocation for each flavor. I assume this D&D path/feature is not (perf) critical so this is acceptable. I wonder if |mDataFlavors| itself really needs to be a pointer, or if it could simply be a direct part of the |nsDataObj| class ? <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h>

Comment on attachment 335972 [details] [diff] [review] (Cv1) <nsDataObj.cpp> + for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) { Í'd place the decrement operation to the third field of the for clause for readability. Other than that, r=me.

(In reply to comment #7) > (From update of attachment 335972 [details] [diff] [review]) > + for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) { > > Í'd place the decrement operation to the third field of the for clause for That wouldn't work. > readability. Other than that, r=me. I could move it to the beginning of the loop, if you prefer. (I don't.)

(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 335972 [details] [diff] [review] [details]) > > + for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) { > > > > Í'd place the decrement operation to the third field of the for clause for > > That wouldn't work. Please enlighten me as I'm failing miserably to see why this wouldn't work: for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) {

(In reply to comment #9) > for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) { Because |idx| is used inside the loop: *that would "need" to add a |- 1| to the first field. *even so, the last iteration, with the |0| value, would not happen.

(In reply to comment #10) > (In reply to comment #9) > > for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) { > > Because |idx| is used inside the loop: > *that would "need" to add a |- 1| to the first field. > *even so, the last iteration, with the |0| value, would not happen. Sorry, I meant to make it: for (PRUint32 idx = mDataEntryList.Length() - 1; idx >= 0; idx--) {

Oops, it's unsigned. Never mind.

(In reply to comment #12) > Oops, it's unsigned. Never mind. Yeah ;-> Then, is it all right with you to leave it as I wrote it ?

Yeah :)

Comment on attachment 335899 [details] [diff] [review] (Bv1a) 2 <ns*RuleProcessor.*> [Checkin: Comment 15] http://hg.mozilla.org/mozilla-central/rev/837cacb67631

Comment on attachment 335972 [details] [diff] [review] (Cv1) <nsDataObj.cpp> > nsDataObj::~nsDataObj() This method isn't perf sensitive and the arrays are small so I see no reason to complicate the loops here to save a few calls to Count()/Length(). I prefer the current code since it's more readable. Feel free to move the "PRInt32" and add the spaces in the first loop though. sr=mats

Cv1, with comment 16 suggestion(s) :-|

Comment on attachment 336745 [details] [diff] [review] (Cv1a) <nsDataObj.cpp> [Checkin: Comment 18] http://hg.mozilla.org/mozilla-central/rev/3632d232b2c5

(In reply to comment #6) > I wonder if |mDataFlavors| itself really needs to be a pointer, > or if it could simply be a direct part of the |nsDataObj| class ? > <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h> Ere, Mats, what about my question ?

(In reply to comment #19) > (In reply to comment #6) > > I wonder if |mDataFlavors| itself really needs to be a pointer, > > or if it could simply be a direct part of the |nsDataObj| class ? > > <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h> > > Ere, Mats, what about my question ? Oh, right, sorry about that. I can't see any reason why it should be a pointer.

(In reply to comment #20) > I can't see any reason why it should be a pointer. I filed bug 455557.

Uncompiled...

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081012 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4) (compiled; assuming run...) Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto" string ? Benjamin: in case the "Auto" is kept, should I do something else than the (3) comment I added ?

Mike: I assume the "Auto" is/are really wanted as is ? Benjamin: in case the "Auto" is kept, should I do something else than the (2) comment I added ?

(In reply to comment #23) > Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto" > string ? I'm not that good with the string classes, but I can't see the reason for using Auto string. It's not performance critical code though, and perhaps there is a reason for this (some specific use pattern or such) so another opinion would be appreciated.

(In reply to comment #25) > (In reply to comment #23) > > Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto" > > string ? > > I'm not that good with the string classes, but I can't see the reason for using > Auto string. http://mxr.mozilla.org/mozilla-central/search?string=sIMECompUnicode&case=on&find=%2Fwidget%2Fsrc%2Fwindows%2FnsWindow%5C (afaict) The only reason is that there is 2 |Truncate()| and the memory space is reused. Considering code like 7460 PRInt32 len = sIMECompUnicode->Length(); 7461 if (len > IME_MAX_CHAR_POS) I wonder if |char sIMECompUnicode[64]| would not even do the job !?

(In reply to comment #26) > I wonder if |char sIMECompUnicode[64]| would not even do the job !? Or |char sIMECompUnicode[IME_MAX_CHAR_POS + 1]| if it's really a string with terminating \0. I think IME isn't performance critical though, so perhaps it isn't worth it.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081012 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4) (compiled, but not run) NB: I would have done it with "char []", but the string being Unicode would make it "needlessly" more complicated, so I concur with comment 28. Ftr, ImmGetCompositionString() documentation is at http://msdn.microsoft.com/en-us/library/ms776186(VS.85).aspx

Ev1a, with 2 "nits".

Comment on attachment 343415 [details] [diff] [review] (Ev1b) <nsWindow.*> [Checkin: See comment 32] It could have been a wide char array, but I like this better.

Comment on attachment 343415 [details] [diff] [review] (Ev1b) <nsWindow.*> [Checkin: See comment 32] sr=mats with the following nits fixed: > - NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null"); > +nsWindow::HandleTextEvent(HIMC hIMEContext, PRBool aCheckAttr) > +{ > + if (NS_UNLIKELY(!sIMECompUnicode)) { > + NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null"); > + return; > + } I think the current code is more readable and I don't see a compelling reason to change it. > nsWindow::GetTextRangeList > - NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null"); same > + if (NS_UNLIKELY(lRtn < 0 || > + !EnsureStringLength(*sIMECompUnicode, (lRtn / sizeof(WCHAR)) + 1))) This line is longer than 80 chars. (You can just drop the NS_UNLIKELY, this code isn't perf sensitive). > - PRUint32 i = 0; > - for (i = 0; i < sIMECompUnicode->Length(); i++) { > - if (PT_IN_RECT(*ptPos, sIMECompCharPos[i])) > - break; > + PRUint32 i = 0, len = sIMECompUnicode->Length(); > + while (i < len && !PT_IN_RECT(*ptPos, sIMECompCharPos[i])) { > + ++i; I prefer the old loop construct which I find easier to read. Feel free to lift out the Length() call to a separate line if you want: PRUint32 len = sIMECompUnicode->Length(); but 'len' is guaranteed to be <= IME_MAX_CHAR_POS (64) at this point so I don't think it's a perf problem to leave it as is.

Comment on attachment 343415 [details] [diff] [review] (Ev1b) <nsWindow.*> [Checkin: See comment 32] http://hg.mozilla.org/mozilla-central/rev/039a107a3cec with comment 31 suggestion(s).

Comment on attachment 342828 [details] [diff] [review] (Fv1) <xpcwrappednative.cpp> This is rotted, but the comment in the first chunk seems vacuous (it duplicates the content of the preceding and more detailed comment) and the second one doesn't mean anything to me, so it's probably not clear to others reading the code either!