759163 - If Account Provisioner tab is restorable on startup, the Account Provisioner dialog should not open.

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR:

1. Create a fresh TB profile 
2. When the Account Provisioner opens, type a search into the input and hit the Search button.
3. When a result comes up, choose one so that an Account Provisioner tab opens.
4. Quit and restart Thunderbird.

What happens?

The Account Provisioner dialog comes up again, and our old Account Provisioner tab is not opened.  The old tab will *only* be restored once the dialog is closed, which is not ideal.

What's expected?

Thunderbird should go back into the last state, where we had an Account Provisioner tab opened, so that the user can continue their transaction.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Hover.com when restoring the tab after a closed session

Hm, so there's a problem with this, at least for Hover.com.

I got to the Hover.com order form, closed Thunderbird, restored, closed the dialog to unblock the tab from restoring, and this is what I got.

That's not the order form...

Comment 2

[Mike Conley (:mconley) (:⚙️)]

So Blake, even if we manage to restore the tab and prevent the dialog from coming up, it looks like Hover.com is returning us to the same order form as before.

Is this something we should bring up with them?

Comment 3

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

Yeah, sounds like something Pete might want to know about…  (So I cc-ed him!  ;)

Comment 4

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) from comment #2)
> So Blake, even if we manage to restore the tab and prevent the dialog from
> coming up, it looks like Hover.com is returning us to the same order form as
> before.

That should have been "it looks like Hover.com is NOT returning us to the same order form as before".

Comment 5

[Mike Conley (:mconley) (:⚙️)]

So I don't think this is as simple as just shuffling one or two things around in the startup sequence of TB. The code that restores tabs (http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#669) only wants to be called by |loadStartFolder| or |loadStartMsgHdr|, and I'd wager that trying to fire up tabs before the dialog first comes up is likely a bad idea.

I wonder if, instead, we should get our accountProvisionerTab to register an observer for the quit-application notification while it's open. If quit-application is fired, it stashes a value somewhere that we can check for on startup. If the value exists, or is true, the dialog is skipped, and we go ahead and restore the tabs.

In my opinion, that's the least volatile / risky solution to the problem. I don't like the idea of modifying too much of our start-up sequence - especially if we're hoping to land on beta.

If we do go that route, where would we put such a value? Stash it in prefs? Is there precedent for using prefs for something like this?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

My first go at it.

A few notes:

1) I had to add a new observer notification. As luck would have it, quit-application is not fired when hitting the "X" or Alt-F4'ing the final 3pane. Who knew.

Also, attaching an unload event observer to the messenger also didn't seem to work, which is why I opted for the observer notification. If something else is preferred, let me know.

2) There were a few instances where, after opening the provisioner order form tab, the inbox tab would be focused again immediately after. This only happened once or twice, and I think it's because of a race where the 3pane hasn't completely finished starting up before the provisioner dialog is displayed (which blocks). I've doubled the time to wait for the 3pane to finish its startup from 100ms to 200ms, and the problem seems to have subsided.

Not the most graceful solutions in here, but I think they're safe.

Comment 7

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

Comment on attachment 627981 [details] [diff] [review]
Patch v1

>+++ b/mail/base/content/msgMail3PaneWindow.js
>@@ -327,24 +327,40 @@ const MailPrefObserver = {
>     // We need to let the event loop pump a little so that the 3pane finishes
>     // opening - so we use setTimeout. The 100ms is a bit arbitrary, but seems
>     // to be enough time to let the 3pane do it's thing, and not pull focus
>     // when the Account Provisioner modal window closes.
>     setTimeout(function() {
>       NewMailAccountProvisioner(msgWindow, { okCallback: okCallback });
>-    }, 100);
>+    }, 200);

If we're changing this to 200 (and was that intentional?), we should also change the comment above.

Other than that, it seems good.  So I'm going to say r=me.

Thanks,
Blake.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #7)
> Comment on attachment 627981 [details] [diff] [review]
> Patch v1
> 
> >+++ b/mail/base/content/msgMail3PaneWindow.js
> >@@ -327,24 +327,40 @@ const MailPrefObserver = {
> >     // We need to let the event loop pump a little so that the 3pane finishes
> >     // opening - so we use setTimeout. The 100ms is a bit arbitrary, but seems
> >     // to be enough time to let the 3pane do it's thing, and not pull focus
> >     // when the Account Provisioner modal window closes.
> >     setTimeout(function() {
> >       NewMailAccountProvisioner(msgWindow, { okCallback: okCallback });
> >-    }, 100);
> >+    }, 200);
> 
> If we're changing this to 200 (and was that intentional?), we should also
> change the comment above.

Yeah, intentional - see my second point in https://bugzilla.mozilla.org/show_bug.cgi?id=759163#c6.

> 
> Other than that, it seems good.  So I'm going to say r=me.
> 

Cool, thanks for the quick review.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2 (carrying over r+ from bwinton)

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3 (carrying over r+ from bwinton)

Forgot to add a comment to the quitObserver.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/8bcff760879f
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/c8a4d641f014
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/dfee2c01d8d9

Comment 12

[Mike Conley (:mconley) (:⚙️)]

STR for Litmus test (since this is hard to test with Mozmill):

1. Create a fresh TB profile
2. Wait for the Account Provisioner dialog to open on startup
3. Type a search into the input and hit the Search button.  Repeat this until a result comes up.  
4. When a result comes up, choose one so that an Account Provisioner tab opens to the provider's order form.
5. Close and restart Thunderbird (try closing both by File > Quit, and hitting Alt-F4 / closing the final 3pane window)

What is expected?

Thunderbird should go back into the last state, where we had an Account Provisioner tab opened, so that the user can continue their transaction.

We'd like our providers to resume the user session from where they left off.