[Sync] Refactor sync configuration logic.

[Sync] Refactor sync configuration logic.

We remove all the pending download/configure state in SBH, in addition
to the split transaction nature of configurations themselves. This allows
us to have a single SyncScheduler::ScheduleConfiguration command that is 
both synchronous (assuming it doesn't fail) and can handle
 CleanupDisabledTypes and GetKey commands.

This also now keys which datatypes need downloading by checking the
initial sync ended bits directly. This allows us to recover from a new
sync db gracefully.

BUG=129665, 133061, 129825
TEST=unit/integration tests

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

[Nicolas Zea, 2012-06-04 22:02:29]

Sneak peek at my patch if you're curious. This is not ready for review yet.

[rlarocque, 2012-06-04 23:01:38]

Just a few initial thoughts, not an in-depth review.

https://chromiumcodereview.appspot.com/10483015/diff/1/chrome/browser/sync/gl...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://chromiumcodereview.appspot.com/10483015/diff/1/chrome/browser/sync/gl...
chrome/browser/sync/glue/sync_backend_host.cc:1165:
sync_manager_->InitialSyncEndedTypes();
The SyncManager is sitting on the stack frame directly below this call. 
Couldn't it just pass the list of types back up through the function call?  That
way this function (DoFinishConfiugreDataTypes) could return to being purely a
shim for moving this call onto another thread.

https://chromiumcodereview.appspot.com/10483015/diff/1/sync/engine/sync_sched...
File sync/engine/sync_scheduler.cc (right):

https://chromiumcodereview.appspot.com/10483015/diff/1/sync/engine/sync_sched...
sync/engine/sync_scheduler.cc:366: if (params.need_cleanup) {
CleanupDisabledTypesCommand has some strange logic built around the
SyncSessionContext's previous_session_routing_info member.  Because you're
custom-building a sync session for this job anyway, maybe you could eliminate
the need for that logic?

In an ideal world, CleanupDisabledTypes would be a function that takes a
'ModelTypeSet types_to_clean' parameter...

https://chromiumcodereview.appspot.com/10483015/diff/1/sync/engine/sync_sched...
sync/engine/sync_scheduler.cc:393: SyncSessionJob
job(SyncSessionJob::CONFIGURATION,
The arguments for converting SyncSessionJobs into real functions keep piling
up...

https://chromiumcodereview.appspot.com/10483015/diff/1/sync/internal_api/sync...
File sync/internal_api/sync_manager.cc (right):

https://chromiumcodereview.appspot.com/10483015/diff/1/sync/internal_api/sync...
sync/internal_api/sync_manager.cc:876: if (current_routing_info.empty() ||      
           // Restart case.
See CleanupDisabltedTypesCommand::ExecuteImpl().  We shouldn't need to have this
logic in both places.

[tim (not reviewing), 2012-06-05 04:02:08]

https://chromiumcodereview.appspot.com/10483015/diff/1/sync/engine/sync_sched...
File sync/engine/sync_scheduler.cc (right):

https://chromiumcodereview.appspot.com/10483015/diff/1/sync/engine/sync_sched...
sync/engine/sync_scheduler.cc:366: if (params.need_cleanup) {
On 2012/06/04 23:01:38, rlarocque wrote:
> CleanupDisabledTypesCommand has some strange logic built around the
> SyncSessionContext's previous_session_routing_info member.  Because you're
> custom-building a sync session for this job anyway, maybe you could eliminate
> the need for that logic?
> 
> In an ideal world, CleanupDisabledTypes would be a function that takes a
> 'ModelTypeSet types_to_clean' parameter...

We do have a function that takes a 'types to clean' parameter --
PurgeEntriesWithTypeIn on Directory.  CleanupDisabledTypes is built around that
to figure out what should be cleaned up and when. I think ideally, cleanup would
be performed by one or as few pieces of code as possible.  Splitting up the
logic across more components makes it harder to write targeted unit tests.  

I'm wondering, do we actually have to go through the extra trouble here? What if
we just schedule a cleanup and let the command impl figure out if it needs to
run or not?

[Nicolas Zea, 2012-06-07 19:25:42]

Okay, this is now diffed off http://codereview.chromium.org/10542044/, which is
itself rebased off the already landed http://codereview.chromium.org/10537032/.

I've left CleanupDisabledTypes untouched, and now call it with every
configuration.

PTAL!

http://codereview.chromium.org/10483015/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10483015/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:1165:
sync_manager_->InitialSyncEndedTypes();
On 2012/06/04 23:01:38, rlarocque wrote:
> The SyncManager is sitting on the stack frame directly below this call. 
> Couldn't it just pass the list of types back up through the function call? 
That
> way this function (DoFinishConfiugreDataTypes) could return to being purely a
> shim for moving this call onto another thread.

This is not called by the sync manager, but by the sync scheduler (this is the
ready_task callback).

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

http://codereview.chromium.org/10483015/diff/1/sync/engine/sync_scheduler.cc#...
sync/engine/sync_scheduler.cc:366: if (params.need_cleanup) {
On 2012/06/04 23:01:38, rlarocque wrote:
> CleanupDisabledTypesCommand has some strange logic built around the
> SyncSessionContext's previous_session_routing_info member.  Because you're
> custom-building a sync session for this job anyway, maybe you could eliminate
> the need for that logic?
> 
> In an ideal world, CleanupDisabledTypes would be a function that takes a
> 'ModelTypeSet types_to_clean' parameter...

Discussed offline. I've removed this logic and now just trigger cleanup with
every configuration cycle (the logic in the command itself prevents us from
doing extra work).

http://codereview.chromium.org/10483015/diff/1/sync/internal_api/sync_manager.cc
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10483015/diff/1/sync/internal_api/sync_manager...
sync/internal_api/sync_manager.cc:876: if (current_routing_info.empty() ||      
           // Restart case.
On 2012/06/04 23:01:38, rlarocque wrote:
> See CleanupDisabltedTypesCommand::ExecuteImpl().  We shouldn't need to have
this
> logic in both places.

See other comment. This is gone now.

[Nicolas Zea, 2012-06-08 02:25:44]

Most recent patch made tests fail. See my comment below about how to address it.

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

http://codereview.chromium.org/10483015/diff/12003/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:360: // will also allow us to get rid of
previous_session_routing_info.
Note that my removal of this is what's causing the current test failures
(CleanupDisabledTypes uses the session routing info, not the context routing
info, so needs it's own job).

I can either bring the job specifically for CLEANUP_DISABLED_TYPES back here, or
simply have CDT look only at the context routing info (and update the previous
session routing info accordingly).

I'm kind of partial to the latter, since given that CDT always happens with
configuration they should probably be part of the same job.

[Nicolas Zea, 2012-06-08 20:17:38]

Removed last patch set. CleanupDisabledTypes is now being removed in
https://chromiumcodereview.appspot.com/10541079/

[rlarocque, 2012-06-09 01:44:35]

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/backend_data_type_configurer.h (right):

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/backend_data_type_configurer.h:36: const
sync_api::ConfigureReason& reason,
nit: ConfigureReason is an enum.  It's probably better to just copy the value.

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:164: void DoConfigureSyncer(
Could we rename either this method or SBH::RequestConfigureSyncer to make it
more clear these functions are closely related?

(Alternatively, I wouldn't mind deleting RequestConfigureSyncer altogether,
though Tim might object to that.)

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:559: const syncable::ModelTypeSet&
disabled_types,
I think disabled_types is not like the other names.  While 'to_add', 'to_remove'
and 'desired' all seem to indicate the state that we will be in, the name
'disabled' suggests that this is a list of types that have already been
disabled.  

Maybe rename it to 'types_to_disable'?

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1206: if
(sync_prefs_->HasSyncSetupCompleted() &&
Because you check the initial_sync_ended bit for Nigori in the configure, I
think you can remove a lot of this logic.  There's no need to skip the
configuration, since the initial_sync_ended check will make it a no-op if the
nigori is already downloaded.

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/test_p...
File chrome/browser/sync/test_profile_sync_service.h (right):

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/test_p...
chrome/browser/sync/test_profile_sync_service.h:51: const
browser_sync::ModelSafeRoutingInfo routing_info,
I think we usually pass around ModelTypeSet and ModelSafeRoutingInfo as const&.

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

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:346: // We set the routing info for all enabled
types in the session context, but
This comment isn't very helpful.  It describes what we're doing without
explaining why.  

I would consider either removing it, or explaining why the session context
requires the 'full' routing info and why the configuration parameters only see a
subset of the regular routes and workers.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:355: &restricted_workers);
I'm pretty sure Fred's work to only run the ModelSafeWorker if it has work to do
has made this calculation (for workers, at least) obsolete.  

There's no reason to do something about it in this patch, though this might be a
good opportunity to add a TODO.  Someone reading this code might (rightly)
wonder whether or not this calculation is redundant.  A comment would help
clarify this.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:72: void SetBool(bool* flag) {
Could you do this with mocks instead?  This looks odd.

(Not that there's anything wrong with it.  I'm just trying to limit the number
of eyebrow-raising moments one might experience when reading the code.)

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:340:
.WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
I disagree with the use of SimulateCommitFailed here.  A configure session isn't
commit anything.  The scheduler would be perfectly justified in adding a
DCHECK_EQ(UNSET, commit_result).  This test really should be simulating a
download failure.  

(This is not related to your changes, so feel free to not fix this if it looks
like a lot of work.)

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:356: 
ASSERT_FALSE(callback_called) here?

See also the more verbose comment around line 404.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:404: ASSERT_EQ(2U,
records.snapshots.size());
Could you ASSERT_FALSE(callback_called) here?

I think the records.snapshots.size() made a lot more sense when we were picking
up on the configure done by examining snapshots in SyncBackendHost.  We probably
should rely less on those kinds of asserts going forward.  

I'm not sure we should start deleting them yet.  They aren't causing any harm. 
However, we should have their replacements ready to go in case we do decide to
get rid of them.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:603: // Runn again to wait for polling.
typo.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:766: // Runn again to wait for polling.
typo.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:779:
.WillOnce(Invoke(sessions::test_util::SimulateSuccess))
Why make the first invocation of SyncShare special?  Could we not record its
snapshot, too?

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:870: EXPECT_CALL(*syncer(),
This indentation looks strange.  Could you move the SyncShare part onto the
first line?

http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/syncapi_...
File sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:2540: void SetBool(bool* called) {
Same comment as in the other file: I'd prefer to see this implemented with
mocks.

[Nicolas Zea, 2012-06-11 23:05:20]

Comments addressed, PTAL

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/backend_data_type_configurer.h (right):

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/backend_data_type_configurer.h:36: const
sync_api::ConfigureReason& reason,
On 2012/06/09 01:44:35, rlarocque wrote:
> nit: ConfigureReason is an enum.  It's probably better to just copy the value.

Done.

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:164: void DoConfigureSyncer(
On 2012/06/09 01:44:35, rlarocque wrote:
> Could we rename either this method or SBH::RequestConfigureSyncer to make it
> more clear these functions are closely related?
> 
> (Alternatively, I wouldn't mind deleting RequestConfigureSyncer altogether,
> though Tim might object to that.)

I'm not sure what you mean? The whole Request/Do naming convention corresponds
to posting to/from the core, i.e. across threads. Since they both have
ConfigureSyncer in the name, I thought it was fairly clear. What were you
suggesting?

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:559: const syncable::ModelTypeSet&
disabled_types,
On 2012/06/09 01:44:35, rlarocque wrote:
> I think disabled_types is not like the other names.  While 'to_add',
'to_remove'
> and 'desired' all seem to indicate the state that we will be in, the name
> 'disabled' suggests that this is a list of types that have already been
> disabled.  
> 
> Maybe rename it to 'types_to_disable'?

But that implies that it's a delta and not already disabled (like
types_to_add/types_to_remove previously). That's the main reason I switched from
to_add/to_remove in the first place: to make it clear that it's not a delta but
in fact the set of desired/disabled types.

Really, we should be able to get rid of disabled_types completely and calculate
it based on the set of registered types (possibly by getting that from the
registrar?), but I think that's outside the scope of this patch. I'll add a todo
about that though.

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1206: if
(sync_prefs_->HasSyncSetupCompleted() &&
On 2012/06/09 01:44:35, rlarocque wrote:
> Because you check the initial_sync_ended bit for Nigori in the configure, I
> think you can remove a lot of this logic.  There's no need to skip the
> configuration, since the initial_sync_ended check will make it a no-op if the
> nigori is already downloaded.

Done.

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/test_p...
File chrome/browser/sync/test_profile_sync_service.h (right):

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/test_p...
chrome/browser/sync/test_profile_sync_service.h:51: const
browser_sync::ModelSafeRoutingInfo routing_info,
On 2012/06/09 01:44:35, rlarocque wrote:
> I think we usually pass around ModelTypeSet and ModelSafeRoutingInfo as
const&.

Done.

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

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:346: // We set the routing info for all enabled
types in the session context, but
On 2012/06/09 01:44:35, rlarocque wrote:
> This comment isn't very helpful.  It describes what we're doing without
> explaining why.  
> 
> I would consider either removing it, or explaining why the session context
> requires the 'full' routing info and why the configuration parameters only see
a
> subset of the regular routes and workers.

Removed comment.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:355: &restricted_workers);
On 2012/06/09 01:44:35, rlarocque wrote:
> I'm pretty sure Fred's work to only run the ModelSafeWorker if it has work to
do
> has made this calculation (for workers, at least) obsolete.  
> 
> There's no reason to do something about it in this patch, though this might be
a
> good opportunity to add a TODO.  Someone reading this code might (rightly)
> wonder whether or not this calculation is redundant.  A comment would help
> clarify this.  

Added TODO.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:72: void SetBool(bool* flag) {
On 2012/06/09 01:44:35, rlarocque wrote:
> Could you do this with mocks instead?  This looks odd.
> 
> (Not that there's anything wrong with it.  I'm just trying to limit the number
> of eyebrow-raising moments one might experience when reading the code.)

Given that we pass in a callback, not an object, I think this is a more
straightforward way of testing that the callback is actually run. Mocks are more
useful for testing member methods of mock classes (although even there,
Fred/Drew have convinced me to use fake's over mock's more often).

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:340:
.WillOnce(DoAll(Invoke(sessions::test_util::SimulateCommitFailed),
On 2012/06/09 01:44:35, rlarocque wrote:
> I disagree with the use of SimulateCommitFailed here.  A configure session
isn't
> commit anything.  The scheduler would be perfectly justified in adding a
> DCHECK_EQ(UNSET, commit_result).  This test really should be simulating a
> download failure.  
> 
> (This is not related to your changes, so feel free to not fix this if it looks
> like a lot of work.)

This change is large enough, I'd rather leave it as is.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:356: 
On 2012/06/09 01:44:35, rlarocque wrote:
> ASSERT_FALSE(callback_called) here?
> 
> See also the more verbose comment around line 404.

Done.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:404: ASSERT_EQ(2U,
records.snapshots.size());
On 2012/06/09 01:44:35, rlarocque wrote:
> Could you ASSERT_FALSE(callback_called) here?
> 
> I think the records.snapshots.size() made a lot more sense when we were
picking
> up on the configure done by examining snapshots in SyncBackendHost.  We
probably
> should rely less on those kinds of asserts going forward.  
> 
> I'm not sure we should start deleting them yet.  They aren't causing any harm.

> However, we should have their replacements ready to go in case we do decide to
> get rid of them.

Done.

I think the snapshot size is still a useful way of tracking the sync cycles.
Yes, we don't rely on the sync cycle completion event for configuration anymore,
but we still have expectations about the cycles occurring and how often.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:603: // Runn again to wait for polling.
On 2012/06/09 01:44:35, rlarocque wrote:
> typo.

Done.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:766: // Runn again to wait for polling.
On 2012/06/09 01:44:35, rlarocque wrote:
> typo.

Done.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:779:
.WillOnce(Invoke(sessions::test_util::SimulateSuccess))
On 2012/06/09 01:44:35, rlarocque wrote:
> Why make the first invocation of SyncShare special?  Could we not record its
> snapshot, too?

This is getting removed altogether in a later patch (see
http://codereview.chromium.org/10541079/)

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:870: EXPECT_CALL(*syncer(),
On 2012/06/09 01:44:35, rlarocque wrote:
> This indentation looks strange.  Could you move the SyncShare part onto the
> first line?

Done.

http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/syncapi_...
File sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:2540: void SetBool(bool* called) {
On 2012/06/09 01:44:35, rlarocque wrote:
> Same comment as in the other file: I'd prefer to see this implemented with
> mocks.

See other response.

[rlarocque, 2012-06-12 17:45:13]

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10483015/diff/17001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:164: void DoConfigureSyncer(
On 2012/06/11 23:05:20, nzea wrote:
> On 2012/06/09 01:44:35, rlarocque wrote:
> > Could we rename either this method or SBH::RequestConfigureSyncer to make it
> > more clear these functions are closely related?
> > 
> > (Alternatively, I wouldn't mind deleting RequestConfigureSyncer altogether,
> > though Tim might object to that.)
> 
> I'm not sure what you mean? The whole Request/Do naming convention corresponds
> to posting to/from the core, i.e. across threads. Since they both have
> ConfigureSyncer in the name, I thought it was fairly clear. What were you
> suggesting?

I got confused between the old and new names of this function.  I withdraw my
comment.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:72: void SetBool(bool* flag) {
On 2012/06/11 23:05:20, nzea wrote:
> On 2012/06/09 01:44:35, rlarocque wrote:
> > Could you do this with mocks instead?  This looks odd.
> > 
> > (Not that there's anything wrong with it.  I'm just trying to limit the
number
> > of eyebrow-raising moments one might experience when reading the code.)
> 
> Given that we pass in a callback, not an object, I think this is a more
> straightforward way of testing that the callback is actually run. Mocks are
more
> useful for testing member methods of mock classes (although even there,
> Fred/Drew have convinced me to use fake's over mock's more often).

You have a point that the mock framework is probably more trouble than its
worth.  

Maybe you could write a simple "CallbackCounter" class?  My main concern is that
I'd like to see a more descriptive type name than "bool".  It would also have
the advantage that it could initialize itself.

http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/sync_man...
sync/internal_api/sync_manager.cc:1148: // TODO(sync): pass this into the
scheduler somehow.
What is 'this' referring to?

[Nicolas Zea, 2012-06-12 20:44:44]

Comments addressed.

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:72: void SetBool(bool* flag) {
On 2012/06/12 17:45:13, rlarocque wrote:
> On 2012/06/11 23:05:20, nzea wrote:
> > On 2012/06/09 01:44:35, rlarocque wrote:
> > > Could you do this with mocks instead?  This looks odd.
> > > 
> > > (Not that there's anything wrong with it.  I'm just trying to limit the
> number
> > > of eyebrow-raising moments one might experience when reading the code.)
> > 
> > Given that we pass in a callback, not an object, I think this is a more
> > straightforward way of testing that the callback is actually run. Mocks are
> more
> > useful for testing member methods of mock classes (although even there,
> > Fred/Drew have convinced me to use fake's over mock's more often).
> 
> You have a point that the mock framework is probably more trouble than its
> worth.  
> 
> Maybe you could write a simple "CallbackCounter" class?  My main concern is
that
> I'd like to see a more descriptive type name than "bool".  It would also have
> the advantage that it could initialize itself.

Done.

http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/sync_man...
sync/internal_api/sync_manager.cc:1148: // TODO(sync): pass this into the
scheduler somehow.
On 2012/06/12 17:45:13, rlarocque wrote:
> What is 'this' referring to?

This comment has been updated in the newer patch.

[rlarocque, 2012-06-12 20:59:26]

On 2012/06/12 20:44:44, nzea wrote:
> Comments addressed.
> 
>
http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
> File sync/engine/sync_scheduler_unittest.cc (right):
> 
>
http://codereview.chromium.org/10483015/diff/17001/sync/engine/sync_scheduler...
> sync/engine/sync_scheduler_unittest.cc:72: void SetBool(bool* flag) {
> On 2012/06/12 17:45:13, rlarocque wrote:
> > On 2012/06/11 23:05:20, nzea wrote:
> > > On 2012/06/09 01:44:35, rlarocque wrote:
> > > > Could you do this with mocks instead?  This looks odd.
> > > > 
> > > > (Not that there's anything wrong with it.  I'm just trying to limit the
> > number
> > > > of eyebrow-raising moments one might experience when reading the code.)
> > > 
> > > Given that we pass in a callback, not an object, I think this is a more
> > > straightforward way of testing that the callback is actually run. Mocks
are
> > more
> > > useful for testing member methods of mock classes (although even there,
> > > Fred/Drew have convinced me to use fake's over mock's more often).
> > 
> > You have a point that the mock framework is probably more trouble than its
> > worth.  
> > 
> > Maybe you could write a simple "CallbackCounter" class?  My main concern is
> that
> > I'd like to see a more descriptive type name than "bool".  It would also
have
> > the advantage that it could initialize itself.
> 
> Done.
> 
>
http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/sync_man...
> File sync/internal_api/sync_manager.cc (right):
> 
>
http://codereview.chromium.org/10483015/diff/17001/sync/internal_api/sync_man...
> sync/internal_api/sync_manager.cc:1148: // TODO(sync): pass this into the
> scheduler somehow.
> On 2012/06/12 17:45:13, rlarocque wrote:
> > What is 'this' referring to?
> 
> This comment has been updated in the newer patch.

One more request: Could you try running the first of the tests described in
http://codereview.chromium.org/10520010/?  I'd like to confirm that this patch
actually does contain all the fixes from that CL.  

LGTM.  I think you should wait for Tim's approval before committing, though.

[tim (not reviewing), 2012-06-14 00:26:53]

https://chromiumcodereview.appspot.com/10483015/diff/12008/chrome/browser/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://chromiumcodereview.appspot.com/10483015/diff/12008/chrome/browser/syn...
chrome/browser/sync/glue/sync_backend_host.cc:557: // registered types - the set
of desired types.
I originally wrote it that way, but I think some folks (Fred, I think?)
preferred to have this API be explicit about added and removed types. See
changes to sync_backend_host.cc in http://codereview.chromium.org/7511004. 

Should we have a referendum and write the outcome it in a comment? :P

https://chromiumcodereview.appspot.com/10483015/diff/12008/chrome/browser/syn...
chrome/browser/sync/glue/sync_backend_host.cc:607: //    is always included if
we do a configuration).
This (and the logic below) seems a bit convoluted?  If we are downloading
anything at all, we must always download NIGORI. Unless there is explicit need
to disable NIGORI (e.g. for migration).  Mentioning 'configuration' here and
making it depend on 'if (types_to_config.Size() > 1)' makes it sound more
special cased than it is.

https://chromiumcodereview.appspot.com/10483015/diff/12008/chrome/browser/syn...
chrome/browser/sync/glue/sync_backend_host.cc:615: if (types_to_config.Size() >
1 && enabled_types.Has(syncable::NIGORI))
Your comment above says "any other data type is being configured" but you check
> 1 (and not > 0, or empty()).

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
File sync/engine/sync_scheduler.cc (right):

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
sync/engine/sync_scheduler.cc:358: // remove this command and add a call to
PurgeEntriesWithTypeIn here. This
nit - well, we don't want to poke the directory directly here

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
sync/engine/sync_scheduler.cc:367: DoSyncSessionJob(cleanup_job);
Hmm. So, this might choose not to actually execute the job, but we don't do any
error checking and continue through.  That seems bad.  Of course, I'm pretty
sure we're relying on the implicit fact that cleanup_disabled_types will just
always run :)  A comment explaining would be nice.

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
sync/engine/sync_scheduler.cc:372: NOTIMPLEMENTED();
bug?

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
sync/engine/sync_scheduler.cc:375: // Only reconfigure if we have types to
config.
It seems weird that this whole flow is called config, but this comment finds
itself half way through.  Should |types_to_config| be called
|types_to_fetch/download|?

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
sync/engine/sync_scheduler.cc:378: // TODO(tim): config-specific
GetUpdatesCallerInfo value?
We have a config specific GetUpdatesCallerInfo...

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
sync/engine/sync_scheduler.cc:904: // we finished successfully.
Why do this in 'ScheduleNextSync'?  It's not really part of the contract... I
don't think there's a reason we have to do this after AdjustPolling, unless I'm
really missing something?  Can we do it before calling ScheduleNextSync instead?

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
File sync/engine/sync_scheduler.h (right):

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/engine/sync_s...
sync/engine/sync_scheduler.h:40: struct ConfigureParams {
nit - ConfigurationParams, to match method

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/internal_api/...
File sync/internal_api/sync_manager.cc (right):

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/internal_api/...
sync/internal_api/sync_manager.cc:862: // TODO(zea): set this based on whether
cryptographer has keystore
bug?

Also... dare I say it... enum vs bool?

https://chromiumcodereview.appspot.com/10483015/diff/12008/sync/internal_api/...
sync/internal_api/sync_manager.cc:1149: // to normal mode. Figure out how to
enforce set_routing_info is always
Hmm... what do you want to figure out here, exactly?

[Nicolas Zea, 2012-06-14 22:35:52]

Comments addressed, PTAL.

http://codereview.chromium.org/10483015/diff/12008/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10483015/diff/12008/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:557: // registered types - the set
of desired types.
On 2012/06/14 00:26:53, timsteele wrote:
> I originally wrote it that way, but I think some folks (Fred, I think?)
> preferred to have this API be explicit about added and removed types. See
> changes to sync_backend_host.cc in http://codereview.chromium.org/7511004. 
> 
> Should we have a referendum and write the outcome it in a comment? :P
> 

Well, previously we had to rely on having the set of datatype controllers around
to determine this, and i don't think we could rely on the registrar to get it at
that point? I think with all the changes recently we might be able to in a
clean/reliable way, but I'd need to dig a bit more to be certain.

http://codereview.chromium.org/10483015/diff/12008/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:607: //    is always included if
we do a configuration).
On 2012/06/14 00:26:53, timsteele wrote:
> This (and the logic below) seems a bit convoluted?  If we are downloading
> anything at all, we must always download NIGORI. Unless there is explicit need
> to disable NIGORI (e.g. for migration).  Mentioning 'configuration' here and
> making it depend on 'if (types_to_config.Size() > 1)' makes it sound more
> special cased than it is.

I disagree that it's not already special cased :-). That said, what are you
suggesting? I've gone ahead and changed "configured" to "downloaded", to be more
accurate, but I think the logic below (barring the bug you found) is
straightforward?

http://codereview.chromium.org/10483015/diff/12008/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:615: if (types_to_config.Size() >
1 && enabled_types.Has(syncable::NIGORI))
On 2012/06/14 00:26:53, timsteele wrote:
> Your comment above says "any other data type is being configured" but you
check
> > 1 (and not > 0, or empty()). 

Good catch, the logic was slightly different in a previous version and I didn't
update this appropriately. Done.

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

http://codereview.chromium.org/10483015/diff/12008/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:358: // remove this command and add a call to
PurgeEntriesWithTypeIn here. This
On 2012/06/14 00:26:53, timsteele wrote:
> nit - well, we don't want to poke the directory directly here

Removed comment. Note that this is being remove anyways as part of
http://codereview.chromium.org/10541079/.

http://codereview.chromium.org/10483015/diff/12008/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:367: DoSyncSessionJob(cleanup_job);
On 2012/06/14 00:26:53, timsteele wrote:
> Hmm. So, this might choose not to actually execute the job, but we don't do
any
> error checking and continue through.  That seems bad.  Of course, I'm pretty
> sure we're relying on the implicit fact that cleanup_disabled_types will just
> always run :)  A comment explaining would be nice.

Done, and mentioned bug number for getting rid of this.

http://codereview.chromium.org/10483015/diff/12008/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:372: NOTIMPLEMENTED();
On 2012/06/14 00:26:53, timsteele wrote:
> bug?

Done.

http://codereview.chromium.org/10483015/diff/12008/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:375: // Only reconfigure if we have types to
config.
On 2012/06/14 00:26:53, timsteele wrote:
> It seems weird that this whole flow is called config, but this comment finds
> itself half way through.  Should |types_to_config| be called
> |types_to_fetch/download|?
> 

Done.

http://codereview.chromium.org/10483015/diff/12008/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:378: // TODO(tim): config-specific
GetUpdatesCallerInfo value?
On 2012/06/14 00:26:53, timsteele wrote:
> We have a config specific GetUpdatesCallerInfo... 

Woops, I removed this is the other patch, but it came back here. Done.

http://codereview.chromium.org/10483015/diff/12008/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:904: // we finished successfully.
On 2012/06/14 00:26:53, timsteele wrote:
> Why do this in 'ScheduleNextSync'?  It's not really part of the contract... I
> don't think there's a reason we have to do this after AdjustPolling, unless
I'm
> really missing something?  Can we do it before calling ScheduleNextSync
instead?

Primarily just because this is where we were already checking Succeeded(). But,
there's not reason we can't check it in more than one place. Done.

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

http://codereview.chromium.org/10483015/diff/12008/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.h:40: struct ConfigureParams {
On 2012/06/14 00:26:53, timsteele wrote:
> nit - ConfigurationParams, to match method

Done.

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
sync/internal_api/sync_manager.cc:862: // TODO(zea): set this based on whether
cryptographer has keystore
On 2012/06/14 00:26:53, timsteele wrote:
> bug?
> 
> Also... dare I say it... enum vs bool?

:-/ Done to both.

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
sync/internal_api/sync_manager.cc:1149: // to normal mode. Figure out how to
enforce set_routing_info is always
On 2012/06/14 00:26:53, timsteele wrote:
> Hmm... what do you want to figure out here, exactly?  

Eventually, I'd like to see the updating of the context's routing info happen
automatically within the scheduler, and not require the caller to remember to
set it properly. We only really need to set it when we call
Start(NORMAL_MODE)...

[tim (not reviewing), 2012-06-15 14:59:36]

Mainly just a test issue / comment left

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
sync/internal_api/sync_manager.cc:1149: // to normal mode. Figure out how to
enforce set_routing_info is always
On 2012/06/14 22:35:52, nzea wrote:
> On 2012/06/14 00:26:53, timsteele wrote:
> > Hmm... what do you want to figure out here, exactly?  
> 
> Eventually, I'd like to see the updating of the context's routing info happen
> automatically within the scheduler, and not require the caller to remember to
> set it properly. We only really need to set it when we call
> Start(NORMAL_MODE)...

I agree it's clunky to have to remember to set it :/  In the old registrar world
the scheduler and friends would take care of auto updating the routing info (by
grabbing a copy from the registrar).  What are you proposing?  (This doesn't
matter for this review, I was just curious)

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

http://codereview.chromium.org/10483015/diff/11011/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:350: // the routing info, we don't really need
|restricted_workers|. Remove it.
Bug would be nice here too!

http://codereview.chromium.org/10483015/diff/11011/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:377: // Only reconfigure if we have types to
config.
s/config/download

http://codereview.chromium.org/10483015/diff/11011/sync/internal_api/syncapi_...
File sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/11011/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:739: virtual bool DoConfigureSyncer(
Ugh.  I see the corner you're in here.  I'm really worried about adding more of
these Test impls, though. It leads to pain and brittleness (just look at
SyncBackendHostForTest).

It would be much better to pass a fake / mock SyncScheduler and have
set_config_result on that class instead, and here test the real SyncManager. You
could make the fixture a friend of SyncManager to allow setting your fake
scheduler.  I know some of these tests rely on a null scheduler, but perhaps you
could stuff it in in your specific test?  And in the future, tests should rely
on our specially crafted fake scheduler vs a null one (to make sure e.g. we
don't actually try and ping the server).

http://codereview.chromium.org/10483015/diff/11011/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:743: ADD_FAILURE();
nit, in general something like ADD_FAILURE() << "Null ready_task in
DoConfigureSyncer" ? would be more useful.

[Nicolas Zea, 2012-06-15 21:23:35]

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
sync/internal_api/sync_manager.cc:1149: // to normal mode. Figure out how to
enforce set_routing_info is always
On 2012/06/15 14:59:36, timsteele wrote:
> On 2012/06/14 22:35:52, nzea wrote:
> > On 2012/06/14 00:26:53, timsteele wrote:
> > > Hmm... what do you want to figure out here, exactly?  
> > 
> > Eventually, I'd like to see the updating of the context's routing info
happen
> > automatically within the scheduler, and not require the caller to remember
to
> > set it properly. We only really need to set it when we call
> > Start(NORMAL_MODE)...
> 
> I agree it's clunky to have to remember to set it :/  In the old registrar
world
> the scheduler and friends would take care of auto updating the routing info
(by
> grabbing a copy from the registrar).  What are you proposing?  (This doesn't
> matter for this review, I was just curious)

I think the simplest option might be passing the routing info in via Start(..).
Then perhaps making the set_routing_info method be private and friend the
scheduler? Not a big fan of friending, but I'd like to make it clear it should
only be set within the scheduler.

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

http://codereview.chromium.org/10483015/diff/11011/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:350: // the routing info, we don't really need
|restricted_workers|. Remove it.
On 2012/06/15 14:59:37, timsteele wrote:
> Bug would be nice here too!

Done.

http://codereview.chromium.org/10483015/diff/11011/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:377: // Only reconfigure if we have types to
config.
On 2012/06/15 14:59:37, timsteele wrote:
> s/config/download

Done.

http://codereview.chromium.org/10483015/diff/11011/sync/internal_api/syncapi_...
File sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/11011/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:739: virtual bool DoConfigureSyncer(
On 2012/06/15 14:59:37, timsteele wrote:
> Ugh.  I see the corner you're in here.  I'm really worried about adding more
of
> these Test impls, though. It leads to pain and brittleness (just look at
> SyncBackendHostForTest).
> 
> It would be much better to pass a fake / mock SyncScheduler and have
> set_config_result on that class instead, and here test the real SyncManager.
You
> could make the fixture a friend of SyncManager to allow setting your fake
> scheduler.  I know some of these tests rely on a null scheduler, but perhaps
you
> could stuff it in in your specific test?  And in the future, tests should rely
> on our specially crafted fake scheduler vs a null one (to make sure e.g. we
> don't actually try and ping the server).

I see what you're saying, but given that that's kind of a refactor of how we
test the SyncManager (and this patch is already fairly large), mind if I do it
as a followup patch? I've added a TODO/bug in sync_manager.h.

http://codereview.chromium.org/10483015/diff/11011/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:743: ADD_FAILURE();
On 2012/06/15 14:59:37, timsteele wrote:
> nit, in general something like ADD_FAILURE() << "Null ready_task in
> DoConfigureSyncer" ? would be more useful.

Done.

[tim (not reviewing), 2012-06-15 21:58:24]

I will take a look as soon as I get back online (have to run now), but LGTM
contingent on the mock scheduler comment + offline chat.

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
File sync/internal_api/sync_manager.cc (right):

http://codereview.chromium.org/10483015/diff/12008/sync/internal_api/sync_man...
sync/internal_api/sync_manager.cc:1149: // to normal mode. Figure out how to
enforce set_routing_info is always
On 2012/06/15 21:23:36, nzea wrote:
> On 2012/06/15 14:59:36, timsteele wrote:
> > On 2012/06/14 22:35:52, nzea wrote:
> > > On 2012/06/14 00:26:53, timsteele wrote:
> > > > Hmm... what do you want to figure out here, exactly?  
> > > 
> > > Eventually, I'd like to see the updating of the context's routing info
> happen
> > > automatically within the scheduler, and not require the caller to remember
> to
> > > set it properly. We only really need to set it when we call
> > > Start(NORMAL_MODE)...
> > 
> > I agree it's clunky to have to remember to set it :/  In the old registrar
> world
> > the scheduler and friends would take care of auto updating the routing info
> (by
> > grabbing a copy from the registrar).  What are you proposing?  (This doesn't
> > matter for this review, I was just curious)
> 
> I think the simplest option might be passing the routing info in via
Start(..).
> Then perhaps making the set_routing_info method be private and friend the
> scheduler? Not a big fan of friending, but I'd like to make it clear it should
> only be set within the scheduler.

OK. But I think there are friendless options :)  I proposed moving the context_
ownership from scheduler to SyncManager as part of the registrar-ectomy
precisely so that the SyncManager could set the routing info.  I would rather
not have an explicit setter on the scheduler for the context, but if it's
coupled to some other method (and can't change without another call to said
method / switching modes), we could move ownership back, meaning no one else
would have access to it.

I'm sure we can come up with something sufficiently elegant.

http://codereview.chromium.org/10483015/diff/11011/sync/internal_api/syncapi_...
File sync/internal_api/syncapi_unittest.cc (right):

http://codereview.chromium.org/10483015/diff/11011/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:739: virtual bool DoConfigureSyncer(
On 2012/06/15 21:23:36, nzea wrote:
> On 2012/06/15 14:59:37, timsteele wrote:
> > Ugh.  I see the corner you're in here.  I'm really worried about adding more
> of
> > these Test impls, though. It leads to pain and brittleness (just look at
> > SyncBackendHostForTest).
> > 
> > It would be much better to pass a fake / mock SyncScheduler and have
> > set_config_result on that class instead, and here test the real SyncManager.
> You
> > could make the fixture a friend of SyncManager to allow setting your fake
> > scheduler.  I know some of these tests rely on a null scheduler, but perhaps
> you
> > could stuff it in in your specific test?  And in the future, tests should
rely
> > on our specially crafted fake scheduler vs a null one (to make sure e.g. we
> > don't actually try and ping the server).
> 
> I see what you're saying, but given that that's kind of a refactor of how we
> test the SyncManager (and this patch is already fairly large), mind if I do it
> as a followup patch? I've added a TODO/bug in sync_manager.h.

As we discussed offline, I think we can special case your tests to have access
to reset the scheduler without touching the rest.

[Nicolas Zea, 2012-06-15 22:31:13]

Done, committing once trybots go green.

[rlarocque, 2012-06-15 22:50:56]

Before you commit, could you add crbug.com/129825 to the BUG= list?  This CL
fixes it, too.