The UX flows for migration call for the sync prefs panes to be updated, and I can't find an existing bug for that.

Here's a really dumb WIP - but it actually works and drives migration to completion!

Updated to reflect the changes in bug 1019985. It still the "upgrade" button and now also includes the "resend" and "forget" buttons.

I've updated this to use localizable strings and css instead of everything being inline. Still only incontent-prefs, but I figure it's worth getting feedback before I copy-paste it all into the windowed prefs.

Comment on attachment 8528142 [details] [diff] [review] 0010-Bug-1098661-very-rough-wip-for-sync-prefs.patch Review of attachment 8528142 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: browser/components/preferences/in-content/sync.js @@ +10,5 @@ > }); > > +XPCOMUtils.defineLazyGetter(this, "fxaMigrator", function () { > + let {fxaMigrator} = Components.utils.import("resource://services-sync/FxaMigrator.jsm"); > + return fxaMigrator; defineLazyModuleGetter? @@ +105,5 @@ > topics.forEach(function (topic) { > Weave.Svc.Obs.add(topic, this.updateWeavePrefs, this); > }, this); > + // The FxA migration observer is a special case. > + Weave.Svc.Obs.add(migrateTopic, this.updateMigrationState, this); You might want to pass in updateMigrationState bound to `this` to avoid surprises in the future when changing updateMigrationState, even though you don't use `this` in updateMigrationState right now. @@ +293,5 @@ > + // *sob* - Sync's Svc.obs wrapper omits |topic| from the args! > + updateMigrationState: function(subject, state) { > + if (!state) { > + // no migration action necessary. > + document.getElementById("sync-migration").setAttribute("hidden", "true"); .hidden = true @@ +304,5 @@ > + selIndex = 0; > + initPromise = Promise.resolve(); > + break; > + case fxaMigrator.STATE_USER_FXA_VERIFIED: > + Cu.import("resource://gre/modules/FxAccounts.jsm"); I'd prefer a defineLazyModuleGetter up top. @@ +314,5 @@ > + ); > + selIndex = 1; > + break; > + default: > + Cu.reportError("updateMigrationState has unknown state!"); Nit: You could catch the !state case here too to avoid the special case at the top. You could still log an error when !!state. @@ +315,5 @@ > + selIndex = 1; > + break; > + default: > + Cu.reportError("updateMigrationState has unknown state!"); > + // I guess hiding everything is the right thing here? I think so. @@ +320,5 @@ > + document.getElementById("sync-migration").setAttribute("hidden", "true"); > + return; > + } > + initPromise.then(() => { > + document.getElementById("sync-migration").removeAttribute("hidden"); .hidden = false ::: browser/components/preferences/in-content/sync.xul @@ +54,5 @@ > + > + <!-- When we are in the "need the user to be verified" state --> > + <hbox align="center"> > + <description> > + &migrate.verifyNeeded.before;<label id="sync-migration-address"/>&migrate.verifyNeeded.after; Hmm, a property string + formatStringFromName would be cleaner.

Comment on attachment 8528142 [details] [diff] [review] 0010-Bug-1098661-very-rough-wip-for-sync-prefs.patch Review of attachment 8528142 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/in-content/sync.js @@ +314,5 @@ > + ); > + selIndex = 1; > + break; > + default: > + Cu.reportError("updateMigrationState has unknown state!"); Should included the unknown state in the message, too.

I'm going to make this just in-content preferences as the "old" preferences UI will need additional UX love before it can be landed.

This addresses all previous review comments.

Comment on attachment 8535270 [details] [diff] [review] 0001-Bug-1098661-Update-sync-in-content-preferences-UI-to.patch Review of attachment 8535270 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, the only thing is that ideally we would share strings between this bug and bug 1016825 instead of duplicating them. Do you think that would be possible?

Pushed with de-duplication of strings. https://hg.mozilla.org/integration/fx-team/rev/069da43ec3be

Backed out in https://hg.mozilla.org/integration/fx-team/rev/a76b8d5ee8dc - one of the two things in that push, or both, was leaking prefs windows in bc1 and bc3

Repushed with a fix to the observer removal - this was causing the leak. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90ea0683e9e5 https://hg.mozilla.org/integration/fx-team/rev/b09fa41b6428

Are there UX specs available anywhere? What is "Forget" supposed to do?

Here's the spec for the migration state with the Forget button: https://www.dropbox.com/s/0l8aj6sjgjz0xbw/Sync%20Preverified.pdf Here's the spec for the state before that, where the user needs to upgrade to an FxA account: https://www.dropbox.com/s/qvhgcv2u9tdltpn/Sync%20Urgent%20Upgrade.pdf The "existing Forget this Email feature" the first one mentions is the link in the Sync pref pane that effectively cancels FxA account creation while Firefox is waiting for the user verify his account. That link's string is here: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/sync.dtd#74

Thanks, this definitely helps. I wonder if the action for "Forget" is clear enough, but the available space is also limited.

(In reply to Francesco Lodolo [:flod] from comment #14) > Are there UX specs available anywhere? What is "Forget" supposed to do? ... > Thanks, this definitely helps. I wonder if the action for "Forget" is clear > enough, but the available space is also limited. I agree "Forget" isn't obvious. "Logout" seems the most logical as this is only used in the context of an FxAccount and only after being logged in (not verified, but still logged in). Even worse though, after a "forget", the previous account name is still used as the default - so "Forget" is actually inaccurate. needinfo Ryan for his consideration.

Mark and I just discussed that “Reset” is a good option as it reverts to the pre-migratory state.

(In reply to Ryan Feeley from comment #18) > Mark and I just discussed that “Reset” is a good option as it reverts to the > pre-migratory state. But then we became less sure :) I think we agreed to let it slide for now and make a decision based on further feedback as the migration UX is seen by more eyes.

Verified on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.9.5 using latest Nightly 37.0a1 (buildID: 20141228030216) and the following mentions should be done here: - I've created a new account (which was not verified) -> on about:preferences#sync, I have "Verify Email" and "Forget this Email", but I don't see this inline notification: "A verification email awaits at user@domain" (with "Start Over" and "Resend" buttons) - it is expected? - I've logged in with an old account -> I don't see this inline notification : "We’ve rebuilt Sync to make it easier for everyone. Please upgrade to a Firefox Account to continue syncing." (with "Unlink Sync" and "Upgrade" buttons) - it is ok? when should I see this notification? Is there something I'm missing here?

This really can't be tested until more of the sync migration stuff lands and sync is configured with special test servers, so I believe everything mentioned above is expected. I'm verifying this and we will ensure there is a QA process so the entire migration flow can be effectively tested.