931801 - Story - Make shared pref sync code 2-way

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P1)

Description

[Brian R. Bondy [:bbondy]]

Currently we have a 1-way sync of prefs from Desktop to Metro.

Pres are pushed from Desktop to registry, on startup here:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#l1202
And are also pushed when the prefs change here:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#l4766

Then on the Metro browser startup, prefs are then pulled into metro here:
http://dxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#l614

By the way, to add new prefs to be synced, all you need to do is add another item to this array:
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#l4758

This bug is to make that sync 2-way. 
Basically we need to add mirrored pull functions to Desktop Firefox, and add mirrored push functions to Metro Firefox.

We should also rename the existing code from *DesktopControlledPref* to *SharedPref*

Comment 1

[Brian R. Bondy [:bbondy]]

p=3

Comment 2

[Brian R. Bondy [:bbondy]]

I think we can actually skip doing this task because I think we can just train people to expect the preferences to be different in each environment. Do you agree Jim?

Comment 3

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> I think we can actually skip doing this task because I think we can just
> train people to expect the preferences to be different in each environment.
> Do you agree Jim?

I thought we wanted a way to sync a subset of prefs between the two based on which ever browser was last running. Wouldn't Sync prefs fall into this category?

Comment 4

[Brian R. Bondy [:bbondy]]

Yep I was about to cancel this request anyway because of bug 931798.

Yep sync was working for me but I didn't have the pref name landed on oak yet (only m-c).

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: WIP

Coded, not yet run or tested, probably has bugs, but basic outline of task is done.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: rev1

This uses a toolkit module to share all code between Metro and Desktop (to avoid code duplication).

It has the ability to 2-way sync prefs, but also has the ability to push from Desktop only for some important prefs like app-update, and push only from Metro (not used yet).

Basically there's a metro controlled list of prefs, and a desktop controlled list of prefs. And if you want it to work 2-way you specify the pref in both locations.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 833340 [details] [diff] [review]
rev1

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

::: browser/base/content/browser.js
@@ +1186,4 @@
>        Cu.reportError("Could not end startup crash tracking: " + ex);
>      }
>  
> +    if (WindowsPrefSync) {

Lets add a comment here explaining what this does.

::: browser/metro/base/content/browser-ui.js
@@ +154,4 @@
>          Util.dumpLn("Exception in delay load module:", ex.message);
>        }
>  
> +      if (WindowsPrefSync) {

ditto.

::: toolkit/modules/WindowsPrefSync.jsm
@@ +56,5 @@
> +  /**
> +   * The following preferences will be pushed to registry from Desktop
> +   * Firefox and pulled in from Metro Firefox.
> +   */
> +  desktopControlledPrefs: ["app.update.auto",

Lets comment each pref category explain why we export/import them.

Comment 8

[Brian R. Bondy [:bbondy]]

Thanks for the weekend review Jim, I'll add those extra comments.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: rev2 - (rev1 with 3 extra comments)

Added the comments, carrying forward r+.

Comment 10

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/oak/rev/25770a08da39

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.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: rev3

Rebased for Australis and fixed a couple build problems on non Windows platforms.

Comment 12

[Brian R. Bondy [:bbondy]]

Pushed to post-australis Oak here:
https://hg.mozilla.org/projects/oak/rev/8e0d2e61fe49

Comment 13

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/fx-team/rev/5257899d8b21

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5257899d8b21

Comment 15

[Manuela Muntean [Away]]

Could you please provide some guidance for QA to test this? Thanks!

Comment 16

[Brian R. Bondy [:bbondy]]

Here are 4 test cases:
app.update.enabled should sync from Desktop to Metro
app.update.enabled should not sync from Metro to Desktop
Make a new pref by any name and it should not sync between metro and desktop
Make a new pref by any name and it should not sync between desktop and metro

Comment 17

[Manuela Muntean [Away]]

Verified as fixed for iteration #20, on Win 8.1 Pro 64-bit, with latest Nightyl 29.0a1 (build ID: 20131216030202). All of the below test cases work as expected.

> Here are 4 test cases:
> app.update.enabled should sync from Desktop to Metro
> app.update.enabled should not sync from Metro to Desktop
> Make a new pref by any name and it should not sync between metro and desktop

When creating a new pref in Metro, it's not visible on Desktop

> Make a new pref by any name and it should not sync between desktop and metro

When creating a new pref in Desktop, it's not visible on Metro