1026342 - Show sync migration indicators in hamburger panel

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

The UX for sync migration adds some requirements for notifications in the hamburger panel.  While this is not completely clear, see http://is.gd/sync_migration_ux_pdf for the current thinking.

Comment 1

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

FTR, this is the "Sync Migration Onboarding" part of the PDF linked above.  The "first launch only" part of that diagram isn't clear - the hamburger menu will be permanent (until migration or manual sync reset), it is "the blue-circle tickling of the Australis menu item" that only happens once.

Comment 2

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #1)
> FTR, this is the "Sync Migration Onboarding" part of the PDF linked above. 

*sigh* - it's more than that.  It covers every box in the UX flow that references that menu, so includes "Urgent Upgrade State", "Join the Party", "Sync Broken State" etc.

Comment 3

[Drew Willcoxon :adw]

Ryan, should the "Firefox Account required to sync" item from https://www.dropbox.com/s/dhqvooba5jeneer/Sync%20Onboarding.pdf and other PDFs persist across Firefox restarts?  In other words, after it first appears, does it stay in the menu until the user creates an FxA account?

Comment 4

[Ryan Feeley [:rfeeley]]

Attachment: unlink-sync.png

Yes Drew, yes, the states should persist. The user has to choose to upgrade or remove Sync. Currently we make that difficult by only doing that through repeated dismissals of the message.

If this is not enough, I will update the PDFs to include a more explicit Unlink Sync button that is available only in Prefs (see attached).

Mark, is it a good idea to include an Unlink Sync button in prefs for the Urgent state? And an equivalent negative option when Sync Broken?

Comment 5

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

I'll try and get some of my patches up soon, but I think the idea here is that the logic will be something like what I've got as a WIP for the "sync prefs" stuff:

    let service = Components.classes["@mozilla.org/weave/service;1"]
                  .getService(Components.interfaces.nsISupports)
                  .wrappedJSObject;
    // Update the state of the migrate UI.
    // Avoid pulling in the migrate module if we are already FxA.
    document.getElementById("sync-migration").setAttribute("hidden", "true");
    if (!service.fxAccountsEnabled) {
      // we only show when the migrator isn't either "complete" or "waiting for eol"
      fxaMigrator.promiseCurrentState().then(state => {
        if ([fxaMigrator.STATE_COMPLETE, fxaMigrator.STATE_WAITING_OFFER].indexOf(state) == -1) {
          document.getElementById("sync-migration").removeAttribute("hidden");
        }
      });
    }

plus an observer for "fxa-migration:state-changed" which re-runs that code

ie:
* If we are already using Fxa, do nothing.
* import FxaMigrator module (in the above example, it's a lazy import, so that first reference is the import)
* if it is "complete" or waiting for an "offer" (ie, EOL notification) from the server, do nothing (but "complete" is really a synonym for "are we using Fxa", so that check is actually redundant)
* Show the migrate UI

This means that (a) the state automagically persists across restarts and (b) allows us to keep the sync server as the "trigger" for this.  A small downside is that the first run means the "migrate" UI is not going to show up until after the first sync for the client, but Ryan previously said that was OK and is actually a feature in that we can ask services to remove the EOL header if the poo hits the fan (or we want to throttle, etc).  However, note that as Sync already stores the EOL state in a pref, a subsequent restart *will* cause the notification to happen immediately.

(In reply to Ryan Feeley from comment #4)

> Mark, is it a good idea to include an Unlink Sync button in prefs for the
> Urgent state? And an equivalent negative option when Sync Broken?

Sync will already have an unlink button in its prefs - we might want to make it more prominent though.  Note however that I'm somewhat ignoring that "urgent" state for now as we will have time to fix that in a later cycle if necessary - there's no way we will transition from "soft-eol" to "hard-eol" in one release cycle - I'd expect soft-eol to live for quite a few months.

Comment 6

[Drew Willcoxon :adw]

(In reply to Mark Hammond [:markh] from comment #5)
> I'll try and get some of my patches up soon, but I think the idea here is
> that the logic will be something like what I've got as a WIP for the "sync
> prefs" stuff:
> 
> ...
> 
> plus an observer for "fxa-migration:state-changed" which re-runs that code

Thanks, that makes sense to me.

Comment 7

[Drew Willcoxon :adw]

Attachment: WIP patch

WIP that doesn't address what we just talked about re: persisting EOL state via the fxaMigrator state.  It makes these other changes in addition to showing the menu panel item on EOL that I'd like feedback on:

- consolidate duplicated code for opening about:accounts
- gSyncUI delegates to gFxAccounts to open about:accounts
- the syncErrorPanel thing passes in appropriate entrypoint instead of using "menupanel"
- get rid of the "signedin" and "failed" attributes in favor of using a single "fxastatus" attribute with "signedin" and "error" values.
- set fxastatus=signup on EOL so that when you click the menu item panel, you can sign up!
- gSyncUI is already setting status=active during sync.  to avoid confusion with fxastatus, and because it's sync status and not fxa status, rename that attr "syncstatus".

Comment 8

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

ack - sorry Drew, I ended up shaving yaks today - I'll get to that first thing tomorrow.

Comment 9

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8523266 [details] [diff] [review]
WIP patch

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

Looking fine to me

::: browser/base/content/browser-syncui.js
@@ +148,5 @@
>      let panelHorizontalButton = document.getElementById("PanelUI-fxa-status");
>      [syncButton, panelHorizontalButton].forEach(function(button) {
>        if (!button)
>          return;
> +      button.removeAttribute("syncstatus");

why this change?

Comment 10

[Drew Willcoxon :adw]

Attachment: patch without tests

Still need to write tests... I'll probably be asking for your help with that, Mark.  The one XXX comment isn't meant to be permanent.

This handles both the need-user and need-verification states in the menu panel.  It works in my limited manual testing.

One thing I realized is that I didn't need to ask fxaMigrator for the initial state at all.  I just send an fxa-migration:state-request and listen for the response.

(In reply to Mark Hammond [:markh] from comment #9)
> ::: browser/base/content/browser-syncui.js
> @@ +148,5 @@
> >      let panelHorizontalButton = document.getElementById("PanelUI-fxa-status");
> >      [syncButton, panelHorizontalButton].forEach(function(button) {
> >        if (!button)
> >          return;
> > +      button.removeAttribute("syncstatus");
> 
> why this change?

See the fourth and six bullets in comment 7.  I actually didn't mean to change sync-button's attribute, only PanelUI-fxa-status's (maybe that's what you're referring to?), so this patch fixes that.  Let me know if I need to elaborate.

Comment 11

[Drew Willcoxon :adw]

Attachment: patch without tests v1.1

Whoops, forgot to hg add accounts.properties.  Those strings come from these PDFs that are linked from the migration flow PDF:

https://www.dropbox.com/s/dhqvooba5jeneer/Sync%20Onboarding.pdf
https://www.dropbox.com/s/0l8aj6sjgjz0xbw/Sync%20Preverified.pdf

Comment 12

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8526387 [details] [diff] [review]
patch without tests v1.1

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

Nice progress, but I don't think it's ready to land, so only f+.  For tests, you should look at browser_aboutaccounts to see how the FxA account status can be used in tests, and I think we should just have a "mock" for the Migrator - ie, don't bother having sync fully configured etc, but instead just send the observer notifications you care about and check the UI reacts accordingly.

::: browser/base/content/browser-fxaccounts.js
@@ +234,5 @@
>        doUpdate(null);
>      });
>    },
>  
> +  showMigrationUI: function () {

I think using Task.async() is nicer.

@@ +240,5 @@
> +      let status = null;
> +      let label = null;
> +      if (this._migrationState == fxaMigrator.STATE_USER_FXA_VERIFIED) {
> +        let user = yield fxAccounts.getSignedInUser();
> +        if (!user) {

I think you should probably just Cu.reportError() and bail here - it doesn't seem to make sense to have this._migrationState be different than what the migration module is seeing and could lead to hard to explain strangeness.

@@ +273,5 @@
> +      break;
> +    case "error":
> +      this.openAccountsPage("menupanel", "reauth");
> +      break;
> +    case "signup":

we are going to need to use the migrator module here as it has logic to try and see if there is an email address we should use.  But yeah, that's tricky before that lands :(

But still, we need to differentiate between 2 states:

* Sync is not configured at all - to start the user needs to signup.
* We need to migrate - the user needs to sign up

But they need to be handled differently.  I *think* that first case is handled by the "default" - which implies to me that the "fxastatus" in the second case should be something like "migrate-signup".

@@ +278,5 @@
> +      this.openAccountsPage("menupanel", "signup");
> +      break;
> +    case "verify":
> +      //XXX Is there an action meaning "verify your account"?
> +      this.openAccountsPage("menupanel", "reauth");

This one has a similar issue but with slightly different considerations.  In the "not migrating - user is manually creating a sync account" case there is still a possibility the user is in an "unverified" state.  Currently that's treated as being "signed in" - if the user selects that item they'll go to sync-prefs and only there will they notice they are unverified and be able to take action.

However, in the migration case things are a little different - going to Sync prefs would show legacy sync, so there's no opportunity there to handle the "not verified" case - so clicking the hamburger in this state does the "resend mail" thing directly.

So again, this really is only "unverified when migrating" and *not* "unverified when normally creating a new account" and I think this should be reflected in the state strings.

@@ +292,5 @@
>    openPreferences: function () {
>      openPreferences("paneSync");
>    },
>  
> +  openAccountsPage: function (entryPoint, action=null) {

See bug 1100666 - we are probably going to want to pass multiple params, specifically "migration=true".  So we could either change this to accept an object with arbitrary additional params, or just an isMigration=false param.

(Although IMO you should consider a single param being the object of arbitrary params - action and entrypoint are both just params, and arguably action is the most important - that changes the "look" of the page while entryPoint and migration are purely for metrics) - IOW, getting entryPoint wrong or omitting it would be only slightly bad, whereas getting action wrong or incorrectly omitting it will screw the user flow.

::: browser/locales/en-US/chrome/browser/accounts.properties
@@ +1,1 @@
> +# LOCALIZATION NOTE (migrate.needUser)

I'd be inclined to name this file something like sync_migration.properties

Comment 13

[Drew Willcoxon :adw]

(In reply to Mark Hammond [:markh] from comment #12)
> I think we should just have a "mock" for the Migrator

Ah, of course.

> But still, we need to differentiate between 2 states:
> 
> * Sync is not configured at all - to start the user needs to signup.
> * We need to migrate - the user needs to sign up
> 
> But they need to be handled differently.  I *think* that first case is
> handled by the "default" - which implies to me that the "fxastatus" in the
> second case should be something like "migrate-signup".

OK, aside from getting the Legacy Sync email address from fxaMigrator and passing it to about:accounts here, I think you're only saying the "signup" fxastatus should be renamed "migrate-signup"?  Sounds fine.  But in case I misunderstand, I'll outline these two cases below as I understand them:

The fxastatus attribute is set by updateUI and showMigrationUI.

In the first case, you're right.  fxastatus would not be present at all: there is no FxA status so updateUI would not have set it, and we would not have received any fxaMigrator notifications so showMigrationUI would not have set it.  Therefore onMenuPanelCommand would hit the `default`, opening about:accounts with no action.

In the second case, we get the fxaMigrator notification, so showMigrationUI sets fxastatus to "signup".

> @@ +278,5 @@
> > +      this.openAccountsPage("menupanel", "signup");
> > +      break;
> > +    case "verify":
> > +      //XXX Is there an action meaning "verify your account"?
> > +      this.openAccountsPage("menupanel", "reauth");
> 
> This one has a similar issue but with slightly different considerations.  In
> the "not migrating - user is manually creating a sync account" case there is
> still a possibility the user is in an "unverified" state.  Currently that's
> treated as being "signed in" - if the user selects that item they'll go to
> sync-prefs and only there will they notice they are unverified and be able
> to take action.

Right, gotcha.

> However, in the migration case things are a little different - going to Sync
> prefs would show legacy sync, so there's no opportunity there to handle the
> "not verified" case - so clicking the hamburger in this state does the
> "resend mail" thing directly.

So you're saying I should not call this.openAccountsPage() but instead call some method on the fxaMigrator that resends the email directly?  FWIW, in my testing, opening about:accounts with "reauth" in this case sends me first to a login page where I type only my password, and after that to a "A verification link has been sent" page, which seems OK to me.  And indeed I do get a new verification email.

> So again, this really is only "unverified when migrating" and *not*
> "unverified when normally creating a new account" and I think this should be
> reflected in the state strings.

Makes sense, yeah.

> @@ +292,5 @@
> >    openPreferences: function () {
> >      openPreferences("paneSync");
> >    },
> >  
> > +  openAccountsPage: function (entryPoint, action=null) {
> 
> See bug 1100666 - we are probably going to want to pass multiple params,
> specifically "migration=true".  So we could either change this to accept an
> object with arbitrary additional params, or just an isMigration=false param.

I'll change the prototype to one of these:

openAccountsPage: function (action=null, urlParams={}) {
openAccountsPage: function (urlParams={}) {

The reason for considering the second is that there are sites that don't pass an action but do pass an entryPoint, IIRC.

> ::: browser/locales/en-US/chrome/browser/accounts.properties
> @@ +1,1 @@
> > +# LOCALIZATION NOTE (migrate.needUser)
> 
> I'd be inclined to name this file something like sync_migration.properties

OK.  But I do think accounts is a better name because we may need other Account-related property strings in the future, and it's unnecessary to have property files for both.

Comment 14

[Drew Willcoxon :adw]

Should've added this dependency a while ago...

Comment 15

[Drew Willcoxon :adw]

One other thing, after jumping to your latest patch in bug 1019985 and then back to my patch here...

(In reply to Mark Hammond [:markh] from comment #12)
> @@ +278,5 @@
> > +      this.openAccountsPage("menupanel", "signup");
> > +      break;
> > +    case "verify":
> > +      //XXX Is there an action meaning "verify your account"?
> > +      this.openAccountsPage("menupanel", "reauth");
> 
> ...
> 
> However, in the migration case things are a little different - going to Sync
> prefs would show legacy sync, so there's no opportunity there to handle the
> "not verified" case - so clicking the hamburger in this state does the
> "resend mail" thing directly.

fxAccounts.resendVerificationEmail doesn't put up any UI feedback telling the user that an email's been sent and they should check their inbox.  If clicking this menu panel button does send an email directly, we'll need to do that.  Otherwise there's no feedback at all that anything happened.

Opening about:accounts?action=reauth satisfies that pretty well I think.  Or can we call fxAccounts.resendVerificationEmail() and then open the page that says "verification link has been sent"?

Comment 16

[Drew Willcoxon :adw]

Attachment: patch without tests v2

I'll work on a test next if this is OK.

Re: openAccountsPage, it seems to me that all callers should pass in entryPoint, but all other params are optional.

Comment 17

[Drew Willcoxon :adw]

Oh, this doesn't implement the blue highlight bubble in the spec.  I couldn't get it to work right and would really not like to block this bug on it.

Comment 18

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8527054 [details] [diff] [review]
patch without tests v2

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

Looking very nice, but I do think another round is necessary (mainly for reasons of not your making :).  For my info, what do you intend with the tests once this is ready for r+?  ie, open another bug, [leave-open] this one, request re-review, or maybe something else?

::: browser/base/content/browser-fxaccounts.js
@@ +271,4 @@
>        this.openPreferences();
> +      break;
> +    case "error":
> +      this.openAccountsPage("menupanel", { action: "reauth" });

Sorry, but the action really does need to be the first param.  I'd suggest we even make entryPoint optional in this module.  As I mentioned, getting entryPoint wrong means wrong metrics, getting action wrong means not working.

(entryPoint is a bad name from this perspective - it should be something like "whatTheUserClickedOnToGetHere" :) - see later comments for more context.

::: browser/base/content/browser-syncui.js
@@ +144,5 @@
>      if (!gBrowser)
>        return;
>  
>      let syncButton = document.getElementById("sync-button");
> +    if (syncButton) {

ouch - sorry about asking for that attribute to change :( If you think it would be better to leave it as it was, feel free to do so :)

@@ -373,5 @@
>      openPreferences("paneSync");
>    },
>  
>    openSignInAgainPage: function (entryPoint = "syncbutton") {
> -    // If the user is also in an uitour, set the entrypoint to `uitour`

Unfortunately, browser-syncui and browser-fxaccounts are in a bit of a mess (which in many ways is just a reflection of the fact that the distinction between an FxA and a "sync account" is also a bit of a mess).

The fact there was an openSignInAgainPage() in both of them is a reflection of this :(  OTOH though, if you want to kill one, why not kill both?  Alternatively, keep the one in -fxaccounts and it *can* have entry-point as the only arg (with action= being implied) - but "openAccountsPage" should definitely have action as the first param IMO.

Also, moving this UITour-checking code means that with this and the browser.xul change, the entryPoint may change to "uitour" when it previously wouldn't have.  I can't see why that would be a problem in practice - the 2 things coinciding would be extremely unlikely, so I think a single helper openSignInAgainPage would make things clearer and cleaner (ie, kill this one and add it back to -fxaccounts as a small helper for openAccountsPage, with the uitour checking in the latter)

::: browser/base/content/browser.xul
@@ +470,5 @@
>              <spacer flex="1"/>
>              <button class="sync-panel-button"
>                      label="&syncErrorPanel.signInButton.label;"
>                      accesskey="&syncErrorPanel.signInButton.accesskey;"
> +                    onclick="gFxAccounts.openAccountsPage('syncErrorPanel', { action: 'reauth' });"/>

IIUC, this is creating a new "entryPoint" which we don't want without coordination with the server team collecting the metrics - but as per the last note, sticking with gFxAccounts.openSignInAgainPage() sounds best to me.

Comment 19

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw from comment #15)
> fxAccounts.resendVerificationEmail doesn't put up any UI feedback telling
> the user that an email's been sent and they should check their inbox.  If
> clicking this menu panel button does send an email directly, we'll need to
> do that.  Otherwise there's no feedback at all that anything happened.

Yeah - although I think sync prefs does something when that's clicked.  We might just have to make that public somehow/where.

> Opening about:accounts?action=reauth satisfies that pretty well I think.

Nah, that would bee wrong - reauth means you need to reenter your password (as you changed it somewhere else), which you don't here.

> can we call fxAccounts.resendVerificationEmail() and then open the page that
> says "verification link has been sent"?

Yeah - sadly there's no such link (ie, that's shown by the server after you click its "resend" link, and doesn't have an entry-point :(  I agree it should though.

Comment 20

[Drew Willcoxon :adw]

(In reply to Mark Hammond [:markh] from comment #18)
> For my info, what do you intend with the tests once this is ready for r+?

I think I'll just include a test in the next patch.  In addition to thinking the current patch was ready, I wanted to ask for review mainly so I could say that this bug was in review on my iteration status report.  The system works! :-/

Comment 21

[Drew Willcoxon :adw]

Attachment: patch with WIP test

Addresses your review comments + the beginning of a test.  Doing the actual assertions in the test is easy.  The hard part is setting up FxA and Sync to make the migration machinery run.  I thought I could get away with broadcasting notifications that mimic the migrator, but my code checks the signed in user in the STATE_USER_FXA_VERIFIED state.

Comment 22

[Drew Willcoxon :adw]

The fact that FxAccounts doesn't queue up requests is a real problem IMO.  It means it's really easy to get errors like "a different user is signed in."  If setSignedInUser is called while getSignedInUser or some other getter is still processing, it ought to be queued up to wait until that processing is done.

Making matters worse, it appears consumers don't catch rejected promises in some cases, like aboutaccount's getSignedInUser call in its init().  Given the non-queuing design of the back end, at least the front end should be prepared to handle rejections in all cases, but even that's not true.

Comment 23

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Drew Willcoxon :adw from comment #22)
> The fact that FxAccounts doesn't queue up requests is a real problem IMO. 
> It means it's really easy to get errors like "a different user is signed
> in."  If setSignedInUser is called while getSignedInUser or some other
> getter is still processing, it ought to be queued up to wait until that
> processing is done.
> 
> Making matters worse, it appears consumers don't catch rejected promises in
> some cases, like aboutaccount's getSignedInUser call in its init().  Given
> the non-queuing design of the back end, at least the front end should be
> prepared to handle rejections in all cases, but even that's not true.

TBH I think this is just a problem in theory (or in tests) rather than in practice - and how to handle the error state isn't clear.

Consider about:accounts - we will be opening this on response to some UI action.  The chance of *another* UI action being initiated which causes a user to log out (or another to log in) while about:accounts is fetching the current user seems tiny.  If it *did* happen, it's not clear the correct answer would simply be to Cu.reportError() and take no further action - maybe about:accounts should retry?  Maybe it should close?  But regardless, I bet we *never* see it in practice, and queueing would be the wrong thing to do here anyway - if the user does manage to log in as a different user which it is loading, we *do not* want about:accounts to reflect the previous user.  There is a single "current state" and a queue would transition through invalid states.

IMO it's not that different than exceptions.  We clearly do not attempt to catch and handle *every possible* exception that could ever be seen as (a) we don't even know what they all are (think OOM or disk failure) and (b) there's no clear remediation for some of them.  We catch ones we can handle and let unhandled ones bubble up, and sometimes we note these, treat them as a bug, decide how it should be handled and implement a fix.

And last but not least, I think this "unhandled rejection" thing actually saves our sanity in this case.  If we did catch the rejection and ignored it, we'd have a much harder time deducing why it doesn't work (and it still wouldn't have worked)

Comment 24

[Drew Willcoxon :adw]

Attachment: patch with test

This includes the double-update() at the end of migration like we talked about.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6269c7046b7e

Comment 25

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8532747 [details] [diff] [review]
patch with test

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

Awesome :)

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ -103,5 @@
>                    .getService(Ci.nsISupports)
>                    .wrappedJSObject;
>  
> -    // Don't show about:accounts with FxA disabled.
> -    if (!weave.fxAccountsEnabled) {

I'm pretty sure this change will die once you rebase against trunk

::: browser/base/content/browser-fxaccounts.js
@@ +88,5 @@
>      gNavToolbox.addEventListener("customizationending", this);
>  
> +    // Request the current Legacy-Sync-to-FxA migration status.  We'll be
> +    // notified of fxa-migration:state-changed in response if necessary.
> +    Services.obs.notifyObservers(null, "fxa-migration:state-request", "");

I thought sending the data as |null| was more common when it's unused?

@@ +191,5 @@
>      if (!this.weave.fxAccountsEnabled) {
> +      // When we're notified at the end of migration as migration transitions
> +      // from needs-verification to the null state, fxAccountsEnabled is not yet
> +      // true, because migration has not actually finished.  In that case, hide
> +      // the button.

I'd add " - we'll get another notification with a null state once migration is complete."

@@ +264,5 @@
> +    if (label && status) {
> +      this.button.label = label;
> +      this.button.setAttribute("fxastatus", status);
> +      this.button.hidden = false;
> +    }

it looks to me like an "} else { Cu.reportError()" might be helpful to a future us here, but I'm not that bothered (it would actually be a good use-case for that new diagnostic facility Yoric is working on, but that doesn't exist yet...)

@@ +296,5 @@
>      openPreferences("paneSync");
>    },
>  
> +  openAccountsPage: function (action, urlParams={}) {
> +    if (UITour.originTabs.get(window) &&

I should have mentioned this before, but I think we could do with a comment along the lines of "An entryPoint param is used for server-side metrics.  See if the UITour is open and replace any such param to reflect this was selected while the UITour was active"

::: browser/base/content/test/general/browser_fxa_migrate.js
@@ +18,5 @@
> +Cu.import("resource://gre/modules/FxAccounts.jsm", imports);
> +Cu.import("resource://services-sync/FxaMigrator.jsm", imports);
> +
> +add_task(function* test() {
> +  // At the beginning the button should have no fxastatus, but it's visible and

TBH I'd remove this check as it is somewhat fragile.  As we've set the services.sync.username pref above, we've "1/2 tricked" Sync into thinking there's a legacy account enabled.  But if we'd *fully* tricked Sync, this block wouldn't be true - it wouldn't say "Sign into Sync" but would instead be invisible.  So I'd remove these 3 lines as they are really just testing the "1/2 tricked" aspect rather than anything we care about.

@@ +52,5 @@
> +  newState = yield stateChangedPromise;
> +  Assert.equal(newState, null, "Migration state should now be: done");
> +
> +  // A little tricky here.  First we pass through a window where there's a null
> +  // migration status yet an FxA user is not set up.  The button is hidden

s/yet an FxA user is not set up/yet Sync is not yet configured with our FxA user/

::: browser/locales/en-US/chrome/browser/syncMigration.properties
@@ +1,2 @@
> +# LOCALIZATION NOTE (needUser)
> +# %S = Firefox Accounts brand name from syncBrand.dtd

I apologize that I already asked for this to be renamed, but it would be great if it can be renamed again so we can later add the "resend verification mail" strings to this file.  Maybe syncFxA.properties would make sense (just FxA.properties might not be great as we start to see non-sync-related FxA stuff land (OTOH though, maybe that *would* still make sense as the number of strings is relatively small so a single file with sync and non-sync FxA strings doesn't really sound like a problem).  Your call :)

::: services/sync/modules/FxaMigrator.jsm
@@ +218,5 @@
>      this.log.info("scheduling initial FxA sync.");
>      this._unblockSync();
>      Weave.Service.scheduler.scheduleNextSync(0);
> +
> +    // Tell the front end that the sentinel has been set and migration is now

I'd remove the sentinel comment here - just "... that migration is now complete" (or maybe "that sync is now configured with the FxA user")

Comment 26

[Drew Willcoxon :adw]

Attachment: patch with test v2

Let's see how this looks on try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c768bce03f63

Comments addressed.  This also fixes a test failure on try due to the fact that the test was doing this:

Services.obs.notifyObservers(null, "weave:eol", null);

Which tripped up gSyncUI.onEOLNotice because it expects the observer subject to be non-null.  So I change the first arg passed to notifyObservers (the subject param) to: { code: "soft-eol" }.

There were other test failures I'm not sure will be fixed by this.  I'm wondering whether my test will be bitten if there are legacy Sync tests that run before it.

(In reply to Mark Hammond [:markh] from comment #25)
> ::: browser/base/content/browser-fxaccounts.js
> @@ +88,5 @@
> >      gNavToolbox.addEventListener("customizationending", this);
> >  
> > +    // Request the current Legacy-Sync-to-FxA migration status.  We'll be
> > +    // notified of fxa-migration:state-changed in response if necessary.
> > +    Services.obs.notifyObservers(null, "fxa-migration:state-request", "");
> 
> I thought sending the data as |null| was more common when it's unused?

Changed to null.  IMO it's more correct to pass an empty string since the last param is a wstring in the IDL, but I'm pretty sure XPConnect coerces null into the empty string, and in any case null works just as well, so no problem.

> ::: browser/locales/en-US/chrome/browser/syncMigration.properties
> @@ +1,2 @@
> > +# LOCALIZATION NOTE (needUser)
> > +# %S = Firefox Accounts brand name from syncBrand.dtd
> 
> I apologize that I already asked for this to be renamed, but it would be
> great if it can be renamed again so we can later add the "resend
> verification mail" strings to this file.

No problem.  I switched back to "accounts.properties" from my original patch, and I moved the entry in the jar.mn back outside of the #ifdef MOZ_SERVICES_SYNC.  "Accounts" reads better to me than "FxA", but let me know if you feel strongly and I'll change it.

Comment 27

[Drew Willcoxon :adw]

Try is still failing.  The first error, among others, is:

0:52.01 TEST_STATUS:  Migration state should now be: need a verified user - null == "waiting for a verified FxA user" FAIL JS frame :: chrome://mochitests/content/browser/browser/base/content/test/general/browser_fxa_migrate.js :: test :: line 45

I can reproduce locally when I run browser_aboutAccounts.js and my new test in the same run.  (Not sure if there's something special about browser_aboutAccounts.js.  It's just the first test in browser.ini that runs on OS X debug.  I commented out all other test entries in browser.ini and ran only these two.)

Do I have to "reset" legacy Sync or something, similar to how I had to be careful not to import service.js before setting services.sync.username?  I mean I can't assume that no other test that ran before mine hasn't caused service.js to be imported.

Comment 28

[Drew Willcoxon :adw]

Attachment: partial patch against attachment 8533315, tests passing

Thanks for the help, Mark.  With this patch applied against the previous patch I posted, attachment 8533315 [details] [diff] [review], running multiple tests passes locally for me.  (Running my new test in isolation has always passed.)

I'll post the combined, hopefully final patch next.

Comment 29

[Drew Willcoxon :adw]

Attachment: patch with test v3

I'm expecting this to pass now: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2274aef80d20

Comment 30

[Drew Willcoxon :adw]

Nope:

TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver] at chrome://browser/content/browser.js:8777

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-syncui.js?rev=f0e0f05aa2d3#59

unload was fired on the window before gSyncUI.initUI was ever called?  *And* Weave.Status.ready was true?

Plus a leak on some runs.

Comment 31

[Drew Willcoxon :adw]

Ah, weave:notification:added is being removed twice, so the error happens the second time.

* initUI adds weave:notification:added to _obs, so we add an observer for weave:notification:added
* The `if (gBrowser && Weave.Notifications.notifications.length)` in initUI is false, so we don't call initNotifications at this point, so the weave:notification:added observer is not removed
* Later, we observe weave:notification:added, so we call initNotifications
* initNotifications removes the observer for weave:notification:added
* Later, the window unloads, so we remove all observer topics in _obs, including weave:notification:added

Seems like it's a latent bug that initNotifications doesn't remove weave:notification:added from _obs when it removes the observer for it.  Or is there a legitimate expectation that the window won't be unloaded if initNotifications is called?  That would be something.

Is my test triggering this because it triggers a notification/infobar due to the EOL message?

Comment 32

[Drew Willcoxon :adw]

Attachment: patch with test v4, a new approach

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=76540aa1ab4d

This sends migration notifications within the test like I mentioned on IRC.  Passes locally for me.  It misses out on accurately testing the UI at the very end of migration, but oh well.

Sorry I don't have a diff between this and my previous patch.  The changes are:

In my previous patches, gFxAccounts.showMigrationUI was calling fxAccounts.getSignedInUser to get the email address to it could include it in the button label in the needs-verification state.  That messes up my test now, because there is no signed in user, so I replaced that with sending the signed-in user's email in the migrator notification when transitioning to the needs-verification state.  I like how that further separates the model from the view as an unintended consequence.

This also includes the fix to the latent bug in gSyncUI where it removed the weave:notification:added observer twice, as we discussed on IRC.

Finally this updates test_fxa_migration.js to account for the email addressed being passed in the migration notification.

Comment 33

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 8534074 [details] [diff] [review]
patch with test v4, a new approach

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

LGTM!

::: browser/base/content/browser-fxaccounts.js
@@ +139,5 @@
>      }
>    },
>  
> +  onMigrationStateChanged: function (newState, email) {
> +    this._migrationInfo = !newState ? null : {

I think "newState == null" reads better than !newState, but don't care much

Comment 34

[Drew Willcoxon :adw]

Thanks so much for all your help with this, Mark.

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Francesco Lodolo [:flod]]

Missing license header in accounts.properties
I guess it would be fast to just land a followup for that that opening a new bug.

Comment 37

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Francesco Lodolo [:flod] from comment #36)
> Missing license header in accounts.properties
> I guess it would be fast to just land a followup for that that opening a new
> bug.

Good spot, thanks! I hope to land changes to this in the next few days, so hopefully between Drew and I we can remember that - which sounds even easier :)

Comment 38

[Drew Willcoxon :adw]

My final patch in bug 1016825 will add the header.

Comment 39

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 37.0b3
Build id:20150305191659
OS: Mac Os X 10.9.5, Ubuntu 14.04 x64, Win 7 x64 

Both "[e-mail] not verified" and "Firefox Account required for sync" indicators are present in the hamburger menu as soon as the migration state is determined. 

However I did not notice the blue circle notification is that still suppose to show?

Comment 40

[Drew Willcoxon :adw]

Thanks Catalin, we never implemented the blue circle, so it is not expected to appear.