Closed Bug 1124956 Opened 5 years ago Closed 5 years ago

Fix Sync engine selection after migration to FxA

Categories

(Firefox :: Sync, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 + fixed

People

(Reporter: adw, Assigned: markh)

References

Details

Attachments

(1 file)

This bug is the result of bug 1119578.  We should just rename the services.sync.ui.showCustomizationDialog pref so that Svc.Prefs.resetBranch doesn't clear it.  See bug 1119578 for context.  Renaming the pref does cause the customize.xul dialog to open after migration as expected, but there's still some problem because all its checkboxes are checked regardless of which engines you selected before migration.  So that needs to be investigated as part of this bug.  Bug 1113493 should have fixed that.
Flags: qe-verify-
Flags: firefox-backlog+
There are 3 parts to this patch:

* Rename the pref as we discussed.

* Introduces a new observer notification weave:service:start-over:init-identity which is sent by .startOver() after everything has been reset but before the new identity has been initialized.  The root of the problem was that this modal dialog (and hence reading prefs etc) happened *before* our weave:service:start-over:finish observer is called - so we were correctly resetting the prefs, but *after* they had been acted on (doh!).

* Adds the sync migration module's log to the sync log initialization code - this is technically unrelated to this patch, but it addresses something I forgot to do previously.

With this patch applied things work perfectly.
Assignee: nobody → mhammond
Attachment #8555041 - Flags: review?(adw)
[Tracking Requested - why for this release]:

We are hoping to enable Sync migration on 37, but it isn't going to work correctly without this patch.
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Comment on attachment 8555041 [details] [diff] [review]
0004-Bug-1124956-Fix-Sync-engine-selection-after-migratio.patch

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

Cool, thanks.

::: services/sync/modules/policies.js
@@ +591,5 @@
>      let fapp = this._logAppender = new Log.StorageStreamAppender(formatter);
>      fapp.level = Log.Level[Svc.Prefs.get("log.appender.file.level")];
>      root.addAppender(fapp);
>  
> +    // Arrange for the a number of other sync-related logs to also go to our

Typo nit: the a -> a
Attachment #8555041 - Flags: review?(adw) → review+
https://hg.mozilla.org/mozilla-central/rev/7f684fa2c9d9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Comment on attachment 8555041 [details] [diff] [review]
0004-Bug-1124956-Fix-Sync-engine-selection-after-migratio.patch

Approval Request Comment
[Feature/regressing bug #]: Sync migration.
[User impact if declined]: Users migrating sync may lose their sync options.
[Describe test coverage new/current, TreeHerder]: Tests pass
[Risks and why]: Low, limited to sync migration
[String/UUID change made/needed]: None
Attachment #8555041 - Flags: approval-mozilla-aurora?
Attachment #8555041 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs a bit of rebasing around bug 1121325 (or an uplift on that too).
Flags: needinfo?(mhammond)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #7)
> Needs a bit of rebasing around bug 1121325 (or an uplift on that too).

Yep, I've an uplift request on that bug - hopefully it will be approved soon.
Flags: needinfo?(mhammond)
You need to log in before you can comment on or make changes to this bug.