Story - Make shared pref sync code 2-way

VERIFIED FIXED in Firefox 28

Status

Firefox for Metro
General
P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
Firefox 28
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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*
(Assignee)

Comment 1

4 years ago
p=3

Updated

4 years ago
Blocks: 838081
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

Updated

4 years ago
Whiteboard: feature=story c=tbd u=tbd p=3 → [block28] feature=story c=tbd u=tbd p=3
(Assignee)

Comment 2

4 years ago
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)

Comment 3

4 years ago
(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)
(Assignee)

Comment 4

4 years ago
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)

Updated

4 years ago
Assignee: nobody → netzen
Blocks: 936489
No longer blocks: 838081
Status: NEW → ASSIGNED

Updated

4 years ago
Priority: -- → P1
QA Contact: jbecerra
(Assignee)

Comment 5

4 years ago
Created attachment 831767 [details] [diff] [review]
WIP

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

Comment 6

4 years ago
Created attachment 833340 [details] [diff] [review]
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.
Attachment #831767 - Attachment is obsolete: true
Attachment #833340 - Flags: review?(jmathies)

Comment 7

4 years ago
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+
(Assignee)

Comment 8

4 years ago
Thanks for the weekend review Jim, I'll add those extra comments.
(Assignee)

Comment 9

4 years ago
Created attachment 8333495 [details] [diff] [review]
rev2 - (rev1 with 3 extra comments)

Added the comments, carrying forward r+.
Attachment #8333495 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #833340 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
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
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

4 years ago
Created attachment 8334805 [details] [diff] [review]
rev3

Rebased for Australis and fixed a couple build problems on non Windows platforms.
Attachment #8333495 - Attachment is obsolete: true
Attachment #8334805 - Flags: review+
(Assignee)

Comment 12

4 years ago
Pushed to post-australis Oak here:
https://hg.mozilla.org/projects/oak/rev/8e0d2e61fe49
(Assignee)

Updated

4 years ago
Whiteboard: [block28] feature=story c=tbd u=tbd p=3 → [completed-oak][block28] feature=story c=tbd u=tbd p=3
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

4 years ago
Status: REOPENED → NEW
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/5257899d8b21
Target Milestone: --- → Firefox 28
https://hg.mozilla.org/mozilla-central/rev/5257899d8b21
Status: NEW → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Could you please provide some guidance for QA to test this? Thanks!
Flags: needinfo?(netzen)
(Assignee)

Comment 16

4 years ago
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.