Status

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mconnor, Assigned: philikon)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta8+)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
from bug 548385:

* don't sync pref type (Preferences.js solves this for us)
* make storage just prefs[prefname] = value and have a simpler data format
  (this would be a storage version bump, but that's not especially terrible for
prefs)
* need to solve whether we sync prefs if the local client doesn't have that pref enabled for sync (if we do, we should enable that pref for sync in future, so it's bidirectional)
* Using Preferences.js would make this code much more compact:

  _prefs:     new Preferences();

  _getAllPrefs: function () {
    let values = {};
    let toSync = this._syncPrefs;

    for (let i = 0; i < toSync.length; i++) {
      if (!this._prefs.get(WEAVE_SYNC_PREFS + toSync[i]), false)
        continue;

      let val = this._prefs.get(toSync[i], "");
      if (val)
        values[toSync[i]] = val;
    }

    return values;
  },

  _setAllPrefs: function(values) {
    for (let pref in values) {
      if (!this._prefs.get(WEAVE_SYNC_PREFS + toSync[i]), false)
        continue;
      try {
        this._prefs.set(pref, values[pref]);
      catch(e) {
        this._log.trace("Failed to set pref: " + pref + " Reason: " + e);
      } 
    }   
  }
(Assignee)

Updated

8 years ago
Duplicate of this bug: 558890
Since we're bumping the global storageVersion to 4 in Sync 1.6, we have an opportunity to redo the prefs engine and bump its storageVersion to 2:

* do not sync the pref type (bug 558890)
* handle some error scenarios more gracefully (bug 579347)
* use AppInfo.ID as the GUID so that each Mozilla app uses its own namespace (bug 598968)
* never ever do disk I/O (bug 609395)

The latter ones are the motivator here since we want them for Firefox + Fennec 4.
Assignee: nobody → philipp
Created attachment 491353 [details] [diff] [review]
v1

In this patch:

* We no longer sync the pref type (bug 558890), all hail Preference.js!

* Use AppInfo.ID as the GUID so that each Mozilla app uses its own namespace
(bug 598968). This and the previous change are an incompatible change to the prefs storage format, so engine.version was upped from 1 to 2.

* We never ever do disk I/O (bug 609395) akin to the tabs engine. Unlike the tabs engine, we store the dirty state (tracker.modified) in a preference so it's persisted across sessions.

* We now also sync the prefs that determine which prefs are to be synced (yo dawg!). That means when you switch off persona sync on one machine it'll propagate to all machines. Predictability FTW!

* Also, the way we determine whether a pref should be included in the record is now much saner.

* The tracker score increment now actually is what the comment said (25 points). Changing the prefs that determine which prefs are to be synced ups the score by 100.
Attachment #491353 - Flags: review?(mconnor)
Requesting blocking2.0 for beta8 as we want to land this at the same time as simplified crypto (bug 603489)
blocking2.0: --- → ?
Created attachment 491370 [details] [diff] [review]
v1.1

Ensure we test PrefEngine.getChangedIDs(). No code change, just moar tests.
Attachment #491353 - Attachment is obsolete: true
Attachment #491370 - Flags: review?(mconnor)
Attachment #491353 - Flags: review?(mconnor)
(Reporter)

Comment 6

8 years ago
Comment on attachment 491370 [details] [diff] [review]
v1.1

>diff --git a/services/sync/modules/engines/prefs.js b/services/sync/modules/engines/prefs.js

>+        if (aData.slice(0, WEAVE_SYNC_PREFS.length) == WEAVE_SYNC_PREFS)

You do this in a few places.  Do this instead (faster/easier to read)

if (aData.indexOf(WEAVE_SYNC_PREFS) == 0)

Otherwise, way to slip this in under the wire...
Attachment #491370 - Flags: review?(mconnor) → review+
(In reply to comment #6)
> >+        if (aData.slice(0, WEAVE_SYNC_PREFS.length) == WEAVE_SYNC_PREFS)
> 
> You do this in a few places.  Do this instead (faster/easier to read)
> 
> if (aData.indexOf(WEAVE_SYNC_PREFS) == 0)

Ah right, forgot that indexOf() can also take a substring. If only strings had startswith()/endswith() methods...
Created attachment 492243 [details] [diff] [review]
v1.2 for checkin

Addressed mconnor's review comment. This is ready for landing in fx-sync, but should be coordinated with landing of bug 603489
Attachment #491370 - Attachment is obsolete: true
Created attachment 492254 [details] [diff] [review]
v1.3 for checkin

Fix + test for a small bug that prevented services.sync.prefs.sync.* preferences from being applied.
Attachment #492243 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Duplicate of this bug: 585755
http://hg.mozilla.org/services/fx-sync/rev/c5812b932589
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Blocks: 615021
(Assignee)

Updated

8 years ago
Duplicate of this bug: 598968
(Assignee)

Updated

8 years ago
Blocks: 609395
(Assignee)

Updated

8 years ago
Depends on: 615604
(Assignee)

Updated

8 years ago
Depends on: 616179

Updated

8 years ago
Whiteboard: [qa-]
Belated blocking+.
blocking2.0: ? → beta8+
(Assignee)

Updated

8 years ago
Blocks: 579347
You need to log in before you can comment on or make changes to this bug.