Probably the whole "sync as soon as possible after startup" logic, so including delayedAutoconnect.

This is kind of rough right now but it passes unit & tps tests. I was thinking of having an observer in SyncScheduler that calls delayedAutoConnect and passes a delay value as a subject. That way the UI stuff in nsBrowserGlue.js doesn't directly touch SyncScheduler and Service doesn't need to either.

Comment on attachment 544349 [details] [diff] [review] move autoconnect functionality to be part of SyncScheduler > let syncTemp = {}; >- Cu.import("resource://services-sync/service.js", syncTemp); >- syncTemp.Weave.Service.delayedAutoConnect(delay); >+ Cu.import("resource://services-sync/policies.js", syncTemp); >+ syncTemp.SyncScheduler.delayedAutoConnect(delay); We should expose SyncScheduler via the Weave object in main.js, so that the UI code can call it from that way. UI code should only ever have to import main.js and access stuff through Weave.* >+ autoConnect: function autoConnect() { >+ let isLocked = Utils.mpLocked(); >+ if (isLocked) { That could be simplified :) >+ // There's no reason to back off if we're locked: we'll just try to login >+ // during sync. Clear our timer, see if we should go ahead and sync, then >+ // just return. >+ this._log.trace("Autoconnect skipped: master password still locked."); >+ >+ if (this._autoTimer) >+ this._autoTimer.clear(); While you're touching this part, can you put add curly braces please? >+ >+ this.checkSyncStatus(); >+ >+ return; >+ } >+ >+ let reason = Weave.Service._checkSync([kSyncNotLoggedIn, kFirstSyncChoiceNotMade]); >+ // Can't autoconnect if we're missing these values. >+ if (!reason) { Hrm. I don't think we need to or even should call Service._checkSync() here at all. Service._lockedSync() will already do it. We should just schedule a sync (provided we have everything set up, but the `if (!Weave.Service.username etc.)` stuff takes care of that. >+ // Once _autoConnect is called we no longer need _autoTimer. >+ if (this._autoTimer) >+ this._autoTimer.clear(); Braces here too, please. > // Applications can specify this preference if they want autoconnect > // to happen after a fixed delay. > let delay = Svc.Prefs.get("autoconnectDelay"); > if (delay) { >- this.delayedAutoConnect(delay); >+ SyncScheduler.delayedAutoConnect(delay); > } That whole paragraph could move to SyncScheduler. Let's put it in a weave:service:ready observer. We should also make sure that we have tests for it. I can think of a bunch of test cases: * calling delayedAutoconnect manually, like Firefox does it * setting the autoconnectDelay pref, like Fennec does it * calling delayedAutoconnect when... - user/password/passphrase bits are missing - Status.service is not STATUS_OK (e.g. STATUS_DISABLED) - ... There are probably more scenarios that I didn't think of right now... MOAR TESTS :D

(In reply to comment #3) > > // Applications can specify this preference if they want autoconnect > > // to happen after a fixed delay. > > let delay = Svc.Prefs.get("autoconnectDelay"); > > if (delay) { > >- this.delayedAutoConnect(delay); > >+ SyncScheduler.delayedAutoConnect(delay); > > } > > That whole paragraph could move to SyncScheduler. Let's put it in a > weave:service:ready observer. I considered doing this but I thought there may be a problem with that. In nsBrowserGlue.js _setSyncAutoconnectDelay() is called on weave:service:ready and if the autoconnectDelay pref has a value that was set, it will simply return (since it expects delayedAutoConnect was already called BEFORE weave:service:ready was notified. If we move this paragraph to weave:service:ready then this code may be executed after _setSyncAutoconnectDelay() and result in no call to delayedAutoConnect O_O So assuming I didn't misunderstand what's happening, it may be better to place this paragraph in _setSyncAutoconnectDelay() instead or add another observer like I mentioned in comment 2.

Hm ok, I think it would still execute delayedAutoConnect(), nevermind about that. This may work.

(In reply to comment #3) We should also make sure that we have tests > for it. I can think of a bunch of test cases: > > * calling delayedAutoconnect manually, like Firefox does it I think this is what we have in test_syncscheduler.js in test_sync_at_startup() no?

(In reply to comment #4) > (In reply to comment #3) > > > // Applications can specify this preference if they want autoconnect > > > // to happen after a fixed delay. > > > let delay = Svc.Prefs.get("autoconnectDelay"); > > > if (delay) { > > >- this.delayedAutoConnect(delay); > > >+ SyncScheduler.delayedAutoConnect(delay); > > > } > > > > That whole paragraph could move to SyncScheduler. Let's put it in a > > weave:service:ready observer. > > I considered doing this but I thought there may be a problem with that. In > nsBrowserGlue.js _setSyncAutoconnectDelay() is called on weave:service:ready > and if the autoconnectDelay pref has a value that was set, it will simply > return (since it expects delayedAutoConnect was already called BEFORE > weave:service:ready was notified. If we move this paragraph to > weave:service:ready then this code may be executed after > _setSyncAutoconnectDelay() and result in no call to delayedAutoConnect O_O Yeah, very good point. In that case, let's move it to SyncScheduler.init(). That gets executed before weave:service:ready. (In reply to comment #6) > > * calling delayedAutoconnect manually, like Firefox does it > > I think this is what we have in test_syncscheduler.js in > test_sync_at_startup() no? True.

(In reply to comment #3) > >+ let reason = Weave.Service._checkSync([kSyncNotLoggedIn, kFirstSyncChoiceNotMade]); > >+ // Can't autoconnect if we're missing these values. > >+ if (!reason) { > > Hrm. I don't think we need to or even should call Service._checkSync() here > at all. Service._lockedSync() will already do it. We should just schedule a > sync (provided we have everything set up, but the `if > (!Weave.Service.username etc.)` stuff takes care of that. I take that back. Bug 671066 has exposed an edge case, so we should leave the checkSync call in there, but without the kFirstSyncChoiceNotMade flag.

* added tests * addressing comment 3

Comment on attachment 545503 [details] [diff] [review] v2: move autoconnect functionality to be part of SyncScheduler Please also address comment 7 and comment 8.

Ah just saw comment 8, will address that. Also, going back to my comment 4, I actually think there is no problem because even if _setSyncAutoconnectDelay() is called first and returns without calling delayedAutoConnect(), delayedAutoConenct() will still be called afterwards using the correct autoconnectDelay pref from the other weave:service:ready observer, so this doesn't actually seem like an issue.

The explanation in comment 11 should be good for addressing comment 7.

(In reply to comment #11) > Ah just saw comment 8, will address that. Also, going back to my comment 4, > I actually think there is no problem because even if > _setSyncAutoconnectDelay() is called first and returns without calling > delayedAutoConnect(), delayedAutoConenct() will still be called afterwards > using the correct autoconnectDelay pref from the other weave:service:ready > observer, so this doesn't actually seem like an issue. Uh yeah. You're right.

Comment on attachment 545503 [details] [diff] [review] v2: move autoconnect functionality to be part of SyncScheduler Please note that bug 671422 changed Service._autoconnect (and the tests) further, so please take that into account as well.

Comment on attachment 547181 [details] [diff] [review] v3: move autoconnect functionality to be part of SyncScheduler r=me

Somehow missed to RESO/FIX this... https://hg.mozilla.org/mozilla-central/rev/e02f888137e5