[Sync] Add support for server controlled nudge delays

[Sync] Add support for server controlled nudge delays

Server nudge delays override the local ones, but must be longer than the
default nudge delay. If set to 0, will reset the nudge to the default.

To accomplish this the delay logic was refactored and moved into the sync
scheduler, which is now aware of all delays for all types and sync cycles.

BUG=401600
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291512

[Nicolas Zea, 2014-08-19 23:38:15]

PTAL

[rlarocque, 2014-08-20 00:13:32]

Some comments.

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_impl.cc:865: // Take the shorted delay from all
requested types.
shortest?

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_impl.cc:921: const std::map<ModelType, int>&
nudge_delays) {
I'd prefer it if this parameter were a map<ModelType, TimeDelta>.  Let's not
store these time durations as ints any longer than we have to.

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_impl.h:254: // Server-tweakable custom nudge delays.
The nudge delay tracking code is somewhat complicated, and reasonably
self-contained.  Maybe you could pull these member variables, and all the
functions that reference them, into a separate class?

SyncSchedulerImpl is already too big.  Let's not make it worse.

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_unittest.cc:498: TimeDelta delay = default_delay();
Remove |delay|?

https://codereview.chromium.org/488843002/diff/20001/sync/engine/syncer_proto...
File sync/engine/syncer_proto_util.cc (right):

https://codereview.chromium.org/488843002/diff/20001/sync/engine/syncer_proto...
sync/engine/syncer_proto_util.cc:444:
delay_map[GetModelTypeFromSpecificsFieldNumber(
Should we be more defensive against unknown model types here?

https://codereview.chromium.org/488843002/diff/20001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/488843002/diff/20001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.cc:91: class NudgeStrategy {
Woo-hoo!  I'm glad to see this being deleted.

https://codereview.chromium.org/488843002/diff/20001/sync/protocol/client_com...
File sync/protocol/client_commands.proto (right):

https://codereview.chromium.org/488843002/diff/20001/sync/protocol/client_com...
sync/protocol/client_commands.proto:44: // |sessions_commit_delay_seconds|
Comment: New in M39?

[Nicolas Zea, 2014-08-20 22:49:44]

Comments address, PTAL. Also moved nudge testing to NudgeTracker's tests.

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
File sync/engine/sync_scheduler_impl.cc (right):

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_impl.cc:865: // Take the shorted delay from all
requested types.
On 2014/08/20 00:13:31, rlarocque wrote:
> shortest?

Done.

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_impl.cc:921: const std::map<ModelType, int>&
nudge_delays) {
On 2014/08/20 00:13:31, rlarocque wrote:
> I'd prefer it if this parameter were a map<ModelType, TimeDelta>.  Let's not
> store these time durations as ints any longer than we have to.

Done.

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
File sync/engine/sync_scheduler_impl.h (right):

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_impl.h:254: // Server-tweakable custom nudge delays.
On 2014/08/20 00:13:31, rlarocque wrote:
> The nudge delay tracking code is somewhat complicated, and reasonably
> self-contained.  Maybe you could pull these member variables, and all the
> functions that reference them, into a separate class?
> 
> SyncSchedulerImpl is already too big.  Let's not make it worse.

Discussed offline, moved this state into NudgeTracker/DataTypeTracker.

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
File sync/engine/sync_scheduler_unittest.cc (right):

https://codereview.chromium.org/488843002/diff/20001/sync/engine/sync_schedul...
sync/engine/sync_scheduler_unittest.cc:498: TimeDelta delay = default_delay();
On 2014/08/20 00:13:31, rlarocque wrote:
> Remove |delay|?

Done.

https://codereview.chromium.org/488843002/diff/20001/sync/engine/syncer_proto...
File sync/engine/syncer_proto_util.cc (right):

https://codereview.chromium.org/488843002/diff/20001/sync/engine/syncer_proto...
sync/engine/syncer_proto_util.cc:444:
delay_map[GetModelTypeFromSpecificsFieldNumber(
On 2014/08/20 00:13:31, rlarocque wrote:
> Should we be more defensive against unknown model types here?

Good call, this has bitten us before.

https://codereview.chromium.org/488843002/diff/20001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.cc (left):

https://codereview.chromium.org/488843002/diff/20001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.cc:91: class NudgeStrategy {
On 2014/08/20 00:13:32, rlarocque wrote:
> Woo-hoo!  I'm glad to see this being deleted.

Acknowledged.

https://codereview.chromium.org/488843002/diff/20001/sync/protocol/client_com...
File sync/protocol/client_commands.proto (right):

https://codereview.chromium.org/488843002/diff/20001/sync/protocol/client_com...
sync/protocol/client_commands.proto:44: // |sessions_commit_delay_seconds|
On 2014/08/20 00:13:32, rlarocque wrote:
> Comment: New in M39?

Done.

[rlarocque, 2014-08-21 17:24:53]

lgtm

https://codereview.chromium.org/488843002/diff/60001/sync/sessions/nudge_trac...
File sync/sessions/nudge_tracker.cc (right):

https://codereview.chromium.org/488843002/diff/60001/sync/sessions/nudge_trac...
sync/sessions/nudge_tracker.cc:20: const int kSyncRefreshDelayMsec = 500;
nit: It would be nice if the Msec vs. Millisecond suffix were consistent.

[Nicolas Zea, 2014-08-22 20:06:25]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-22 20:07:05]

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

[Nicolas Zea, 2014-08-22 20:07:51]

The CQ bit was unchecked by zea@chromium.org

[Nicolas Zea, 2014-08-22 20:15:48]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-22 20:16:24]

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

[commit-bot: I haz the power, 2014-08-22 21:15:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-22 21:41:16]

Committed patchset #6 (100001) as 291512