1112440 - Infobar doesn't update correctly to reflect "needs verified user" on about:accounts

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

STR:
* Arrange for migration infobar.
* Click on the "upgrade" button.

At this point about:accounts opens and the infobar hides.

* Create FxA account, login, and close about:accounts

Expected:
* Infobar re-appears.
Actual:
* Nothing.

Similarly, but probably less important:

* After clicking "Upgrade", close about:accounts without creating an account.  Then continue browsing as normal, and at no point does the infobar reappear.

Comment 1

[Drew Willcoxon :adw]

Is this about the infobar in the main browser window (bug 1016825) or the in-content prefs infobar (bug 1098661, which you landed right after filing this)?  I wouldn't expect the main browser infobar to appear in the steps you've described, but I also don't understand what you mean by login, since at that point you're already "logged in" in an unverified state.

I looked at Ryan's flow chart and I think we're missing this after you successfully migrate, but it doesn't sound like what you're talking about: https://www.dropbox.com/s/32d83uxd5patinv/Sync%20Success.pdf

(In reply to Mark Hammond [:markh] from comment #0)
> Similarly, but probably less important:
> 
> * After clicking "Upgrade", close about:accounts without creating an
> account.  Then continue browsing as normal, and at no point does the infobar
> reappear.

Yeah, we'll likely want a timer within the migrator module to retrigger its user-state notifications?  Did Ryan have anything to say about that?  (No, as far as I know.)

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw from comment #1)
> Is this about the infobar in the main browser window (bug 1016825) or the
> in-content prefs infobar (bug 1098661, which you landed right after filing
> this)?

I mean the infobar in the main browser window.

To put it another way: The infobar shows an "upgrade" button.  You click it and the infobar hides.  Best I can tell the infobar will never again re-show again in this session.  In particular, it will never show when in the "needs verify" state.

> Yeah, we'll likely want a timer within the migrator module to retrigger its
> user-state notifications?  Did Ryan have anything to say about that?  (No,
> as far as I know.)

Other options:
* It always and unconditionally shows as the migration state changes.
* It always shows when about:accounts is *not* the current tab.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

I chatted with Ryan about this.  He came up with:

* The infobar should hide after the user takes any action with it - this includes closing it (ie, just clicking the 'x', or clicking any of the buttons.

* The infobar should only re-appear when the migration state changes (ie, as it transitions to "needs verify" and on browser restart.

This means the "dismiss" and "take action" behaviour is the same.  If the user never completes the FxA account process or just dismisses the bar, it doesn't come back until next restart.

Comment 4

[Drew Willcoxon :adw]

Thanks, Mark.  That's the infobar's behavior now unless there's some bug.  The only wrinkle is that I chose to not show it at all if about:accounts is the current tab, as I mentioned in bug 1016825 comment 6.  Is there anything to do here?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw (Away 12/22–1/2) from comment #4)
> Thanks, Mark.  That's the infobar's behavior now unless there's some bug. 
> The only wrinkle is that I chose to not show it at all if about:accounts is
> the current tab, as I mentioned in bug 1016825 comment 6.  Is there anything
> to do here?

Yeah, I think we should revert that.  Consider:

* infobar is showing, and user selects "upgrade" from Sync prefs pane.

(here, about:accounts opens but the existing notification does *not* hide - this isn't that much of a problem, but...)

* In about:accounts the user creates the account and about:accounts changes to reflect "please verify your account)

Here, the infobar remains in the "needs fxa user" state - it does *not* change to reflect the new "needs verified user" state - so the infobar here is actually wrong.

My digging shows that is due to this special handling of about:accounts.

(I *think*, but I'm not sure, that the infobar even remains after the migration is complete if about:accounts is current)

So it seems like we have 2 options:

1) Make the about:accounts handling smarter, so it never shows in about:accounts (ie, we'd have to listen for tab switch changes etc).  This would mean the infobar *does* go away if the user starts migration from Sync Prefs.

2) Nuke all about:accounts special casing - that would fix the problems above, but still leave us with an infobar on about:accounts when started from Prefs.

I think 2 is probably fine (and certainly easier ;)

FWIW, you can see the issue with the "needs verify" state not working correctly by running the following script in a browser scratch-pad, and toggling that boolean condition.  The infobar updates state correctly *except* when it is showing in about:accounts:

--8<--
Cu.import("resource://services-sync/FxaMigrator.jsm");

let STATE_CHANGED_TOPIC = "fxa-migration:state-changed";

if (true) {
  Services.obs.notifyObservers(null, STATE_CHANGED_TOPIC,
                               fxaMigrator.STATE_USER_FXA);
} else {
  let email = Cc["@mozilla.org/supports-string;1"].
              createInstance(Ci.nsISupportsString);
  email.data = "[e-mail]";
  Services.obs.notifyObservers(email, STATE_CHANGED_TOPIC,
                               fxaMigrator.STATE_USER_FXA_VERIFIED);
}
--8<--

Comment 6

[Drew Willcoxon :adw]

Attachment: don't special-case about:accounts at all

I agree 2 is probably fine, for now at least.  We can always do something smarter later if people think we should.

Comment 7

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/f8cdb129a54e

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f8cdb129a54e