Closed Bug 931801 Opened 6 years ago Closed 6 years ago

Story - Make shared pref sync code 2-way

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 28

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: [completed-oak][block28] feature=story c=tbd u=tbd p=3)

Attachments

(1 file, 3 obsolete files)

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*
p=3
Summary: Make shared pref sync code 2-way → Story - Make shared pref sync code 2-way
Whiteboard: feature=story c=tbd u=tbd p=3
Whiteboard: feature=story c=tbd u=tbd p=3 → [block28] feature=story c=tbd u=tbd p=3
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?
Flags: needinfo?(jmathies)
(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?
Flags: needinfo?(jmathies)
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).
Assignee: nobody → netzen
Blocks: metrov1it19
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P1
QA Contact: jbecerra
Attached patch WIP (obsolete) — Splinter Review
Coded, not yet run or tested, probably has bugs, but basic outline of task is done.
Attached patch rev1 (obsolete) — Splinter Review
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.
Attachment #831767 - Attachment is obsolete: true
Attachment #833340 - Flags: review?(jmathies)
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.
Attachment #833340 - Flags: review?(jmathies) → review+
Thanks for the weekend review Jim, I'll add those extra comments.
Added the comments, carrying forward r+.
Attachment #8333495 - Flags: review+
Attachment #833340 - Attachment is obsolete: true
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.
Blocks: 935099
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attached patch rev3Splinter Review
Rebased for Australis and fixed a couple build problems on non Windows platforms.
Attachment #8333495 - Attachment is obsolete: true
Attachment #8334805 - Flags: review+
Pushed to post-australis Oak here:
https://hg.mozilla.org/projects/oak/rev/8e0d2e61fe49
Whiteboard: [block28] feature=story c=tbd u=tbd p=3 → [completed-oak][block28] feature=story c=tbd u=tbd p=3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
https://hg.mozilla.org/integration/fx-team/rev/5257899d8b21
Target Milestone: --- → Firefox 28
https://hg.mozilla.org/mozilla-central/rev/5257899d8b21
Status: NEW → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Could you please provide some guidance for QA to test this? Thanks!
Flags: needinfo?(netzen)
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
Flags: needinfo?(netzen)
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.