Closed Bug 1113493 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8539006 - Flags: feedback?(rnewman)
Attachment #8539006 - Flags: feedback?(adw)
Flags: qe-verify+
Flags: firefox-backlog+
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?
Attachment #8539006 - Flags: feedback?(adw) → feedback+
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").
Attachment #8539006 - Flags: feedback?(rnewman) → feedback+
(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.
Assignee: nobody → mhammond
Attachment #8539006 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8543815 - Flags: review?(rnewman)
Attachment #8543815 - Flags: review?(adw)
Iteration: --- → 37.3
(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 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
Attachment #8543815 - Flags: review?(rnewman) → review+
Attachment #8543815 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/336ead0f4965
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
QA Contact: twalker
See Also: → 1119578
Depends on: 1124956
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.
Depends on: 1142068
You need to log in before you can comment on or make changes to this bug.