434493 - Drop more instances of nsISupportsArray in mailnews.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix (diff -w)

Here are some more instances of nsISupportsArray that are easy to fix because they are stand-alone:

nsMsgSearchSession
nsMsgDBFolder
nsMsgDatabase
nsMsgImapProtocol

nsMsgComposeProgress is not used by anything, so this patch cvs removes it (which does mean we get rid of some nsISupportsArray instances of grep).

Comment 1

[Mark Banner (:standard8)]

Attachment: [checked in] The fix (full patch)

Comment 2

[David :Bienvenu]

Comment on attachment 321590 [details] [diff] [review]
The fix (diff -w)

this looks like nice cleanup!

some nits - can you use the prevailing brace style in nsMsgSearchSession, instead of K&R?

e.g.,
+  nsCOMPtr<nsIMsgSearchNotify> listener;
+  while (iter.HasMore()) {


Here, do you mean the member variables like m_hasPendingMoves?
+  // Nothing to do, so don't change the local variables.
+  if (numFolders == 0)
+    return NS_OK;
+

not bailing out on error with the nsMsgDatabase notify methods is a change in behavior - errors should be rare, and it's probably better to not to bail out - I guess we'll see :-)

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)

> Here, do you mean the member variables like m_hasPendingMoves?
> +  // Nothing to do, so don't change the local variables.
> +  if (numFolders == 0)
> +    return NS_OK;
> +
Yes, I've done s/local/member/ in that comment. I checked the patch again, if AddMove only created the array if something was going to be added to it. So I think this is equivalent code.

> not bailing out on error with the nsMsgDatabase notify methods is a change in
> behavior - errors should be rare, and it's probably better to not to bail out -
> I guess we'll see :-)

Yes, I think I was slightly worried about extensions doing the wrong things and messing us up. If we are really relying on the order of listeners being called there its probably going to be an issue anyway.

I'll check the patch in with these comments addressed.

Comment 4

[Mark Banner (:standard8)]

Patch checked in.

Comment 5

[[e-mail]]

Note: since you know the nsCOMArray is never going to have more than kNumHdrsToXfer elements you could reserve space for it i.e.

nsMsgImapHdrXferInfo::nsMsgImapHdrXferInfo()
  : m_hdrInfos(kNumHdrsToXfer)
{
  m_nextFreeHdrInfo = 0;
}

The other alternative would be an nsAutoTArray<nsCOMPtr<nsIImapHeaderInfo>, 10>

Comment 6

[Kent James (:rkent)]

Unfortunately this patch broke my unit test for search that I was preparing for bug 217034.

Previously, I was able start a test search with something like:

searchSession.search(null)

with a null window. But the new test at line 268 of nsMsgSearchSession.cpp:

  m_msgWindowWeak = do_GetWeakReference(aWindow, &rv);
  if (NS_FAILED(rv))
    return rv;

now fails the search with a null window. Is this really necessary? The code elsewhere seems to ignore null windows.

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> Unfortunately this patch broke my unit test for search that I was preparing for
> bug 217034.
...
> Is this really necessary? The code elsewhere seems to ignore null windows.

No it isn't necessary, sorry, that was my error. I know you've included it in the patch for bug 217034, but I'd like to back it out via this bug as I'm probably not going to get to review yours until Monday/Tuesday and I don't know if it'll have caused other regressions or not.

Comment 8

[Mark Banner (:standard8)]

Attachment: [checked in] Drop/Backout the unneeded check

David, please see comments 6/7.