Mark Banner (:standard8) Assignee
	Description • 17 years ago

There is one attribute on nsIMsgMailSession that is no longer used (bug 128297 appears to have removed its only caller):

readonly attribute nsISupportsArray msgWindowsArray;

Additionally, replacing nsCOMPtr<nsISupportsArray> with nsCOMArray<nsIMsgWindow> in nsMsgMailSession should give us some code size & perf improvements as well as moving off an obsolete interface.

	neil@parkwaycc.co.uk
	Comment 2 • 17 years ago

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

>+  // Ensures the shutdown service is initialised?
>   nsCOMPtr<nsIMsgShutdownService> shutdownService = do_GetService(NS_MSGSHUTDOWNSERVICE_CONTRACTID);
Yes, see bug 397498. (Do we want to return the rv from do_GetService?)

>-  if (!aMsgWindow) return NS_ERROR_NULL_POINTER;
>+  if (!aMsgWindow)
>+    return NS_ERROR_NULL_POINTER;
Could use NS_ENSURE_ARG_POINTER?

>+    NS_IF_ADDREF(*aMsgWindow = mWindows[0]);
I think we can assume that this isn't null.

>     // If multiple message windows then we have lots more work.
>-    else if (count > 1)
You seem to have lost this check.

>+      msgWindow = mWindows[count - 1];
> 
>+      if (msgWindow)
Again, we can assume that this isn't null, can't we?

> NS_IMETHODIMP nsMsgMailSession::AddMsgWindow(nsIMsgWindow *msgWindow)
> {
>-  mWindows->AppendElement(msgWindow);
>+  mWindows.AppendObject(msgWindow);
>   return NS_OK;
> }
Well, maybe we should null-check msgWindow here instead?

>+  PRUint32 count = mWindows.Count();
>     if (count == 0)
Nit: Could inline this now that we don't need the address of count.

	Mark Banner (:standard8) Assignee
	Comment 3 • 17 years ago

Revised patch fixing Neil's comments.

	neil@parkwaycc.co.uk
	Comment 5 • 17 years ago

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

You can't see it in the diff -w but I guess you could change topMostWindow to be an nsCOMPtr<nsPIDOMWindow> which avoids the extra do_QueryInterface.

>+      nsIMsgWindow *msgWindow;
>+
>         while (count)
>         {
>+        msgWindow = mWindows[count - 1];
Nit: Might as well declare msgWindow here.
Also consider moving --count here.

	Mark Banner (:standard8) Assignee
	Comment 6 • 17 years ago

Patch checked in -> fixed.

	David :Bienvenu
	Comment 7 • 17 years ago

Being able to enumerate the msgWindows might be useful, so we might want to put this functionality back somehow, maybe by adding an enumerate method. 

For example, I was just asked how to find the UI window a given message uri is open in. Enumerating the msg windows would be one way to do that. I guess using the window mediator would work as well...