[Sync] Add RequestCleanupDisabledTypes() method to SyncManager

[Sync] Add RequestCleanupDisabledTypes() method to SyncManager

Use that instead of nudging when a type is disabled.

Cleaned up SyncScheduler and SyncManager a bit.

BUG=78165
TEST=
TBR=tim

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

[akalin, 2011-06-29 20:58:13]

+tim for review

[tim (not reviewing), 2011-06-29 22:50:52]

http://codereview.chromium.org/7281017/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/sync_scheduler_unittest.cc (right):

http://codereview.chromium.org/7281017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/sync_scheduler_unittest.cc:982: PumpLoop();
remind me again why we have to do this twice now?
shouldn't we roll these together, either into PumpLoop() or PumpOnce and
PumpForSpecialReason (twice)

http://codereview.chromium.org/7281017/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7281017/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi.cc:1893:
scheduler()->ScheduleDisabledTypesCleanup();
It does seem weird to always do this at this layer, I assume it was easier than
scheduling from SBH?  On the flip side... would it'd be weirder to do it inside
the scheduler Start()?  and not bother exposing this method (I suggest it since
it looks like you're trying to bury the api)

Have you tried running the Migration* integration tests?
The migrator explicitly waits for cleanups to happen...

[tim (not reviewing), 2011-07-21 16:55:42]

On 2011/06/29 22:50:52, timsteele wrote:
>
http://codereview.chromium.org/7281017/diff/1/chrome/browser/sync/engine/sync...
> File chrome/browser/sync/engine/sync_scheduler_unittest.cc (right):
> 
>
http://codereview.chromium.org/7281017/diff/1/chrome/browser/sync/engine/sync...
> chrome/browser/sync/engine/sync_scheduler_unittest.cc:982: PumpLoop();
> remind me again why we have to do this twice now?
> shouldn't we roll these together, either into PumpLoop() or PumpOnce and
> PumpForSpecialReason (twice)
> 
>
http://codereview.chromium.org/7281017/diff/1/chrome/browser/sync/engine/sync...
> File chrome/browser/sync/engine/syncapi.cc (right):
> 
>
http://codereview.chromium.org/7281017/diff/1/chrome/browser/sync/engine/sync...
> chrome/browser/sync/engine/syncapi.cc:1893:
> scheduler()->ScheduleDisabledTypesCleanup();
> It does seem weird to always do this at this layer, I assume it was easier
than
> scheduling from SBH?  On the flip side... would it'd be weirder to do it
inside
> the scheduler Start()?  and not bother exposing this method (I suggest it
since
> it looks like you're trying to bury the api)
> 
> Have you tried running the Migration* integration tests?
> The migrator explicitly waits for cleanups to happen...

ping?

[akalin, 2011-07-21 19:30:24]

> ping?

Sorry, got discouraged by yaks.  I think I'll send out another CL to add the
sync cycle type (normal, config, cleanup) to the harness stuff

[akalin, 2011-07-27 18:17:07]

On 2011/07/21 19:30:24, akalin wrote:
> > ping?
> 
> Sorry, got discouraged by yaks.  I think I'll send out another CL to add the
> sync cycle type (normal, config, cleanup) to the harness stuff

Updated patch to make it less invasive.

[akalin, 2011-07-28 00:29:40]

> Updated patch to make it less invasive.

Integration tests should pass now, please take a look.

[tim (not reviewing), 2011-07-28 01:42:23]

http://codereview.chromium.org/7281017/diff/15001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/7281017/diff/15001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.h:289: void CleanupDisabledTypes();
So, I get why this is here, but it confuses me to have two separate ways to do
this. Since the canonical way the syncer works is via its internal state
machine, maybe we should be thinking about this that way.  E.g. I just realized
a better way to do what I tried to do before (pulling SyncerEndCommand out of
the cycle) would have been to simply add a condition in case APPLY_UPDATES:,
around the assignment to next_state, to consider the end_state.  If end_state ==
APPLY_UPDATES then next_state = SYNCER_END.

I think that solves all the world's problems?

[akalin, 2011-07-28 02:45:50]

PTAL

http://codereview.chromium.org/7281017/diff/15001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.h (right):

http://codereview.chromium.org/7281017/diff/15001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.h:289: void CleanupDisabledTypes();
On 2011/07/28 01:42:23, timsteele wrote:
> So, I get why this is here, but it confuses me to have two separate ways to do
> this. Since the canonical way the syncer works is via its internal state
> machine, maybe we should be thinking about this that way.  E.g. I just
realized
> a better way to do what I tried to do before (pulling SyncerEndCommand out of
> the cycle) would have been to simply add a condition in case APPLY_UPDATES:,
> around the assignment to next_state, to consider the end_state.  If end_state
==
> APPLY_UPDATES then next_state = SYNCER_END.
> 
> I think that solves all the world's problems?

Done.  Let's see if that works.

[tim (not reviewing), 2011-07-28 04:10:40]

one comment / question

http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:759: job.purpose !=
SyncSessionJob::CLEANUP_DISABLED_TYPES) {
hmm.. are you sure about this?  See
http://code.google.com/p/chromium/issues/detail?id=71616  and
http://code.google.com/p/chromium/issues/detail?id=57360

[akalin, 2011-07-28 06:36:45]

http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:759: job.purpose !=
SyncSessionJob::CLEANUP_DISABLED_TYPES) {
On 2011/07/28 04:10:40, timsteele wrote:
> hmm.. are you sure about this?  See
> http://code.google.com/p/chromium/issues/detail?id=71616  and
> http://code.google.com/p/chromium/issues/detail?id=57360

Well, we'll hit the DCHECK otherwise.  And even if we pass that, it'll just get
dropped anyway, since only configures and nudges are saved.

At least for cleanup disabled types, saving the job doesn't make sense as it's
not affected by the wait interval and doesn't communicate with the server.

Those issues are valid, but I don't think this CL is the one to fix them,
especially since ClearUserData isn't even hooked up to anything.  I don't know
if we should even keep that code around.

[tim (not reviewing), 2011-07-28 17:28:04]

http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:759: job.purpose !=
SyncSessionJob::CLEANUP_DISABLED_TYPES) {
On 2011/07/28 06:36:45, akalin wrote:
> On 2011/07/28 04:10:40, timsteele wrote:
> > hmm.. are you sure about this?  See
> > http://code.google.com/p/chromium/issues/detail?id=71616  and
> > http://code.google.com/p/chromium/issues/detail?id=57360
> 
> Well, we'll hit the DCHECK otherwise.  And even if we pass that, it'll just
get
> dropped anyway, since only configures and nudges are saved.
> 
> At least for cleanup disabled types, saving the job doesn't make sense as it's
> not affected by the wait interval and doesn't communicate with the server.
> 
> Those issues are valid, but I don't think this CL is the one to fix them,
> especially since ClearUserData isn't even hooked up to anything.  I don't know
> if we should even keep that code around.

Well, my point is that one of the main things we did last quarter was make all
sync jobs reliable, so that we can make a blanket simplifying assumption that
we'll never drop jobs. It seems wrong to intentionally regress something by
inspection that we just fixed not that long ago :) 

Maybe I'm misremembering, so to be on the safe side I'll cc Lingesh.

[akalin, 2011-07-28 17:50:27]

http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.cc:759: job.purpose !=
SyncSessionJob::CLEANUP_DISABLED_TYPES) {
> Maybe I'm misremembering, so to be on the safe side I'll cc Lingesh. 

Replaced with TODOs, PTAL.

[akalin, 2011-07-28 19:38:35]

On 2011/07/28 17:50:27, akalin wrote:
>
http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
> File chrome/browser/sync/engine/sync_scheduler.cc (right):
> 
>
http://codereview.chromium.org/7281017/diff/14013/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/sync_scheduler.cc:759: job.purpose !=
> SyncSessionJob::CLEANUP_DISABLED_TYPES) {
> > Maybe I'm misremembering, so to be on the safe side I'll cc Lingesh. 
> 
> Replaced with TODOs, PTAL.

Checking in TBR=tim, as discussed