Bug 1622979 - Ongoing comment improvements in IMAP code. r=mkmelin
authorBen Campbell <benc@thunderbird.net>
Tue, 17 Mar 2020 11:13:18 +1300
changeset 38583 009404ae25d8c7a718f730f3c7fd3a9f8202fd7e
parent 38582 41e7738adee2070b2a3f77c4c32ad40110a0c6c7
child 38584 0ddfc1c62fefdabc3dc6f4726314f1ad8d4601c9
push id400
push userclokep@gmail.com
push dateMon, 04 May 2020 18:56:09 +0000
reviewersmkmelin
bugs1622979
Bug 1622979 - Ongoing comment improvements in IMAP code. r=mkmelin
mailnews/base/public/nsIMsgMessageService.idl
mailnews/imap/public/nsIImapProtocol.idl
mailnews/imap/public/nsIImapService.idl
mailnews/imap/src/nsImapIncomingServer.h
mailnews/imap/src/nsImapProtocol.cpp
--- a/mailnews/base/public/nsIMsgMessageService.idl
+++ b/mailnews/base/public/nsIMsgMessageService.idl
@@ -15,16 +15,23 @@ interface nsIMsgSearchSession;
 interface nsIMsgDBHdr;
 interface nsIStreamConverter;
 interface nsICacheEntry;
 
 %{C++
 #include "MailNewsTypes.h"
 %}
 
+/**
+ * nsIMsgMessageService provides higher-level, UI-oriented calls for
+ * dealing with messages in a protocol-agnostic way.
+ * Things the user would recognise as actions they initiated.
+ * This covers things like displaying messages, copying them, saving them
+ * to disk, saving attachments...
+ */
 [scriptable, uuid(3aa7080a-73ac-4394-9636-fc00e182319b)]
 interface nsIMsgMessageService : nsISupports {
 
   /**
    * If you want a handle on the running task, pass in a valid nsIURI
    * ptr. You can later interrupt this action by asking the netlib
    * service manager to interrupt the url you are given back.
    * Remember to release aURL when you are done with it. Pass nullptr
--- a/mailnews/imap/public/nsIImapProtocol.idl
+++ b/mailnews/imap/public/nsIImapProtocol.idl
@@ -15,29 +15,38 @@ interface nsIImapFlagAndUidState;
 
 [ptr] native nsIImapHostSessionList (nsIImapHostSessionList);
 %{C++
 class nsIImapHostSessionList;
 %}
 
 [scriptable, uuid(6ef189e5-8711-4845-9625-d1c74c22c4b5)]
 interface nsIImapProtocol : nsISupports {
+  /**
+   * Set up this connection to run a URL.
+   * Called by nsImapIncomingServer to process a queued URL when it spots a
+   * free connection.
+   * Because nsImapProtocol is really a connection and doesn't follow the
+   * usual nsIChannel lifecycle, this function is provided to allow reuse.
+   * Over and over again.
+   */
   void LoadImapUrl(in nsIURI aUrl, in nsISupports aConsumer);
 
   /**
-   * IsBusy returns true if the connection is currently processing a url
+   * IsBusy returns true if the connection is currently processing a URL
    * and false otherwise.
    */
   void IsBusy(out boolean aIsConnectionBusy,
               out boolean isInboxConnection);
 
-  /** Protocol instance examines the url, looking at the host name,
+  /**
+   * Protocol instance examines the URL, looking at the host name,
    * user name and folder the action would be on in order to figure out
-   * if it can process this url. I decided to push the semantics about
-   * whether a connection can handle a url down into the connection level
+   * if it can process this URL. I decided to push the semantics about
+   * whether a connection can handle a URL down into the connection level
    * instead of in the connection cache.
    */
   void CanHandleUrl(in nsIImapUrl aImapUrl, out boolean aCanRunUrl,
                     out boolean hasToWait);
 
   /**
    * Initialize a protocol object.
    * @param aHostSessionList host session list service
--- a/mailnews/imap/public/nsIImapService.idl
+++ b/mailnews/imap/public/nsIImapService.idl
@@ -20,16 +20,21 @@ interface nsIUrlListener;
 interface nsIURI;
 interface nsIFile;
 interface nsIMsgFolder;
 interface nsIMsgWindow;
 interface nsIMsgMailNewsUrl;
 interface nsIImapIncomingServer;
 interface nsICacheStorage;
 
+/**
+ * Most of the nsIImapService methods are friendly front ends for composing and
+ * issuing "imap://" protocol operations. Usually a nsImapUrl will be returned.
+ * This url object is stateful and tracks the issued request.
+ */
 [scriptable, uuid(aba44b3d-7a0f-4987-8794-96d2de66d966)]
 interface nsIImapService : nsISupports
 {
   // You can pass in null for the url listener and the url if you don't require either.....
   void selectFolder(in nsIMsgFolder aImapMailFolder,
                     in nsIUrlListener aUrlListener,
                     in nsIMsgWindow   aMsgWindow,
                     out nsIURI aURL);
--- a/mailnews/imap/src/nsImapIncomingServer.h
+++ b/mailnews/imap/src/nsImapIncomingServer.h
@@ -110,23 +110,29 @@ class nsImapIncomingServer : public nsMs
 
   /**
    * All requests waiting for a real connection.
    * Each URL object holds a reference to the nsIImapMockChannel that
    * represents the request.
    */
   nsCOMArray<nsIImapUrl> m_urlQueue;
 
+  /**
+   * Consumers for the queued urls. The number of elements here should match
+   * that of m_urlQueue. So requests with no consumer should have a nullptr
+   * entry here.
+   */
+  nsTArray<nsISupports *> m_urlConsumers;
+
   nsCOMPtr<nsIStringBundle> m_stringBundle;
   nsCOMArray<nsIMsgFolder>
       m_subscribeFolders;  // used to keep folder resources around while
                            // subscribe UI is up.
   nsCOMArray<nsIMsgImapMailFolder>
       m_foldersToStat;  // folders to check for new mail with Status
-  nsTArray<nsISupports *> m_urlConsumers;
   eIMAPCapabilityFlags m_capability;
   nsCString m_manageMailAccountUrl;
   bool m_userAuthenticated;
   bool mDoingSubscribeDialog;
   bool mDoingLsub;
   bool m_shuttingDown;
 
   mozilla::Mutex mLock;
--- a/mailnews/imap/src/nsImapProtocol.cpp
+++ b/mailnews/imap/src/nsImapProtocol.cpp
@@ -732,18 +732,19 @@ static void SetSecurityCallbacksFromChan
   NS_NewNotificationCallbacksAggregation(callbacks, loadGroup,
                                          getter_AddRefs(securityCallbacks));
   if (securityCallbacks) aTrans->SetSecurityCallbacks(securityCallbacks);
 }
 
 // Setup With Url is intended to set up data which is held on a PER URL basis
 // and not a per connection basis. If you have data which is independent of the
 // url we are currently running, then you should put it in Initialize(). This is
-// only ever called from the UI thread. It is called from LoadUrl, right before
-// the url gets run - i.e., the url is next in line to run.
+// only ever called from the UI thread. It is called from LoadImapUrl, right
+// before the url gets run - i.e., the url is next in line to run.
+// See also ReleaseUrlState(), which frees a bunch of the things set up in here.
 nsresult nsImapProtocol::SetupWithUrl(nsIURI *aURL, nsISupports *aConsumer) {
   nsresult rv = NS_ERROR_FAILURE;
   NS_ASSERTION(aURL, "null URL passed into Imap Protocol");
   if (aURL) {
     m_runningUrl = do_QueryInterface(aURL, &rv);
     m_runningUrlLatest = m_runningUrl;
     if (NS_FAILED(rv)) return rv;
 
@@ -2118,27 +2119,31 @@ bool nsImapProtocol::TryToRunUrlLocally(
   if (imapChannel->ReadFromLocalCache()) {
     (void)imapChannel->NotifyStartEndReadFromCache(true);
     return true;
   }
   return false;
 }
 
 // LoadImapUrl takes a url, initializes all of our url specific data by calling
-// SetupUrl. If we don't have a connection yet, we open the connection. Finally,
-// we signal the url to run monitor to let the imap main thread loop process the
-// current url (it is waiting on this monitor). There is a contract that the
-// imap thread has already been started b4 we attempt to load a url....
+// SetupUrl. Finally, we signal the url to run monitor to let the imap main
+// thread loop process the current url (it is waiting on this monitor). There
+// is a contract that the imap thread has already been started before we
+// attempt to load a url...
+// LoadImapUrl() is called by nsImapIncomingServer to run a queued url on a free
+// connection.
 NS_IMETHODIMP nsImapProtocol::LoadImapUrl(nsIURI *aURL,
                                           nsISupports *aConsumer) {
   nsresult rv = NS_ERROR_FAILURE;
   if (aURL) {
 #ifdef DEBUG_bienvenu
     printf("loading url %s\n", aURL->GetSpecOrDefault().get());
 #endif
+    // We might be able to fulfil the request locally (e.g. fetching a message
+    // which is already stored offline).
     if (TryToRunUrlLocally(aURL, aConsumer)) return NS_OK;
     m_urlInProgress = true;
     m_imapMailFolderSink = nullptr;
     rv = SetupWithUrl(aURL, aConsumer);
     NS_ASSERTION(NS_SUCCEEDED(rv), "error setting up imap url");
 
     m_lastActiveTime = PR_Now();
   }