Fix some migration-related bugs

Fix some migration-related bugs

This change treat jobs that receive MIGRATION_DONE response from the
server as a failure, so that they will be retried.  Otherwise we could
end up unnecessarily dropping nudges or notifications.  This was already
effectively the case for jobs that committed items for types that were
being migrated due to a detail of the way ProcessCommitResponse errors
were detected.

This also modifies the handling of configuration done notifications in
the ProfileSyncService so it no longer causes the scheduler to briefly
transition back to NORMAL mode in mid-migration.  The old way of doing
things would often lead to pending nudges being executed while the
migration was still in progress.

Finally, this change reroutes the backend migrator's OnConfigureDone
callback thorugh ProfileSyncService.  Now we can guarantee that the
ProfileSyncService's callbck gets executed first.  This should be less
brittle than hoping that the notifications arrive in a certain order.

BUG=123954, 94882
TEST=


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

[tim (not reviewing), 2012-04-20 22:38:09]

http://codereview.chromium.org/10167017/diff/2001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10167017/diff/2001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:300: void StartSyncingWithServer();
We can't expose this through here. Any old code in chrome/browser should not
concern itself with, or worse,  depend on / tinker with this most fundamental of
actions in sync. Can we hand the BackendMigrator a callback instead?

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

http://codereview.chromium.org/10167017/diff/2001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:170: bool Succeeded();
I don't think this is part of this patch?

[rlarocque, 2012-04-20 22:45:08]

http://codereview.chromium.org/10167017/diff/2001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10167017/diff/2001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:300: void StartSyncingWithServer();
On 2012/04/20 22:38:09, timsteele wrote:
> We can't expose this through here. Any old code in chrome/browser should not
> concern itself with, or worse,  depend on / tinker with this most fundamental
of
> actions in sync. Can we hand the BackendMigrator a callback instead?

I was struggling to find the least bad way to do this.  I never thought of using
a callback as a means of information hiding.  I'll give it a shot.

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

http://codereview.chromium.org/10167017/diff/2001/sync/engine/sync_scheduler....
sync/engine/sync_scheduler.h:170: bool Succeeded();
On 2012/04/20 22:38:09, timsteele wrote:
> I don't think this is part of this patch?

No.  I have no idea how this got included.

[rlarocque, 2012-04-21 00:26:27]

On 2012/04/20 22:45:08, rlarocque wrote:
>
http://codereview.chromium.org/10167017/diff/2001/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service.h (right):
> 
>
http://codereview.chromium.org/10167017/diff/2001/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service.h:300: void StartSyncingWithServer();
> On 2012/04/20 22:38:09, timsteele wrote:
> > We can't expose this through here. Any old code in chrome/browser should not
> > concern itself with, or worse,  depend on / tinker with this most
fundamental
> of
> > actions in sync. Can we hand the BackendMigrator a callback instead?
> 
> I was struggling to find the least bad way to do this.  I never thought of
using
> a callback as a means of information hiding.  I'll give it a shot.
> 
> http://codereview.chromium.org/10167017/diff/2001/sync/engine/sync_scheduler.h
> File sync/engine/sync_scheduler.h (right):
> 
>
http://codereview.chromium.org/10167017/diff/2001/sync/engine/sync_scheduler....
> sync/engine/sync_scheduler.h:170: bool Succeeded();
> On 2012/04/20 22:38:09, timsteele wrote:
> > I don't think this is part of this patch?
> 
> No.  I have no idea how this got included.

Here's the callback-based alternative.  

I should mention that there's another option.  The BackendMigrator has
infrastructure to allow other classes to observe its state changes.  This
functionality is currently used only for tests, and I didn't want to write the
first non-test client of this API.  However, I can try that approach if you
would prefer it over the callback.

[tim (not reviewing), 2012-04-21 00:37:35]

On 2012/04/21 00:26:27, rlarocque wrote:
> On 2012/04/20 22:45:08, rlarocque wrote:
> >
>
http://codereview.chromium.org/10167017/diff/2001/chrome/browser/sync/profile...
> > File chrome/browser/sync/profile_sync_service.h (right):
> > 
> >
>
http://codereview.chromium.org/10167017/diff/2001/chrome/browser/sync/profile...
> > chrome/browser/sync/profile_sync_service.h:300: void
StartSyncingWithServer();
> > On 2012/04/20 22:38:09, timsteele wrote:
> > > We can't expose this through here. Any old code in chrome/browser should
not
> > > concern itself with, or worse,  depend on / tinker with this most
> fundamental
> > of
> > > actions in sync. Can we hand the BackendMigrator a callback instead?
> > 
> > I was struggling to find the least bad way to do this.  I never thought of
> using
> > a callback as a means of information hiding.  I'll give it a shot.
> > 
> >
http://codereview.chromium.org/10167017/diff/2001/sync/engine/sync_scheduler.h
> > File sync/engine/sync_scheduler.h (right):
> > 
> >
>
http://codereview.chromium.org/10167017/diff/2001/sync/engine/sync_scheduler....
> > sync/engine/sync_scheduler.h:170: bool Succeeded();
> > On 2012/04/20 22:38:09, timsteele wrote:
> > > I don't think this is part of this patch?
> > 
> > No.  I have no idea how this got included.
> 
> Here's the callback-based alternative.  
> 
> I should mention that there's another option.  The BackendMigrator has
> infrastructure to allow other classes to observe its state changes.  This
> functionality is currently used only for tests, and I didn't want to write the
> first non-test client of this API.  However, I can try that approach if you
> would prefer it over the callback.

I think I like this for it's straightforwardness and futureproofness.  I'd worry
about having to track the previous migration state or cases where we call
OnMigrationStateChanged / IDLE and we may not want to respond in the same way.
Maybe not a big deal, but the current approach seems pretty reasonable to me. 

LGTM

[commit-bot: I haz the power, 2012-04-23 18:21:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10167017/8001

[commit-bot: I haz the power, 2012-04-23 19:44:13]

Change committed as 133498