Trackers should disable themselves when the client isn't configured (Status.service == CLIENT_NOT_CONFIGURED, STATUS_DISABLED), enable themselves as soon as setup happens (weave:service:setup-complete notification) and disable themselves again if "Use different account" is used to reset the service (weave:service:start-over notification)

Change trackers to not hook themselves up until they receive the "weave:engine:start-tracking" notification. Tear down on the "weave:engine:stop-tracking" notification. "start-tracking" is emitted by Weave.Service on startup if crypto is available and no setup is needed, as well as after completing the setup wizard. "stop-tracking" is emitted when the user starts over (which puts the client in the same state as if it had never been configured).

If the tracker doesn't track data while not configured, there's a chance of data being reconciled the wrong way. Tracking the data records when the data was changed vs getting all ids and setting the modified time to "now".

(In reply to comment #2) > If the tracker doesn't track data while not configured, there's a chance of > data being reconciled the wrong way. Tracking the data records when the data > was changed vs getting all ids and setting the modified time to "now". Sure, though this could've happened with the add-on already (e.g. if somebody disabled it). I wouldn't call this "the wrong way" therefore, but I will agree that our modified/changed semantics could be better (just because a server record is newer doesn't mean it's actually the newer record).

We have the same problem with "use a different account" then, so we need to fix/resolve this anyway. Tracking changes for all users just in case they might maybe sync someday doesn't make sense, so we need to just fix the initial sync case harder.

Split tests off into separate patch. No code changes.

Tests and test fixes.

(In reply to comment #6) > Tests and test fixes. I should say I also added tests for the form tracker, mostly so I have test coverage before I further touch the form tracker for bug 487558. We could still do with tests for the password, prefs and tabs trackers.

Have tests clean up after themselves even on failures.

Comment on attachment 463146 [details] [diff] [review] v1.1 >+ Observers.add("places-shutdown", this); >+ Observers.add("weave:engine:start-tracking", this); >+ Observers.add("weave:engine:stop-tracking", this); We have util.js here, so we can use Svc.Obs... we can probably stop importing observers.js in this file... >+ _enabled: false, >+ observe: function observe(subject, topic, data) { >+ switch (topic) { >+ case "weave:engine:start-tracking": >+ if (!this._enabled) { >+ Svc.Bookmark.addObserver(this, true); >+ this._enabled = true; >+ } >+ break; >+ case "weave:engine:stop-tracking": >+ if (this._enabled) { >+ Svc.Bookmark.removeObserver(this); >+ this._enabled = false; >+ } >+ break; hmm, why not fall through here? If we're stopping tracking, we should null things out right away, not at shutdown. >+ case "places-shutdown": >+ // Explicitly nullify our references to our cached services so >+ // we don't leak >+ this.__ls = null; >+ this.__bms = null; >+ break; >diff --git a/services/sync/modules/engines/prefs.js b/services/sync/modules/engines/prefs.js >+ case "weave:engine:stop-tracking": >+ if (this._enabled) { >+ this._prefs.removeObserver("", this); >+ this._enabled = false; >+ } >+ break; > case "profile-before-change": > this.__prefs = null; > this.__syncPrefs = null; fall through here as well? r=me with changes

Addressed review comments.

http://hg.mozilla.org/services/fx-sync/rev/02f76eecea87 http://hg.mozilla.org/services/fx-sync/rev/d876d115769c