560580 - pref sync cleanup

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

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);
      } 
    }   
  }

Comment 2

[Philipp von Weitershausen [:philikon]]

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.

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: 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.

Comment 4

[Philipp von Weitershausen [:philikon]]

Requesting blocking2.0 for beta8 as we want to land this at the same time as simplified crypto (bug 603489)

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: v1.1

Ensure we test PrefEngine.getChangedIDs(). No code change, just moar tests.

Comment 6

[Mike Connor [:mconnor]]

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...

Comment 7

[Philipp von Weitershausen [:philikon]]

(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...

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: 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

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: v1.3 for checkin

Fix + test for a small bug that prevented services.sync.prefs.sync.* preferences from being applied.

Comment 11

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/services/fx-sync/rev/c5812b932589

Comment 13

[Dietrich Ayala (:dietrich)]

Belated blocking+.