[Sync] Remove unnecessary posting of methods in sync scheduler.

[Sync] Remove unnecessary posting of methods in sync scheduler.

Configuration, Clear User Data, Cleanup Disabled Types, and StartMode are all
now synchronous methods. Nudge and Stop are the only methods that do posting.

BUG=129665, 103327
TEST=unit_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141541

[Nicolas Zea, 2012-06-07 00:45:42]

PTAL. This mostly deals with a TODO Fred added, and paves the way for the
refactor I'm performing.

[tim (not reviewing), 2012-06-11 20:06:13]

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler.cc
File sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.cc:617: // TODO(tim): config-specific
GetUpdatesCallerInfo value?
I think this can be removed.

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:90: void Configure(
I'm not sure about this rename -- it is still up to the scheduler to choose to
not actually perform the job (see WaitInterval::pending_configure_job). Granted,
we should have a more graceful way of handling this (also see bug 46078), but
from an API point of view the truth is the scheduler makes no guarantee that
anything _will happen immediately_.

(side note - I don't like 'Configure' just 'cause it sounds like we're
configuring the scheduler or something. And we have way too many 'Configure'
methods to begin with.)

I think there may have been some confusion with the reconfig prod issue from
last year, but in that case we weren't actually in backoff because
MIGRATION_DONE isn't an error worthy of backoff.  The loop (IIRC) was
(MIGRATION_DONE > CONFIGURATION)*

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:96: void CleanupDisabledTypes();
nit - You can add a TODO here for me and note bug 131336

[tim (not reviewing), 2012-06-11 20:06:57]

On 2012/06/11 20:06:13, timsteele wrote:
>
http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler.cc
> File sync/engine/sync_scheduler.cc (right):
> 
>
http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
> sync/engine/sync_scheduler.cc:617: // TODO(tim): config-specific
> GetUpdatesCallerInfo value?
> I think this can be removed.
> 
> http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler.h
> File sync/engine/sync_scheduler.h (right):
> 
>
http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
> sync/engine/sync_scheduler.h:90: void Configure(
> I'm not sure about this rename -- it is still up to the scheduler to choose to
> not actually perform the job (see WaitInterval::pending_configure_job).
Granted,
> we should have a more graceful way of handling this (also see bug 46078), but
> from an API point of view the truth is the scheduler makes no guarantee that
> anything _will happen immediately_.
> 
> (side note - I don't like 'Configure' just 'cause it sounds like we're
> configuring the scheduler or something. And we have way too many 'Configure'
> methods to begin with.)
> 
> I think there may have been some confusion with the reconfig prod issue from
> last year, but in that case we weren't actually in backoff because
> MIGRATION_DONE isn't an error worthy of backoff.  The loop (IIRC) was
> (MIGRATION_DONE > CONFIGURATION)*
> 
>
http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
> sync/engine/sync_scheduler.h:96: void CleanupDisabledTypes();
> nit - You can add a TODO here for me and note bug 131336

Also I think you can add 103327 to the BUG=

[Nicolas Zea, 2012-06-11 20:55:09]

Comments addressed, PTAL

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler.cc
File sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.cc:617: // TODO(tim): config-specific
GetUpdatesCallerInfo value?
On 2012/06/11 20:06:13, timsteele wrote:
> I think this can be removed.

Done.

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:90: void Configure(
On 2012/06/11 20:06:13, timsteele wrote:
> I'm not sure about this rename -- it is still up to the scheduler to choose to
> not actually perform the job (see WaitInterval::pending_configure_job).
Granted,
> we should have a more graceful way of handling this (also see bug 46078), but
> from an API point of view the truth is the scheduler makes no guarantee that
> anything _will happen immediately_.
> 
> (side note - I don't like 'Configure' just 'cause it sounds like we're
> configuring the scheduler or something. And we have way too many 'Configure'
> methods to begin with.)
> 
> I think there may have been some confusion with the reconfig prod issue from
> last year, but in that case we weren't actually in backoff because
> MIGRATION_DONE isn't an error worthy of backoff.  The loop (IIRC) was
> (MIGRATION_DONE > CONFIGURATION)*

Done.

http://codereview.chromium.org/10542044/diff/7001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:96: void CleanupDisabledTypes();
On 2012/06/11 20:06:13, timsteele wrote:
> nit - You can add a TODO here for me and note bug 131336

Done.

[tim (not reviewing), 2012-06-11 21:20:37]

LGTM with nit

http://codereview.chromium.org/10542044/diff/4008/sync/engine/sync_scheduler.h
File sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/10542044/diff/4008/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:93: void ScheduleConfigure(
uber nit - Lets call this 'ScheduleConfiguration' to be consistent with the job
name and Mode (and less like all the 'Configure' stuff at the glue/ layer).

[commit-bot: I haz the power, 2012-06-11 21:30:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10542044/1009

[commit-bot: I haz the power, 2012-06-11 22:51:27]

Change committed as 141541