691490 - Eliminate direct calls to sync() in UI code

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

browser/base/content/aboutSyncTabs.js
236:      Weave.Clients.sync();
241:    engine.sync();

browser/base/content/syncQuota.js
111:      Weave.Utils.nextTick(function() { Service.sync(); });

mobile/chrome/content/sync.js
311:    setTimeout(function () { Weave.Service.sync(); }, 0);
344:    Weave.Service.sync();

Comment 1

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #0)
> browser/base/content/aboutSyncTabs.js
> 236:      Weave.Clients.sync();
> 241:    engine.sync();

I'm very happy to make this go away.

> browser/base/content/syncQuota.js
> 111:      Weave.Utils.nextTick(function() { Service.sync(); });

This one's perfectly justified. Why do you want to remove it?

> mobile/chrome/content/sync.js
> 311:    setTimeout(function () { Weave.Service.sync(); }, 0);
> 344:    Weave.Service.sync();

These two seem justified.

Comment 2

[Richard Newman [:rnewman]]

(In reply to Philipp von Weitershausen [:philikon] from comment #1)

> These two seem justified.

I disagree.

If a caller isn't interested in having a sync occur *right that second*, and isn't interested in some return value of the sync, then it should be asking the SyncScheduler to do it.

Put differently: these callers just want a sync to happen soon, and we should offer a way to do that.

Having random code scattered throughout the browser that directly invoke syncs will confound our attempts to improve scheduling logic, and reinforces the perception that a sync invocation can be a synchronous process.

Comment 3

[Philipp von Weitershausen [:philikon]]

I would say that all those, with the exception of aboutSyncTabs.js, want a sync to happen *right now*. We could add an abstraction for that on the SyncScheduler, sure. I'm not sure I see the point for that, other than having only one entry point to triggering syncs (which isn't true because the UI also triggers syncs via ErrorHandler.syncAndReportFailures().)

So IMHO calling Weave.Service.sync() isn't a bad thing at all when you want a sync to happen now now (which is a legitimate use case).

Comment 4

[Chris Karlof [:ckarlof]]

I think these are gone.