1113493 - Maintain sync engine state and offer customization as part of migration.

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

Attachment: 0002-Bug-XXXXXXX-maintain-engine-state-and-offer-engine-c.patch

This was kinda bug 1098694, but that morphed into the server side.  There are 2 birds we can kill with one stone here:

* Before we reset sync we should note the enabled state of the engines and re-configure those states after we reset.

* As described in bug 1098694, we should offer the user to choose what to sync if there are any disabled engines.

I haven't done full testing of this as the eol staging servers are, for some reason, no longer returning eol notifications, but figure I might as well get feedback now.

Comment 1

[Drew Willcoxon :adw]

Comment on attachment 8539006 [details] [diff] [review]
0002-Bug-XXXXXXX-maintain-engine-state-and-offer-engine-c.patch

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

Looks fine to me.

::: services/sync/tests/unit/test_fxa_migration.js
@@ +98,3 @@
>    do_register_cleanup(() => {
>      Services.prefs.setBoolPref("services.sync-testing.startOverKeepIdentity", oldValue)
> +    Services.prefs.setBoolPref("services.sync.engine.addons", true);

clearUserPref?

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8539006 [details] [diff] [review]
0002-Bug-XXXXXXX-maintain-engine-state-and-offer-engine-c.patch

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

FxaMigrator -> FxAMigrator.

::: services/sync/modules/FxaMigrator.jsm
@@ +216,5 @@
>                                   this.STATE_INTERNAL_WAITING_WRITE_SENTINEL);
>      yield this._setMigrationSentinelIfNecessary();
>  
> +    // Get the list of enabled engines to we can restore that state.
> +    let enginePrefs = this._getEngineEnabledPrefs();

Declined, too?

@@ +342,5 @@
> +    let allPrefs = ["bookmarks", "history", "tabs", "passwords", "addons", "prefs"];
> +    let result = [];
> +    for (let pref of allPrefs) {
> +      let fullPref = "services.sync.engine." + pref;
> +      result.push([fullPref, Services.prefs.getBoolPref(fullPref)]);

You probably want to do one of three things:

* Weave.Service.engineManager.getAll().map((e) => e.prefName), which will include all known engines and give you their pref name. This is the preferred choice if you're calling this while Sync is still set up (which you must be, else the prefs would have been cleared).

* Walk the tree of services.sync.engines.* directly.

* Look at the value of services.sync.registerEngines, which will include Mozilla-sourced engines only, by name ("Bookmarks").

Comment 3

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

Attachment: 0002-Bug-1113493-maintain-engine-state-and-offer-engine-c.patch

(In reply to Richard Newman [:rnewman] from comment #2)

> You probably want to do one of three things:

> * Weave.Service.engineManager.getAll().map((e) => e.prefName), which will
> include all known engines and give you their pref name. This is the
> preferred choice if you're calling this while Sync is still set up (which
> you must be, else the prefs would have been cleared).

I've pretty-much gone with this.  A downside is that we might well decide an engine is disabled and thus default this "customizeUI" to true - but when the dialog appears, every engine will be selected (ie, it will *look* like we should have defaulted that checkbox to false).  But that sounds like a trivial issue we can live with.

(Also, I just noticed our "select engine" UI in both the stand-alone customize.xul and via sync prefs, doesn't show the "Forms" engine, which means even limiting our checks to builtin engines would need to treat that as a special-case, and that sounds like a problem waiting to happen - so the check now is simply "are there any disabled engines?")

This version also handles the declinedEngines pref.

Requesting review from both rnewman and adw - rnewman just that the prefs look sane and adw for the migrator module changes.

Comment 4

[Richard Newman [:rnewman]]

(In reply to Mark Hammond [:markh] from comment #3)

> (Also, I just noticed our "select engine" UI in both the stand-alone
> customize.xul and via sync prefs, doesn't show the "Forms" engine, which
> means even limiting our checks to builtin engines would need to treat that
> as a special-case, and that sounds like a problem waiting to happen - so the
> check now is simply "are there any disabled engines?")

For the historical record: "forms" (which is also search history!) shares a checkbox with "history".

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8543815 [details] [diff] [review]
0002-Bug-1113493-maintain-engine-state-and-offer-engine-c.patch

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

::: services/sync/modules/FxaMigrator.jsm
@@ +347,5 @@
>    _unblockSync() {
>      Weave.Service.scheduler.unblockSync();
>    },
>  
> +  /* return a list of [prefName, prefType, prefVal] for all engine related

/**
 * Return a ...
 */

@@ +366,5 @@
> +      let prefName = "services.sync.declinedEngines";
> +      let prefVal = Services.prefs.getCharPref(prefName);
> +      result.push([prefName, Services.prefs.PREF_STRING, prefVal]);
> +    }
> +    catch (ex) {}

} catch

Comment 6

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

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aa7815576ee

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/336ead0f4965

Comment 8

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 37.0b4
Build Id: 20150309191715
OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64

During the testing I've encountered this bug 1142068 .
The Sync prefs differ between devices after the migration is done.