467083 - Clean up startup code-flow

Status: RESOLVED INACTIVE

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: patch v1

This patch is to fix a couple of my pet peeves.
(1) The giant try/catch block on startup makes it tough to find coding errors. This block is only protecting against 2 cases: (a) No default account, so we now check whether there's at least one account and hence a default and (b) subscribing to a folder. CVS history says this will throw because the subscribed folder isn't in the folder-pane yet, so I've added that case to the selectFolder try block.

(2) gStartFolderUri is only used in one function. It doesn't need to be global. I've also made loadStartFolder stop calling a few other globals in the process.

This patch also fixes (3) gSearchEmailAddress is obsolete aim integration.

Comment 1

[Magnus Melin [:mkmelin]]

Comment on attachment 350471 [details] [diff] [review]
patch v1

>     // filter our any feed urls that came in as arguments to the new window...

Filter out?

>-    if (window.arguments.length && /^feed:/i.test(window.arguments[0] ))
>-    {
>+    if (/^feed:/i.test(window.arguments[0])) {
       var feedHandler = Components.classes["@mozilla.org/newsblog-feed-downloader;1"].getService(Components.interfaces.nsINewsBlogFeedDownloader);
       if (feedHandler)
         feedHandler.subscribeToFeed(window.arguments[0], null, msgWindow);
>-      gStartFolderUri = null;
>-    }
>-    else
>-      gStartFolderUri = (window.arguments.length > 0) ? window.arguments[0] : null;
>-    gStartMsgKey = (window.arguments.length > 1) ? window.arguments[1]: nsMsgKey_None;
>-    gSearchEmailAddress = (window.arguments.length > 2) ? window.arguments[2] : null;
>+    } else
>+      startFolder = GetMsgFolderFromUri(window.arguments[0]);

Putting the else on the next row will fix indention too;)

>+      // Get the user pref to see if the login at startup is enabled for default
>+      // account

.

Other than that, looks pretty good! r=mkmelin

Comment 2

[Wayne Mery (:wsmwk)]

Joey, (or Nomis101) are you able to drive in what remains from this patch, which has r+

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'm taking this - jminta has been inactive for some time.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch v2 (unbitrotted)

Original patch was really heavily bitrotted, this revised patch shows what may be applicable.

Not sure if the revised patch makes sense as-is, thus requesting for another review and extra pair of eyes. :)

I'm fiddling with comm-central try-server to see if this makes it through.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: failures from mozmill tests

patch v2 caused some mozmill build failures, attaching in case someone would like to take it on. Unfortunately I don't know enough of the code edits (besides unbitrotting) to move the situation forward.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Back to the pool.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Just a note: this isn't likely to get in before the 10th, and I'll be in Budapest for the rest of that week, so the feedback on this is going to have to wait until then, or you could ask someone else to take a look at the mozmill failures, like sid0 or asuth, and go with what they say…

Thanks,
Blake.

Comment 8

[Rain [:rain1] (inactive)]

Comment on attachment 530266 [details] [diff] [review]
patch v2 (unbitrotted)

Review of attachment 530266 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like you lost a bunch of changes from jminta's patch. You change the loadStartFolder signature but never touch any callers.

Given how much code has been rewritten since the patch was first put up, I don't think salvaging it is worth it at this point.

::: mail/base/content/msgMail3PaneWindow.js
@@ +725,5 @@
>      if (initialUri)
>        loadFolder = true;
>  
> +    var am = Components.classes["@mozilla.org/messenger/account-manager;1"]
> +                       .getService(Components.interfaces.nsIMsgAccountManager);

If you're touching the code anyway, might as well switch to MailServices.accounts.

@@ +736,5 @@
> +      // all users to one plane. This allows all users to go to Inbox. User can
> +      // always go to server settings panel and turn off "Check for new mail at
> +      // startup"
> +      var pref = Components.classes["@mozilla.org/preferences-service;1"]
> +                           .getService(Components.interfaces.nsIPrefBranch);

Services.prefs

@@ +749,5 @@
>  
> +      // Get Inbox only if login at startup is enabled.
> +      if (isLoginAtStartUpEnabled) {
> +        // Now find Inbox
> +        var inboxFolder = startFolder.getFolderWithFlags(0x1000);

This should stay as Components.interfaces.nsMsgFolderFlags.Inbox.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 530266 [details] [diff] [review]
patch v2 (unbitrotted)

I think sid's feedback means this needs more work before review.

Comment 10

[Wayne Mery (:wsmwk)]

rereading the bug and patch, I don't think there is a performance win here. Please comment if you disagree.

Comment 11

[Wayne Mery (:wsmwk)]

Geoff,
Is there still value in ths bug report

Comment 12

[Geoff Lankow (:darktrojan)]

No.