583350 - Move autoconnectDelay checking out of the service

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

We should make this the responsibility of the app.

Comment 1

[Mike Connor [:mconnor]]

The entire point of checking the pref is so that apps have an easy way of setting a fixed delay without having to call anything.  Apps don't have to set this, and can do their own thing without defining the pref, so I don't actually understand why this is a problem?  (I also don't understand why it blocks landing the UI, since I assume the Firefox implementation does this itself, as is already possible...)

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

It came up in review comments.

(In reply to comment #1)
> The entire point of checking the pref is so that apps have an easy way of
> setting a fixed delay without having to call anything.

So that's sort of the problem. If it's for apps to define, why make it a pref? When prefs are user-editable, it can lead to bad things. This one isn't that bad, it'll just negatively effect startup time & would be nearly impossible to track down whereas if we just said "Weave will always be inited X seconds after startup is complete", we would know that.

A worse example is services.sync.registerEngines. The sync UI (at least in the addon, I made some changes in Firefox) depended on Tabs being in that string. If it wasn't, the addon would break hard. But that's a user-editable thing. Sure, editing about:config "voids your warranty" and it's not expected that people will edit that. BUT when it actually breaks the application, that's totally not OK.

> Apps don't have to set
> this, and can do their own thing without defining the pref, so I don't actually
> understand why this is a problem?  (I also don't understand why it blocks
> landing the UI, since I assume the Firefox implementation does this itself, as
> is already possible...)

Well no, its only sort of possible. If the value is set, the Sync service will autoconnect. So in Firefox land, we'd have to clear that pref before we even load the sync module if we want our expected behavior.

Long story short, I don't think that prefs should give the user the ability to screw up the application. In this case, Sync should never autoconnect unless the app using sync wants it to. It's now out of the app's control.

Comment 3

[Philipp von Weitershausen [:philikon]]

To avoid Ts regressions, Sync doesn't start up until 10 secs after start up anyway. And then, thanks to autoconnectDelay, it stays disconnected for a brief moment before the autoconnect kicks in.

I think we should have just one delay. Basically just connect upon weave:service:ready, if we're set up. Essentially this gets us back the old behaviour before autoconnectDelay was introduced, obsoleting this pref altogether.

Comment 4

[Mike Connor [:mconnor]]

We're not going to have autoconnectDelay at all once we kill login/logout as separate steps, but this should live in the service.