We have code that is not needed anymore for automatically setting up Sync in Metro Firefox, when you set it up for Desktop Firefox. That code isn't needed anymore: With the exception of browser/themes/windows/syncSetup.css (there is code to fix alignment there) We should remove all of this code: https://hg.mozilla.org/mozilla-central/rev/b855b1ac3f14 https://hg.mozilla.org/mozilla-central/rev/19eed0fb290e https://hg.mozilla.org/mozilla-central/rev/c75172789a3c p=1

Looks like the load sync setup call was already removed by someone, I think when we got rid of the custom sync code on metro, so this feature would have been broken anyway :S Tested setting up sync and changing environments and it works for me, sync is still setup.

I think I'll have another patch with the extra prefs to share between the 2 browsers, I need to get that on oak to test it first there though.

Pushed the r+ed changeset to oak: https://hg.mozilla.org/projects/oak/rev/ba7008808453 Waiting to resolve this because I think an additional patch will be needed.

I'm a bit concerned with not syncing *all* prefs related to sync, but there are a lot of them. And this seems to work. Richard is there any possibility of a of damage if the 2 browsers are sharing the same sync db but have different sync prefs? The 2 browsers now share the same profile for everything except for prefs.

I think we might need client info as well. 'name' at least, not sure about syncID and GUID. I'll have to default to rnewman, code looks ok though.

Sorry for the delay in responding. > Richard is there any possibility of a of damage if the 2 browsers are > sharing the same sync db but have different sync prefs? Yes. We can divide the prefs as follows: * Configuration items. Logging levels, for example. * Run-time data storage: timestamps etc. * These are both global and per-engine, and this set is extensible (e.g., services.sync.adblockplus.*) * Pref overrides: indicators for which prefs to sync. (services.sync.prefs.*) * This is intrinsically extensible. Failing to replicate configuration data will typically be harmless, because most users don't modify these. If they do, then consequences will be unpredictable. The pref overrides are what controls which prefs are synchronized through Sync. This should be the same for all devices, let alone different 'views' onto the same profile, else you get divergence. Run-time data storage covers the majority of user-set prefs under services.sync.*. All of these are critical: a misplaced timestamp, an un-set syncID, or a differing account name would cause repeated work, skipped work, or conflicts. Typically Sync will recover from these, but you're essentially exercising a fatal-error code path -- Sync tries hard not to trash your bookmarks if Firefox fails to write a pref, but I wouldn't want that to be a daily thing. In short: you should ensure that every pref in services.sync.* is the same from either side of the 'wall'.

Comment on attachment 8333602 [details] [diff] [review] Sync pref sync. rev1 Review of attachment 8333602 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/WindowsPrefSync.jsm @@ +70,5 @@ > "app.update.service.enabled", > "app.update.metro.enabled", > + "browser.sessionstore.resume_session_once", > + "services.sync.account", > + "services.sync.username" This really needs to handle a whole pref tree, dynamically.

Looks like I'll need to add some kind of wildcard functionality to WindowsPrefSync to match pref branches to the registry (perhaps if they are user set). Can you think of a better way than that jimm?

(In reply to Brian R. Bondy [:bbondy] from comment #9) > Looks like I'll need to add some kind of wildcard functionality to > WindowsPrefSync to match pref branches to the registry (perhaps if they are > user set). > Can you think of a better way than that jimm? Longer term I think what we need to look at is adding a mirroring feature to the pref service so it can take care of this for us. Both prefs files are available in the same profile. To avoid these issues here, I'll pitch the idea that we disable the Sync panel entirely in metrofx and revisit once Firefox Accounts (formerly known as picl) is up and running and well tested and well understood.

I don't think disabling the panel will help too much, we'd still need to sync the data no? If the user uses both browsers then that's not a problem, but if the user uses mostly the metro browser the data will never be synced up.

(In reply to Brian R. Bondy [:bbondy] from comment #11) > I don't think disabling the panel will help too much, we'd still need to > sync the data no? > If the user uses both browsers then that's not a problem, but if the user > uses mostly the metro browser the data will never be synced up. I'm suggesting we disable sync entirely in metrofx. There are still a number of outstanding issues with it (particularly that modal thread loop that has issues on 8.1). Feels to me like sync support is becoming too much of a pita to support for v1. Am I being too paranoid here? Setting observers on ~50 sync prefs both in desktop and metro and flushing changes to the Windows registry every time something changes feels risky to me. Looking at block28 bugs we still have quite a bit of profile sharing work to do, and we'd like to get that landed by the beginning of next week so it has two weeks of bake time.

Yuan does this sound acceptable to you? Mark can you weigh in too? I personally like Jim's suggestion, but I wanted to get some extra feedback.

If your only approach to mirroring involves flushing to the registry a few dozen times during a sync, then I agree that that is fragile, and I support turning Sync off in Metro. That imposes obvious constraints on users, as well as some non-obvious ones (no sending or receiving of tabs in Metro mode), but if users already need to use Classic to install software updates, maybe that's OK. For the record, FxA and existing Sync is months out. New new Sync isn't even scheduled.

what it would involve is flushing out a few dozen registry entries on startup, pulling them in on startup as well. And flushing them out as they changed via an observer. (Only one browser can be open at a time). Affecting startup perf with that many prefs to move to registry and pull from registry scares me.

Those observers will fire for several prefs during a sync. One timestamp per collection, plus overall last sync, next sync, maybe some others. You'd need to batch for sanity.

Adding mbrubeck to weigh in

(In reply to Jim Mathies [:jimm] from comment #12) > (In reply to Brian R. Bondy [:bbondy] from comment #11) > > I don't think disabling the panel will help too much, we'd still need to > > sync the data no? > > If the user uses both browsers then that's not a problem, but if the user > > uses mostly the metro browser the data will never be synced up. > > I'm suggesting we disable sync entirely in metrofx. There are still a number > of outstanding issues with it (particularly that modal thread loop that has > issues on 8.1). Feels to me like sync support is becoming too much of a pita > to support for v1. > > Am I being too paranoid here? Setting observers on ~50 sync prefs both in > desktop and metro and flushing changes to the Windows registry every time > something changes feels risky to me. > > Looking at block28 bugs we still have quite a bit of profile sharing work to > do, and we'd like to get that landed by the beginning of next week so it has > two weeks of bake time. Given the fact that having shared profile work is to resolve sync in between classic and metro, I think not supporting it for v1 metro is fine with me. Also, the next generation Sync is likely to replace the existing UI we have eventually.

If we decide to turn off Sync for v1 and wait for FxAccounts + Sync to change the rules we'll be a bit disappointed. FxAccounts will change the UI (as Yuan points out) but the Sync system is the same as the one we are working with now. And the NextGen Sync is not really on a roadmap yet. We can always turn off Sync for Metro v1 as a way to give us time to work on a better fix. I'd suggest treating this as a P2 so we can spend some brain-power on it, but push it off if needed.

Does WindowsPrefSync.jsm handle multiple profiles?

It'll sync the said prefs from registry down if you have multiple profiles. There's a lower priority bug somewhere out there to have a better solution for that though.

Actually it's much easier to fix the multiple profile issue now since we have the same profile name for the browsers we want to sync. I'll get that filed/fixed for v1.

[12:15:52] <mbrubeck> bbondy: I've been thinking that over. I'm torn... I know that a minority of users use sync at all, but for me personally (and I would guess for a lot of the others who do use it), it is *the* main reason to use Firefox on my tablet or phone. [12:16:12] <mbrubeck> Having to use desktop Firefox on my tablet to have working sync would be a real disadvantage. [12:16:33] <mbrubeck> But on the other hand, I don't have a good solution to offer to the immediate technical problems [12:16:39] <mbrubeck> so I'm not sure there's a better alternative. :/ [12:17:12] <bbondy> I was thinking along the lines of filtering certain prefs into a new pref file that is shared by both environments. [12:17:47] <bbondy> but it gets a little fuzzy for people who already have those prefs in the main pref file [12:18:06] <bbondy> 1 time import into the ne pref file I guess [12:18:14] <mbrubeck> yeah [12:18:19] <bbondy> jimm^ thoughts? [12:18:35] <bbondy> ne -> new [12:19:59] <mbrubeck> bbondy: Even if we can't get that landed in time for Fx28, I'd be happier if we could at least plan on doing it (and re-enabling sync) around Fx29... I don't think that lack of sync would kill our release, but I'd like it to be temporary rather than something that languishes until we reach some PiCL promised land. [12:21:15] <bbondy> mbrubeck, so maybe disable in this bug, put the backout of that disabling immediately as a patch in a new bug, and have that bug also try to do the trategy mentioned above. And mark that bug as release28? [12:21:30] <bbondy> and if we decide it can't make release28 then retriage it later for release29 [12:21:48] <mbrubeck> bbondy: yeah, I'd be on board with that. [12:21:53] * asadotzler (asadotzler@moz-7B0110AD.mv.mozilla.com) joins #windev [12:22:03] <bbondy> trategy -> strategy (sorry my sony vaio's keys only intermittently work) [12:22:28] <bbondy> k I'll go that route, thanks.

To reiterate, we can't have any dependency on 'new sync', and even locking down a timeline for FxA is hard to do with any degree of confidence. And I'm with mbrubeck - sync is still an important part of the story, although with the idea of shared profile support, I would want some 'magic' to happen so that the sync set up on my classic desktop obviously carries over to the devices I have synced with it (including the info from metro usage). I don't think it's imperative to have this landed in 28, but I wouldn't want it delayed to a 'bundled v2' release either, and explicitly calling out no dependencies to FxA or new sync :). Of course we need to be cognizant on the implementation to make it easier when those components are ready, but we can't be waiting on them either. (I'll be working on trying to nail down that FxA timeframe :/ )

Great, thanks Karen.

I think this should be enough to disable sync temporarily until we get the pref handling in the pref service figured out closer to v28/v29. Everyone will be starting with a new pref file when shared profiles land, so no one will have things already setup in metro for sync. Disabling the access to the flyout I think is enough.

There are some keyboard and command shortcuts too, the markup in browser.xul looks like it needs to be commented out - http://mxr.mozilla.org/mozilla-central/search?find=%2Fbrowser%2Fmetro%2F&string=MOZ_SERVICES_SYNC Also, we might want to stop loading/initializing the module here - http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/flyoutpanels/FlyoutPanelsUI.js#25 to avoid - http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/flyoutpanels/SyncFlyoutPanel.js#10

Landed on oak here: https://hg.mozilla.org/projects/oak/rev/0aaea71c1c6f https://hg.mozilla.org/projects/oak/rev/e81ee13beea0 Bug 935099 will track the landing on m-c. When it lands on m-c a new comment will be added here as well with the m-c changeset. This is being done so we can still use scrumbugs efficiently. See bug 935099 for further details.

https://hg.mozilla.org/integration/fx-team/rev/325f2f06ca0b https://hg.mozilla.org/integration/fx-team/rev/f1bab224b379

https://hg.mozilla.org/mozilla-central/rev/325f2f06ca0b https://hg.mozilla.org/mozilla-central/rev/f1bab224b379

Tested for iteration #19, on Win 8 64-bit, with latest Nightly (build ID: 20131208030204): Sync charm isn't available anymore. Is there anything else QA should cover here?

What have you tested?

(In reply to Brian R. Bondy [:bbondy] from comment #35) > What have you tested? That we no longer see the Sync charm, as we can see the other flyouts: About, Options, Permissions, etc.

That should be sufficient for this bug. Thanks.

Marking this verified based on comments: 34, 36 and 37.