479844 - Consolidate checking for messages to send later into one function

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

We currently have various places where we're iterating around identities and trying to work out if they have messages to send in the unsent messages folder.

As we've got this code in at least 3 places it seems to make sense to consolidate it into one function (hasUnsentMessages) on nsIMsgSendLater. Attaching patch which does this.

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix v2

Fixed syntax error in TB (; in the wrong place).

Comment 2

[[e-mail]]

Comment on attachment 363872 [details] [diff] [review]
The fix v2

>+    return Components.classes["@mozilla.org/messengercompose/sendlater;1"]
>+                     .getService(Components.interfaces.nsIMsgSendLater)
>+                     .hasUnsentMessages();
Maybe I've misunderstood the code, but it seems to me that if you don't pass in an identity then hasUnsentMessages might fail to find the outbox.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> (From update of attachment 363872 [details] [diff] [review])
> >+    return Components.classes["@mozilla.org/messengercompose/sendlater;1"]
> >+                     .getService(Components.interfaces.nsIMsgSendLater)
> >+                     .hasUnsentMessages();
> Maybe I've misunderstood the code, but it seems to me that if you don't pass in
> an identity then hasUnsentMessages might fail to find the outbox.

hasUnsent messages calls nsMsgSendLater::GetUnsentMessagesFolder which in turn calls GetFolderURIFromUserPrefs [1], the parameter was nsIMsgSend::nsMsgQueueForLater so in that function, it just gets the mail.default_sendlater_uri" pref and ignores the identity argument.

At the moment I've left the identity arguments in for future use, though I would eventually expect a "null" identity to check all outboxes if we implemented it.

[1] http://hg.mozilla.org/comm-central/diff/e4f4569d451a/mailnews/compose/src/nsMsgCompUtils.cpp

Comment 4

[[e-mail]]

Comment on attachment 363872 [details] [diff] [review]
The fix v2

OK, I guess I must have misread the code the first time around.

Comment 5

[David :Bienvenu]

Comment on attachment 363872 [details] [diff] [review]
The fix v2

couple nits, which you can address or not, up to you:

+    // folder (context menu), so we can use the folder and return true/false
+    // straight away.
+    return (folderResource.getTotalMessages(false) > 0);

don't need parens here.

+  // Otherwise, we don't know where we are, so use the current identity and
+  // find out if we have messages or not via that.
+  var identity;
+  var folders = GetSelectedMsgFolders();
+  if (folders.length > 0)
+    identity = getIdentityForServer(folders[0].server);
 
you could use "let" instead of var here...

Comment 6

[Mark Banner (:standard8)]

Checked in with comments addressed: http://hg.mozilla.org/comm-central/rev/b2161ed4236a