Closed Bug 479844 Opened 11 years ago Closed 11 years ago

Consolidate checking for messages to send later into one function

Categories

(MailNews Core :: Networking: SMTP, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: standard8, Assigned: standard8)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch The fix (obsolete) — Splinter Review
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.
Attachment #363747 - Flags: superreview?(bienvenu)
Attachment #363747 - Flags: review?(neil)
Attached patch The fix v2Splinter Review
Fixed syntax error in TB (; in the wrong place).
Attachment #363747 - Attachment is obsolete: true
Attachment #363872 - Flags: superreview?(bienvenu)
Attachment #363872 - Flags: review?(neil)
Attachment #363747 - Flags: superreview?(bienvenu)
Attachment #363747 - Flags: review?(neil)
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.
(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
Whiteboard: [needs review neil,bienvenu]
Comment on attachment 363872 [details] [diff] [review]
The fix v2

OK, I guess I must have misread the code the first time around.
Attachment #363872 - Flags: review?(neil) → review+
Attachment #363872 - Flags: superreview?(bienvenu) → superreview+
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...
Checked in with comments addressed: http://hg.mozilla.org/comm-central/rev/b2161ed4236a
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review neil,bienvenu]
You need to log in before you can comment on or make changes to this bug.