[Sync] Make BackendMigrator not wait for full sync cycles

[Sync] Make BackendMigrator not wait for full sync cycles

Make sure that scheduling data type cleanup before scheduling start
implies that the cleanup happes before the start.

Make BackendMigrator simply wait for configuration to be done,
since data type cleanup is done before configuration.

Make BackendMigrator always preempt any existing migration.

Make DataTypeManagerImpl configuration handle being preempted
with enable_nigori set (since BackendMigrator can now do that).

Make DataTypeManagerImpl check for pending reconfigure on DownloadReady,
ignoring download success if so (since a migration may cause a download
failure, now that we don't wait for a full sync cycle).

Re-enable and rewrite failing MigrationCycle tests.

Fix subtle bug in SyncBackendHost where types_to_remove is used
where types_to_remove_with_nigori should be used instead.

Make syncer_end_command propagate up all download progress markers,
as that is needed by migration tests.

Add integration test methods for migration.

BUG=92928
TEST=

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

[akalin, 2011-08-19 00:14:22]

+tim/lingesh for review

Also, opinions on whether this fix (if LGTMed) is safe to merge to M14?

[akalin, 2011-08-19 01:34:27]

http://codereview.chromium.org/7655055/diff/1011/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/7655055/diff/1011/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:265: // Ignore |success| if
we need to reconfigure anyway.
I'm not sure if it is completely safe to ignore the return value.  This only
matters for migration where the getupdates for configure returns a migration
error, which will kick off another config for migration.

Perhaps pass around a for_migration bool to Configure etc.?

http://codereview.chromium.org/7655055/diff/1011/chrome/test/live_sync/migrat...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7655055/diff/1011/chrome/test/live_sync/migrat...
chrome/test/live_sync/migration_errors_test.cc:87:
ASSERT_TRUE(GetClient(0)->AwaitSyncCycleCompletion("Migration"));
TODO: wait for configure to be done instead of a full cycle

http://codereview.chromium.org/7655055/diff/1011/chrome/test/live_sync/migrat...
chrome/test/live_sync/migration_errors_test.cc:119:
ASSERT_TRUE(GetClient(0)->AwaitSyncCycleCompletion("Migration"));
Here, too

[tim (not reviewing), 2011-08-22 14:44:06]

Adding Nicolas for DataTypeManagerImpl changes.

[tim (not reviewing), 2011-08-22 15:08:49]

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
File chrome/browser/sync/backend_migrator.cc (left):

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.cc:104: if (num_empty_migrated_markers <
to_migrate_.size())
Is there a way we can still have this check?  I know it's not needed in the
happy case, but attempting to re-sync a datatype that hasn't been purged is
quite bad and leaves the whole account in an unspecified state...  Just before
configuring to re-enable, it would be good to make sure the clear took place. 
It's better to "do nothing" than try and reconfigure.

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
File chrome/browser/sync/backend_migrator.cc (right):

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.cc:40: using std::swap;
nit - I'd just specify std::swap inline since you only use it once...

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.cc:55: if (state_ == IDLE) {
nit - file uses no-braces-on-single-line-if

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.cc:60: TryStart();
Would probably be simpler (here and below) if TryStart returned a bool. Makes
sense too given that it's named 'TryStart'.

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
File chrome/browser/sync/backend_migrator.h (right):

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.h:49: void TryStart();
This comment should specify what the start conditions are.

[Nicolas Zea, 2011-08-22 18:43:56]

DTM changes LGTM

[akalin, 2011-08-24 22:50:17]

All comments addressed, PTAL

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
File chrome/browser/sync/backend_migrator.cc (right):

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.cc:40: using std::swap;
On 2011/08/22 15:08:49, timsteele wrote:
> nit - I'd just specify std::swap inline since you only use it once...

Done.

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.cc:55: if (state_ == IDLE) {
On 2011/08/22 15:08:49, timsteele wrote:
> nit - file uses no-braces-on-single-line-if

Done.

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.cc:60: TryStart();
On 2011/08/22 15:08:49, timsteele wrote:
> Would probably be simpler (here and below) if TryStart returned a bool. Makes
> sense too given that it's named 'TryStart'.

Done.

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
File chrome/browser/sync/backend_migrator.h (right):

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.h:49: void TryStart();
On 2011/08/22 15:08:49, timsteele wrote:
> This comment should specify what the start conditions are.  

Done.

[akalin, 2011-08-24 22:55:01]

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
File chrome/browser/sync/backend_migrator.cc (left):

http://codereview.chromium.org/7655055/diff/5001/chrome/browser/sync/backend_...
chrome/browser/sync/backend_migrator.cc:104: if (num_empty_migrated_markers <
to_migrate_.size())
On 2011/08/22 15:08:49, timsteele wrote:
> Is there a way we can still have this check?  I know it's not needed in the
> happy case, but attempting to re-sync a datatype that hasn't been purged is
> quite bad and leaves the whole account in an unspecified state...  Just before
> configuring to re-enable, it would be good to make sure the clear took place. 
> It's better to "do nothing" than try and reconfigure.

Done.

[lipalani1, 2011-08-24 23:26:29]

LGTM from my side!!(I only reviewed the logic not the style). Looks like pretty
much all edge cases were covered!!

[akalin, 2011-08-24 23:43:45]

On 2011/08/24 23:26:29, lipalani1 wrote:
> LGTM from my side!!(I only reviewed the logic not the style). Looks like
pretty
> much all edge cases were covered!!

Looking at linux_sync failures (they look legit)...

[akalin, 2011-08-25 03:13:33]

On 2011/08/24 23:43:45, akalin wrote:
> On 2011/08/24 23:26:29, lipalani1 wrote:
> > LGTM from my side!!(I only reviewed the logic not the style). Looks like
> pretty
> > much all edge cases were covered!!
> 
> Looking at linux_sync failures (they look legit)...

Ugh, of course I can't check the progress markers, since there's no sync cycle
to update it.  I'll figure out a way to do an equivalent check.

[akalin, 2011-08-26 00:23:33]

I added a method to check the progress markers directly.  This ended up
revealing a subtle race condition; basically, running
ScheduleCleanupDisableTypes before StartConfiguration doesn't mean that disabled
types cleanup runs before the latter.

Please take another look.

[tim (not reviewing), 2011-08-26 12:41:40]

Few more comments.  Hang in there...

Would be good for Lingesh to peek at the new on_success callback in
SyncScheduler.

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

http://codereview.chromium.org/7655055/diff/22025/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.h:86: // triggered the nudge.
So, one of the "future todos / open questions" in the doc for SyncerThread2 was
about connecting nudges back to the originating sites. But with coalescing, that
gets pretty tricky, and I'm not convinced it's worth the complexity.  There are
some cases it would help, e.g. with ClearUserData, but for straight nudges, I
don't know.

Instead of "TODO: add Foo as needed" can you be more specific with TODOs on the
individual functions and/or file a bug?

http://codereview.chromium.org/7655055/diff/22025/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.h:99: void
ScheduleCleanupDisabledTypes(const base::Closure& callback);
Please comment the new parameter.

http://codereview.chromium.org/7655055/diff/22025/chrome/browser/sync/engine/...
chrome/browser/sync/engine/sync_scheduler.h:172: base::Closure on_success;
I like this.  I'm still wrapping my head around what the outcome is with all the
coalescing we do, but it seems valuable in general.

http://codereview.chromium.org/7655055/diff/22025/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/7655055/diff/22025/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:288: void
SyncBackendHost::StartConfiguration(bool cleanup_disabled_types) {
I'll point out the bools vs enum style guide at
http://dev.chromium.org/developers/coding-style#Code_Formatting

I'll let you make the call.

http://codereview.chromium.org/7655055/diff/22025/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:376:
trans.GetLookup()->GetDownloadProgress(type, &progress_marker);
I like the GetDownloadProgress approach, but want to make sure there isn't a way
we can just let the initial snapshot (sent by
SyncScheduler::SendInitialSnapshot) bubble up so we can grab this without having
to add any accessors?  The point of that snapshot was intended to be a "here's
the initial state of the world", but I forget why we don't let it bubble up
here.  There be a valid / twisted reason.

If we do keep GetDownloadProgress, the only thing I'd like to change is this
conversion to a ModelTypeSet, since technically,  an empty progress marker does
not imply purged... it could simply be that the type wasn't ever enabled.  
Seems like we should just pass the sync_pb::DataTypeProgressMarker, but I could
be missing something.  Basically I don't like introducing the "purged types"
concept here (at least, at the SBH layer).

[akalin, 2011-08-26 17:19:18]

http://codereview.chromium.org/7655055/diff/22025/chrome/browser/sync/glue/sy...
> chrome/browser/sync/glue/sync_backend_host.cc:376:
> trans.GetLookup()->GetDownloadProgress(type, &progress_marker);
> I like the GetDownloadProgress approach, but want to make sure there
> isn't a way we can just let the initial snapshot (sent by
> SyncScheduler::SendInitialSnapshot) bubble up so we can grab this
> without having to add any accessors?  The point of that snapshot was
> intended to be a "here's the initial state of the world", but I forget
> why we don't let it bubble up here.  There be a valid / twisted reason.

Well, the problem is that any scheduled disabled-types-cleanup job may
be run (via DoPendingJob) only after StartImpl() is run, even if it
was scheduled before StartImpl().  So any initial snapshot would be
updated.

> If we do keep GetDownloadProgress, the only thing I'd like to change is
> this conversion to a ModelTypeSet, since technically,  an empty progress
> marker does not imply purged... it could simply be that the type wasn't
> ever enabled.   Seems like we should just pass the
> sync_pb::DataTypeProgressMarker, but I could be missing something.
> Basically I don't like introducing the "purged types" concept here (at
> least, at the SBH layer).

I don't like it much, either.  I had to stick it in SBH because it's
the only object that the DataTypeManagerImpl knows about.  Having DTMI
know about purged types isn't great either.

I could rename it to "GetDatalessDataTypes" or something, if you don't
like the name "purged".  But I don't see that changing it to a map
from type -> DataTypeProgressMarker would gain much; all the proto has
is a list of opaque tokens, and all we can deduce about it is from
whether the token is empty.

It's worth noting that all this complexity is because we treat the
cleanup-disabled-types process like a job.  My previous
implementation, where CleanupDisabledTypesImpl simply created a
CleanupDisabledTypesCommand and executed it, would make this
completely unnecessary, since we could just schedule it before the
Start() and know that it would be ordered before StartImpl().

What do you think?

[lipalani1, 2011-08-26 21:34:03]

Fred - I am trying to understand the need for callback(although I agree with
having the callback it looks like they dont give you any functionality here.) If
you schedule cleanupdisabled types before start it has to execute before
startimpl.
because cleanupdisabled type is a one off job we never save it in the
pending_job variable. We could even add a DCHECK in DoPendingJob to that effect.


This is also enforced in the following CL(i can check it in soon. Just got
distracted.):
http://codereview.chromium.org/7640011

But even without the above CL it is true but the CL formalizes it.
Am I missing something?
On 2011/08/26 17:19:18, akalin wrote:
>
http://codereview.chromium.org/7655055/diff/22025/chrome/browser/sync/glue/sy...
> > chrome/browser/sync/glue/sync_backend_host.cc:376:
> > trans.GetLookup()->GetDownloadProgress(type, &progress_marker);
> > I like the GetDownloadProgress approach, but want to make sure there
> > isn't a way we can just let the initial snapshot (sent by
> > SyncScheduler::SendInitialSnapshot) bubble up so we can grab this
> > without having to add any accessors?  The point of that snapshot was
> > intended to be a "here's the initial state of the world", but I forget
> > why we don't let it bubble up here.  There be a valid / twisted reason.
> 
> Well, the problem is that any scheduled disabled-types-cleanup job may
> be run (via DoPendingJob) only after StartImpl() is run, even if it
> was scheduled before StartImpl().  So any initial snapshot would be
> updated.
> 
> > If we do keep GetDownloadProgress, the only thing I'd like to change is
> > this conversion to a ModelTypeSet, since technically,  an empty
progress
> > marker does not imply purged... it could simply be that the type wasn't
> > ever enabled.   Seems like we should just pass the
> > sync_pb::DataTypeProgressMarker, but I could be missing something.
> > Basically I don't like introducing the "purged types" concept here (at
> > least, at the SBH layer).
> 
> I don't like it much, either.  I had to stick it in SBH because it's
> the only object that the DataTypeManagerImpl knows about.  Having DTMI
> know about purged types isn't great either.
> 
> I could rename it to "GetDatalessDataTypes" or something, if you don't
> like the name "purged".  But I don't see that changing it to a map
> from type -> DataTypeProgressMarker would gain much; all the proto has
> is a list of opaque tokens, and all we can deduce about it is from
> whether the token is empty.
> 
> It's worth noting that all this complexity is because we treat the
> cleanup-disabled-types process like a job.  My previous
> implementation, where CleanupDisabledTypesImpl simply created a
> CleanupDisabledTypesCommand and executed it, would make this
> completely unnecessary, since we could just schedule it before the
> Start() and know that it would be ordered before StartImpl().
> 
> What do you think?

[akalin, 2011-08-26 21:40:05]

On Sat, Aug 27, 2011 at 5:34 AM,  <lipalani@chromium.org> wrote:
> Fred - I am trying to understand the need for callback(although I agree with
> having the callback it looks like they dont give you any functionality
> here.) If
> you schedule cleanupdisabled types before start it has to execute before
> startimpl.
> because cleanupdisabled type is a one off job we never save it in the
> pending_job variable. We could even add a DCHECK in DoPendingJob to that
> effect.

Oh, I was mistaken, it's not run via DoPendingJob.  Here's the sequence:

1) ScheduleCleanupDisabledTypes posts ScheduleCleanupDisabledTypesImpl.
2) Start() posts StartImpl
3) ScheduleCleanupDisabledTypesImpl runs, and calls
ScheduleSyncSessionJob, which posts DoSyncSessionJob
4) StartImpl runs.
5) DoSyncSessionJob runs, which cleans up the disabled types

I guess one solution would be to remove the Impl function and call
ScheduleSyncSessionJob directly.  Although the other functions have
Impl equivalents.  Are they really needed?

[lipalani1, 2011-08-26 21:48:58]

ah.. it bites us. the impl functions were an artifact of multiple threads. I was
planning on cleaning up the impl functions at some point precisely to make sure
things like this dont happen!
The answer to your question impl functions are not needed. And more importantly
job ordering should be preserved so impl functions should go away. Feel free to
assign a bug to me. I guess we have 3 options:
1. go with the closure.
2. for cleanupdisabled types dont use an impl function and leave a todo for the
rest.
3. Get rid of the impl functions altogethere.

I kind of prefer either 1 or 2.
On 2011/08/26 21:40:05, akalin wrote:
> On Sat, Aug 27, 2011 at 5:34 AM,  <mailto:lipalani@chromium.org> wrote:
> > Fred - I am trying to understand the need for callback(although I agree with
> > having the callback it looks like they dont give you any functionality
> > here.) If
> > you schedule cleanupdisabled types before start it has to execute before
> > startimpl.
> > because cleanupdisabled type is a one off job we never save it in the
> > pending_job variable. We could even add a DCHECK in DoPendingJob to that
> > effect.
> 
> Oh, I was mistaken, it's not run via DoPendingJob.  Here's the sequence:
> 
> 1) ScheduleCleanupDisabledTypes posts ScheduleCleanupDisabledTypesImpl.
> 2) Start() posts StartImpl
> 3) ScheduleCleanupDisabledTypesImpl runs, and calls
> ScheduleSyncSessionJob, which posts DoSyncSessionJob
> 4) StartImpl runs.
> 5) DoSyncSessionJob runs, which cleans up the disabled types
> 
> I guess one solution would be to remove the Impl function and call
> ScheduleSyncSessionJob directly.  Although the other functions have
> Impl equivalents.  Are they really needed?

[akalin, 2011-08-26 21:55:28]

On Sat, Aug 27, 2011 at 5:48 AM,  <lipalani@chromium.org> wrote:
> ah.. it bites us. the impl functions were an artifact of multiple threads. I
> was
> planning on cleaning up the impl functions at some point precisely to make
> sure
> things like this dont happen!
> The answer to your question impl functions are not needed. And more
> importantly
> job ordering should be preserved so impl functions should go away. Feel free
> to
> assign a bug to me. I guess we have 3 options:
> 1. go with the closure.
> 2. for cleanupdisabled types dont use an impl function and leave a todo for
> the
> rest.
> 3. Get rid of the impl functions altogethere.

I think removing the impl functions is big enough that it should be in
another CL.  I'll file a bug and do #2.  That'll let me go back to the
previous simpler approach.  Thanks!

[akalin, 2011-08-27 00:55:29]

Okay, I reverted my changes to plumb the closure through, and instead made
ScheduleCleanupDisabledTypes directly schedule the job.  Tim, all the comments
you had were regarding the closure plumbing, so they're moot now.

Please take another look.

[akalin, 2011-08-27 06:57:48]

On 2011/08/27 00:55:29, akalin wrote:
> Okay, I reverted my changes to plumb the closure through, and instead made
> ScheduleCleanupDisabledTypes directly schedule the job.  Tim, all the comments
> you had were regarding the closure plumbing, so they're moot now.
> 
> Please take another look.

Derp, I forgot to put the purged data types check back in.  Hang on...

[akalin, 2011-08-31 06:54:12]

Okay, this is finally ready for another look.

Raghu: please take a look at the integration test changes
Nicolas: the DTM didn't change much, but you may want to quickly look at it
again
Tim/Lingesh: Everything else

[Nicolas Zea, 2011-08-31 17:34:27]

http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.h:100: bool last_enable_nigori_;
Is it possible to determine this based on the last_configure_reason_? We don't
currently include nigori in the requested_types, but I wonder if that would be
cleaner. This would also get rid of the need for the separate
ConfigureWithoutNigori method.

[akalin, 2011-08-31 21:15:50]

http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager_impl.h (right):

http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager_impl.h:100: bool last_enable_nigori_;
On 2011/08/31 17:34:27, nzea wrote:
> Is it possible to determine this based on the last_configure_reason_? We don't
> currently include nigori in the requested_types, but I wonder if that would be
> cleaner. This would also get rid of the need for the separate
> ConfigureWithoutNigori method.

I don't think so.  A false enable_nigori flag implies last_configure_reason_ ==
migration, but not vice versa.

I tried removing the ConfigureWithoutNigori function before, but the problem is
that you'd have to change all the calls of Configure() to inject nigori, and
really only SyncBackendHost and lower layers should care about nigori. 
Migration is really the only thing that may need to Configure without nigori,
which is why that method was added. :(

[Nicolas Zea, 2011-08-31 21:35:43]

Bleh. Alright, LGTM.

[akalin, 2011-08-31 22:10:20]

On 2011/08/31 21:35:43, nzea wrote:
> Bleh. Alright, LGTM.

Tim/Raghu/Lingesh?

[lipalani, 2011-08-31 22:20:24]

i am reviewing now. will let you know by eod.

On Wed, Aug 31, 2011 at 3:10 PM, <akalin@chromium.org> wrote:

> On 2011/08/31 21:35:43, nzea wrote:
>
>> Bleh. Alright, LGTM.
>>
>
> Tim/Raghu/Lingesh?
>
>
>
http://codereview.chromium.**org/7655055/<http://codereview.chromium.org/7655...
>

[tim (not reviewing), 2011-08-31 23:18:01]

one comment, LGTM pending Lingesh's review.

http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_registrar.h (right):

http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_registrar.h:39: const std::string& name,
Name of what?

[akalin, 2011-08-31 23:56:28]

Talked with Lingesh offline.  His comments:

- Is it necessary for the DTM/SBH to handle the querying for unsynced types,
even though the BackendMigrator is the only one that needs it?

I think so, since backend migrator can't create a transaction of its own.

I should rename the var in DTM to all_unsynced_types and/or add a comment,
though.

- Really need PSS to listen to BackendMigrator events?  ConfigureDone should
notify observers.

Good question, I'll investigate.  I may have needed it in a previous version,
but maybe not anymore.

On 2011/08/31 23:18:01, timsteele wrote:
> one comment, LGTM pending Lingesh's review.
> 
>
http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/sy...
> File chrome/browser/sync/glue/sync_backend_registrar.h (right):
> 
>
http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/sy...
> chrome/browser/sync/glue/sync_backend_registrar.h:39: const std::string& name,
> Name of what?

[akalin, 2011-09-01 01:38:39]

http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_registrar.h (right):

http://codereview.chromium.org/7655055/diff/36117/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_registrar.h:39: const std::string& name,
On 2011/08/31 23:18:02, timsteele wrote:
> Name of what?

Added comments.

[akalin, 2011-09-01 01:40:32]

On 2011/08/31 23:56:28, akalin wrote:
> Talked with Lingesh offline.  His comments:
> 
> - Is it necessary for the DTM/SBH to handle the querying for unsynced types,
> even though the BackendMigrator is the only one that needs it?
> 
> I think so, since backend migrator can't create a transaction of its own.
> 
> I should rename the var in DTM to all_unsynced_types and/or add a comment,
> though.

Actually, I solved this by having the PSS pass in the UserShare to its migrator,
and the migrator can query the unsynced types itself.  That's much cleaner, so
did that and reverted my DTM/SBH changes.

> - Really need PSS to listen to BackendMigrator events?  ConfigureDone should
> notify observers.
> 
> Good question, I'll investigate.  I may have needed it in a previous version,
> but maybe not anymore.

Yes, this is still true.  The PSS calls NotifyObservers when it gets the
CONFIGURE_DONE event, but if it gets called before the BackendMigrator gets the
CONFIGURE_DONE event, then when the harness queries the BackendMigrator state,
it'll still think the migration is still going on and hang.  Sad. :(

[akalin, 2011-09-01 01:41:06]

Addressed all comments, please take another look.

Or if no one looks at it before they go to Vegas, I'll probably TBR it.

[tim (not reviewing), 2011-09-01 01:50:55]

This sure got complicated :\
You'll have to talk to Anna about re-running the full migration test suite,
since the previous version was cleared for takeoff, but this basically rewrites
the whole thing.

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/backend...
File chrome/browser/sync/backend_migrator.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/backend...
chrome/browser/sync/backend_migrator.cc:150: syncable::ModelTypeSet
GetUnsyncedDataTypes(sync_api::UserShare* user_share) {
Ugh :(  It's pretty unfortunate that we have to hook everything up directly to
syncapi here.  Especially stuff that isn't even in glue, since that's kind of
why we have glue to begin with :\

That and I've always thought it was weird that we need the UserShare at all and
wanted to get rid of it fully.. seems silly since there is only ever one of
them...

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:451:
migrator_->RemoveMigrationObserver(this);
hm, I thought we discussed just having the harness watch migration so it can
test it...  not sure why the service needs to do this

[lipalani, 2011-09-01 01:51:50]

Can you make the harness post a task to query the migrator.
On Aug 31, 2011 6:41 PM, <akalin@chromium.org> wrote:
> Addressed all comments, please take another look.
>
> Or if no one looks at it before they go to Vegas, I'll probably TBR it.
>
> http://codereview.chromium.org/7655055/

[akalin, 2011-09-01 02:15:07]

On 2011/09/01 01:51:50, lipalani wrote:
> Can you make the harness post a task to query the migrator.

What do you mean?  The harness only has access to the ProfileSyncService.  Do
you mean exposing an accessor for the migrator?

[akalin, 2011-09-01 02:19:39]

I'd like opinions on what I should do before I make more changes, as I'm getting
mixed messages.

I guess this just has to wait until next week. *sigh*

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/backend...
File chrome/browser/sync/backend_migrator.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/backend...
chrome/browser/sync/backend_migrator.cc:150: syncable::ModelTypeSet
GetUnsyncedDataTypes(sync_api::UserShare* user_share) {
On 2011/09/01 01:50:56, timsteele wrote:
> Ugh :(  It's pretty unfortunate that we have to hook everything up directly to
> syncapi here.  Especially stuff that isn't even in glue, since that's kind of
> why we have glue to begin with :\
> 
> That and I've always thought it was weird that we need the UserShare at all
and
> wanted to get rid of it fully.. seems silly since there is only ever one of
> them...
> 

Okay, so should I put this code back into SyncBackendHost?  I don't care which
method I do, but I'd like you guys to agree on which one. :)

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:451:
migrator_->RemoveMigrationObserver(this);
On 2011/09/01 01:50:56, timsteele wrote:
> hm, I thought we discussed just having the harness watch migration so it can
> test it...  not sure why the service needs to do this

Okay, I guess that requires plumbing the migrator up to PSS, or at least the
Add/RemoveMigrationObserver() methods.  I can do that.

[akalin, 2011-09-01 02:23:39]

On 2011/09/01 01:50:55, timsteele wrote:
> This sure got complicated :\
> You'll have to talk to Anna about re-running the full migration test suite,
> since the previous version was cleared for takeoff, but this basically
rewrites
> the whole thing.

I guess, although it's worth mentioning that the test suite didn't catch any of
the problems this CL solves.

[tim (not reviewing), 2011-09-01 02:36:22]

On 2011/09/01 02:23:39, akalin wrote:
> On 2011/09/01 01:50:55, timsteele wrote:
> > This sure got complicated :\
> > You'll have to talk to Anna about re-running the full migration test suite,
> > since the previous version was cleared for takeoff, but this basically
> rewrites
> > the whole thing.
> 
> I guess, although it's worth mentioning that the test suite didn't catch any
of
> the problems this CL solves.

True.  Though the fact that manual QA didn't catch any of these bugs would imply
they are not common, since her testing was pretty thorough. So it's important to
make sure we still have parity with the 95% correct case in addition to the
newly solid edge cases :)

I'm fine with the UserShare thing.  I don't think it's ideal, but it's something
we can tackle later.  I think that + looking into making the harness listen to
the migrator takes care of my comments!

[akalin, 2011-09-01 03:39:50]

Okay, addressed all comments.  Please take another look!

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/backend...
File chrome/browser/sync/backend_migrator.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/backend...
chrome/browser/sync/backend_migrator.cc:150: syncable::ModelTypeSet
GetUnsyncedDataTypes(sync_api::UserShare* user_share) {
On 2011/09/01 02:19:40, akalin wrote:
> On 2011/09/01 01:50:56, timsteele wrote:
> > Ugh :(  It's pretty unfortunate that we have to hook everything up directly
to
> > syncapi here.  Especially stuff that isn't even in glue, since that's kind
of
> > why we have glue to begin with :\
> > 
> > That and I've always thought it was weird that we need the UserShare at all
> and
> > wanted to get rid of it fully.. seems silly since there is only ever one of
> > them...
> > 
> 
> Okay, so should I put this code back into SyncBackendHost?  I don't care which
> method I do, but I'd like you guys to agree on which one. :)

Added a TODO in the header file to remove dep on user_share.

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:451:
migrator_->RemoveMigrationObserver(this);
On 2011/09/01 02:19:40, akalin wrote:
> On 2011/09/01 01:50:56, timsteele wrote:
> > hm, I thought we discussed just having the harness watch migration so it can
> > test it...  not sure why the service needs to do this
> 
> Okay, I guess that requires plumbing the migrator up to PSS, or at least the
> Add/RemoveMigrationObserver() methods.  I can do that.

Exposed migrator via GetBackendMigratorForTest().

[Raghu Simha, 2011-09-01 03:53:06]

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:227: //
|pending_migration_types_|.
Should we not simply replace pending_migration_types_ with
new_pending_migration_types?

For instance: What if pending_migration_types_ was originally {BOOKMARKS,
PREFERENCES}, and then, AUTOFILL was added to the list via
GetPendingMigrationTypesForTest(), but in the meantime, BOOKMARKS was already
migrated?

Will we not end up with {BOOKMARKS, PREFERENCES, AUTOFILL} when we should in
fact end up with {PREFERENCES, AUTOFILL}?

Or is something like the above not possible?

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:433: UpdateMigrationState();
Any particular reason for calling UpdateMigrationState() here, where it will be
run after every single state change? Would it not be sufficient to call it in
RunStateChangeMachine() only when we are in either
WAITING_FOR_MIGRATION_TO_START or WAITING_FOR_MIGRATION_TO_FINISH? (where it
will be run a much smaller percentage of the time)

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:727: // TODO(akalin):
Shouldn't this with the intersection check?
Is this comment Yoda-speak? :)

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:730: VLOG(1) <<
profile_debug_name_ << ": not synced, assuming doesn't match";
This should probably be a VLOG(2), since it's a genuine possibility that the
client is not yet fully synced, and we don't want to pollute the logs with
several reps of this statement. (Ignore this comment if you've run the
integration tests with --v=1 and feel that this is really useful while
debugging)

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:749: VLOG(1) << "non-empty
common types and "
Same as above. This too should probably be a VLOG(2).

http://codereview.chromium.org/7655055/diff/41012/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sync_test.h (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_test.h:151: void EnableNotifications();
Add a similar comment as the one above DisableNotifications. It might seem
repetitive, but it really helps when the comment shows up in intellisense.

http://codereview.chromium.org/7655055/diff/41012/chrome/test/live_sync/migra...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/test/live_sync/migra...
chrome/test/live_sync/migration_errors_test.cc:22: namespace {
Now that you're adding all these tests, does it does it make sense to rename it
to migration_test.cc? (Even if it does, it's probably best to do so in a
different CL so that the svn history is retained)

[akalin, 2011-09-01 04:03:23]

PTAL

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:227: //
|pending_migration_types_|.
On 2011/09/01 03:53:06, rsimha wrote:
> Should we not simply replace pending_migration_types_ with
> new_pending_migration_types?
> 
> For instance: What if pending_migration_types_ was originally {BOOKMARKS,
> PREFERENCES}, and then, AUTOFILL was added to the list via
> GetPendingMigrationTypesForTest(), but in the meantime, BOOKMARKS was already
> migrated?
> 
> Will we not end up with {BOOKMARKS, PREFERENCES, AUTOFILL} when we should in
> fact end up with {PREFERENCES, AUTOFILL}?
> 
> Or is something like the above not possible?

The idea is to accumulate all migrated types instead of just the one in the
current migration "session".  But in practice, all the pending types get
migrated at the same time (since the migrator waits until the configuration is
complete for all of them).

So in a nutshell, simply assigning would work, but I'm merging to be defensive,
since the intended end result is an accumulation.

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:433: UpdateMigrationState();
On 2011/09/01 03:53:06, rsimha wrote:
> Any particular reason for calling UpdateMigrationState() here, where it will
be
> run after every single state change? Would it not be sufficient to call it in
> RunStateChangeMachine() only when we are in either
> WAITING_FOR_MIGRATION_TO_START or WAITING_FOR_MIGRATION_TO_FINISH? (where it
> will be run a much smaller percentage of the time)

Addressed in latest patch.

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:727: // TODO(akalin):
Shouldn't this with the intersection check?
On 2011/09/01 03:53:06, rsimha wrote:
> Is this comment Yoda-speak? :)

Fixed!

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:730: VLOG(1) <<
profile_debug_name_ << ": not synced, assuming doesn't match";
On 2011/09/01 03:53:06, rsimha wrote:
> This should probably be a VLOG(2), since it's a genuine possibility that the
> client is not yet fully synced, and we don't want to pollute the logs with
> several reps of this statement. (Ignore this comment if you've run the
> integration tests with --v=1 and feel that this is really useful while
> debugging)

Done.

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:749: VLOG(1) << "non-empty
common types and "
On 2011/09/01 03:53:06, rsimha wrote:
> Same as above. This too should probably be a VLOG(2).

Done.

http://codereview.chromium.org/7655055/diff/41012/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sync_test.h (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_test.h:151: void EnableNotifications();
On 2011/09/01 03:53:06, rsimha wrote:
> Add a similar comment as the one above DisableNotifications. It might seem
> repetitive, but it really helps when the comment shows up in intellisense.

Done.

http://codereview.chromium.org/7655055/diff/41012/chrome/test/live_sync/migra...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/test/live_sync/migra...
chrome/test/live_sync/migration_errors_test.cc:22: namespace {
On 2011/09/01 03:53:06, rsimha wrote:
> Now that you're adding all these tests, does it does it make sense to rename
it
> to migration_test.cc? (Even if it does, it's probably best to do so in a
> different CL so that the svn history is retained)

Yeah, I agree.  Should be a different CL, though.  Added TODO.

[Raghu Simha, 2011-09-01 04:23:58]

The migration tests seem to have been beefed up really nicely in this patch. All
test changes LGTM pending green trybots.

For extra credit: If you have the time, I'd recommend running the migration
tests (and maybe a small handful of the other sync integration tests) locally
with --vmodule="profile_sync_service_harness={1,2}" to make sure the debug spew
is still readable, useful and looks correct.

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:227: //
|pending_migration_types_|.
On 2011/09/01 04:03:24, akalin wrote:
> The idea is to accumulate all migrated types instead of just the one in the
> current migration "session".  But in practice, all the pending types get
> migrated at the same time (since the migrator waits until the configuration is
> complete for all of them).
> 
> So in a nutshell, simply assigning would work, but I'm merging to be
defensive,
> since the intended end result is an accumulation.

SGTM.

http://codereview.chromium.org/7655055/diff/49001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7655055/diff/49001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:187: // (possible only
choosing data types).
nit: Perhaps s/only choosing/only while choosing/ ?

http://codereview.chromium.org/7655055/diff/49001/chrome/test/live_sync/migra...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7655055/diff/49001/chrome/test/live_sync/migra...
chrome/test/live_sync/migration_errors_test.cc:75: enum TriggerMethod {
MODIFY_PREF, MODIFY_BOOKMARK, TRIGGER_NOTIFICATION };
The sync test harness now lets you operate on all datatypes in a single test
file. In future CLs, you should be able to expand this to all datatypes, at
which point you might be able to use ModelType instead of this enum.

http://codereview.chromium.org/7655055/diff/52004/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7655055/diff/52004/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:188:
DCHECK(TryListeningToMigrationEvents());
DCHECKs don't get run on the main waterfall, since they use Release. Perhaps
make this a CHECK or LOG(ERROR) and return false if the call fails?

http://codereview.chromium.org/7655055/diff/52004/chrome/test/live_sync/migra...
File chrome/test/live_sync/migration_errors_test.cc (left):

http://codereview.chromium.org/7655055/diff/52004/chrome/test/live_sync/migra...
chrome/test/live_sync/migration_errors_test.cc:53: // 93167.
Looks like you've fixed 93167 with this patch? (if so, BUG+=93167)

[lipalani1, 2011-09-01 04:37:07]

This looks good. I think having usershare there is cleaner but it is possible my
opinion collides with Tim's!

LGTM from my side!
On 2011/09/01 04:23:58, rsimha wrote:
> The migration tests seem to have been beefed up really nicely in this patch.
All
> test changes LGTM pending green trybots.
> 
> For extra credit: If you have the time, I'd recommend running the migration
> tests (and maybe a small handful of the other sync integration tests) locally
> with --vmodule="profile_sync_service_harness={1,2}" to make sure the debug
spew
> is still readable, useful and looks correct.
> 
>
http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service_harness.cc (right):
> 
>
http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_harness.cc:227: //
> |pending_migration_types_|.
> On 2011/09/01 04:03:24, akalin wrote:
> > The idea is to accumulate all migrated types instead of just the one in the
> > current migration "session".  But in practice, all the pending types get
> > migrated at the same time (since the migrator waits until the configuration
is
> > complete for all of them).
> > 
> > So in a nutshell, simply assigning would work, but I'm merging to be
> defensive,
> > since the intended end result is an accumulation.
> 
> SGTM.
> 
>
http://codereview.chromium.org/7655055/diff/49001/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service_harness.cc (right):
> 
>
http://codereview.chromium.org/7655055/diff/49001/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_harness.cc:187: // (possible only
> choosing data types).
> nit: Perhaps s/only choosing/only while choosing/ ?
> 
>
http://codereview.chromium.org/7655055/diff/49001/chrome/test/live_sync/migra...
> File chrome/test/live_sync/migration_errors_test.cc (right):
> 
>
http://codereview.chromium.org/7655055/diff/49001/chrome/test/live_sync/migra...
> chrome/test/live_sync/migration_errors_test.cc:75: enum TriggerMethod {
> MODIFY_PREF, MODIFY_BOOKMARK, TRIGGER_NOTIFICATION };
> The sync test harness now lets you operate on all datatypes in a single test
> file. In future CLs, you should be able to expand this to all datatypes, at
> which point you might be able to use ModelType instead of this enum.
> 
>
http://codereview.chromium.org/7655055/diff/52004/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service_harness.cc (right):
> 
>
http://codereview.chromium.org/7655055/diff/52004/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_harness.cc:188:
> DCHECK(TryListeningToMigrationEvents());
> DCHECKs don't get run on the main waterfall, since they use Release. Perhaps
> make this a CHECK or LOG(ERROR) and return false if the call fails?
> 
>
http://codereview.chromium.org/7655055/diff/52004/chrome/test/live_sync/migra...
> File chrome/test/live_sync/migration_errors_test.cc (left):
> 
>
http://codereview.chromium.org/7655055/diff/52004/chrome/test/live_sync/migra...
> chrome/test/live_sync/migration_errors_test.cc:53: // 93167.
> Looks like you've fixed 93167 with this patch? (if so, BUG+=93167)

[tim (not reviewing), 2011-09-01 04:38:42]

LGTM

On Wed, Aug 31, 2011 at 9:37 PM, <lipalani@chromium.org> wrote:

> This looks good. I think having usershare there is cleaner but it is
> possible my
> opinion collides with Tim's!
>
> LGTM from my side!
>
> On 2011/09/01 04:23:58, rsimha wrote:
>
>> The migration tests seem to have been beefed up really nicely in this
>> patch.
>>
> All
>
>> test changes LGTM pending green trybots.
>>
>
>  For extra credit: If you have the time, I'd recommend running the
>> migration
>> tests (and maybe a small handful of the other sync integration tests)
>> locally
>> with --vmodule="profile_sync_**service_harness={1,2}" to make sure the
>> debug
>>
> spew
>
>> is still readable, useful and looks correct.
>>
>
>
> http://codereview.chromium.**org/7655055/diff/41012/chrome/**
>
browser/sync/profile_sync_**service_harness.cc<http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile_sync_service_harness.cc>
>
>> File chrome/browser/sync/profile_**sync_service_harness.cc (right):
>>
>
>
> http://codereview.chromium.**org/7655055/diff/41012/chrome/**
>
browser/sync/profile_sync_**service_harness.cc#newcode227<http://codereview.chromium.org/7655055/diff/41012/chrome/browser/sync/profile_sync_service_harness.cc#newcode227>
>
>> chrome/browser/sync/profile_**sync_service_harness.cc:227: //
>> |pending_migration_types_|.
>> On 2011/09/01 04:03:24, akalin wrote:
>> > The idea is to accumulate all migrated types instead of just the one in
>> the
>> > current migration "session".  But in practice, all the pending types get
>> > migrated at the same time (since the migrator waits until the
>> configuration
>>
> is
>
>> > complete for all of them).
>> >
>> > So in a nutshell, simply assigning would work, but I'm merging to be
>> defensive,
>> > since the intended end result is an accumulation.
>>
>
>  SGTM.
>>
>
>
> http://codereview.chromium.**org/7655055/diff/49001/chrome/**
>
browser/sync/profile_sync_**service_harness.cc<http://codereview.chromium.org/7655055/diff/49001/chrome/browser/sync/profile_sync_service_harness.cc>
>
>> File chrome/browser/sync/profile_**sync_service_harness.cc (right):
>>
>
>
> http://codereview.chromium.**org/7655055/diff/49001/chrome/**
>
browser/sync/profile_sync_**service_harness.cc#newcode187<http://codereview.chromium.org/7655055/diff/49001/chrome/browser/sync/profile_sync_service_harness.cc#newcode187>
>
>> chrome/browser/sync/profile_**sync_service_harness.cc:187: // (possible
>> only
>> choosing data types).
>> nit: Perhaps s/only choosing/only while choosing/ ?
>>
>
>
> http://codereview.chromium.**org/7655055/diff/49001/chrome/**
>
test/live_sync/migration_**errors_test.cc<http://codereview.chromium.org/7655055/diff/49001/chrome/test/live_sync/migration_errors_test.cc>
>
>> File chrome/test/live_sync/**migration_errors_test.cc (right):
>>
>
>
> http://codereview.chromium.**org/7655055/diff/49001/chrome/**
>
test/live_sync/migration_**errors_test.cc#newcode75<http://codereview.chromium.org/7655055/diff/49001/chrome/test/live_sync/migration_errors_test.cc#newcode75>
>
>> chrome/test/live_sync/**migration_errors_test.cc:75: enum TriggerMethod {
>> MODIFY_PREF, MODIFY_BOOKMARK, TRIGGER_NOTIFICATION };
>> The sync test harness now lets you operate on all datatypes in a single
>> test
>> file. In future CLs, you should be able to expand this to all datatypes,
>> at
>> which point you might be able to use ModelType instead of this enum.
>>
>
>
> http://codereview.chromium.**org/7655055/diff/52004/chrome/**
>
browser/sync/profile_sync_**service_harness.cc<http://codereview.chromium.org/7655055/diff/52004/chrome/browser/sync/profile_sync_service_harness.cc>
>
>> File chrome/browser/sync/profile_**sync_service_harness.cc (right):
>>
>
>
> http://codereview.chromium.**org/7655055/diff/52004/chrome/**
>
browser/sync/profile_sync_**service_harness.cc#newcode188<http://codereview.chromium.org/7655055/diff/52004/chrome/browser/sync/profile_sync_service_harness.cc#newcode188>
>
>> chrome/browser/sync/profile_**sync_service_harness.cc:188:
>> DCHECK(**TryListeningToMigrationEvents(**));
>> DCHECKs don't get run on the main waterfall, since they use Release.
>> Perhaps
>> make this a CHECK or LOG(ERROR) and return false if the call fails?
>>
>
>
> http://codereview.chromium.**org/7655055/diff/52004/chrome/**
>
test/live_sync/migration_**errors_test.cc<http://codereview.chromium.org/7655055/diff/52004/chrome/test/live_sync/migration_errors_test.cc>
>
>> File chrome/test/live_sync/**migration_errors_test.cc (left):
>>
>
>
> http://codereview.chromium.**org/7655055/diff/52004/chrome/**
>
test/live_sync/migration_**errors_test.cc#oldcode53<http://codereview.chromium.org/7655055/diff/52004/chrome/test/live_sync/migration_errors_test.cc#oldcode53>
>
>> chrome/test/live_sync/**migration_errors_test.cc:53: // 93167.
>> Looks like you've fixed 93167 with this patch? (if so, BUG+=93167)
>>
>
>
>
>
http://codereview.chromium.**org/7655055/<http://codereview.chromium.org/7655...
>

[akalin, 2011-09-01 04:39:05]

http://codereview.chromium.org/7655055/diff/49001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7655055/diff/49001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:187: // (possible only
choosing data types).
On 2011/09/01 04:23:58, rsimha wrote:
> nit: Perhaps s/only choosing/only while choosing/ ?

Done.

http://codereview.chromium.org/7655055/diff/49001/chrome/test/live_sync/migra...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7655055/diff/49001/chrome/test/live_sync/migra...
chrome/test/live_sync/migration_errors_test.cc:75: enum TriggerMethod {
MODIFY_PREF, MODIFY_BOOKMARK, TRIGGER_NOTIFICATION };
On 2011/09/01 04:23:58, rsimha wrote:
> The sync test harness now lets you operate on all datatypes in a single test
> file. In future CLs, you should be able to expand this to all datatypes, at
> which point you might be able to use ModelType instead of this enum.

Added TODO.  Still need an enum, though, to represent triggering via
notifications.

http://codereview.chromium.org/7655055/diff/52004/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/7655055/diff/52004/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:188:
DCHECK(TryListeningToMigrationEvents());
On 2011/09/01 04:23:59, rsimha wrote:
> DCHECKs don't get run on the main waterfall, since they use Release. Perhaps
> make this a CHECK or LOG(ERROR) and return false if the call fails?

Done.

http://codereview.chromium.org/7655055/diff/52004/chrome/test/live_sync/migra...
File chrome/test/live_sync/migration_errors_test.cc (left):

http://codereview.chromium.org/7655055/diff/52004/chrome/test/live_sync/migra...
chrome/test/live_sync/migration_errors_test.cc:53: // 93167.
On 2011/09/01 04:23:59, rsimha wrote:
> Looks like you've fixed 93167 with this patch? (if so, BUG+=93167)

Not quite, it's just that I don't have to work around it now.

[akalin, 2011-09-01 04:43:22]

On Wed, Aug 31, 2011 at 9:23 PM,  <rsimha@chromium.org> wrote:
> The migration tests seem to have been beefed up really nicely in this patch.
> All
> test changes LGTM pending green trybots.
>
> For extra credit: If you have the time, I'd recommend running the migration
> tests (and maybe a small handful of the other sync integration tests)
> locally
> with --vmodule="profile_sync_service_harness={1,2}" to make sure the debug
> spew
> is still readable, useful and looks correct.

I took a quick look at the final output and it seems reasonable.  I
did most of my debugging in this CL with the logging info, and I
added/fixed logging as I saw fit.

[akalin, 2011-09-01 04:44:47]

Landing this after trybots pass!

I will talk with Anna re. migration test pass tomorrow.

[commit-bot: I haz the power, 2011-09-01 07:42:21]

Presubmit check for 7655055-51002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
You might be calling functions intended only for testing from
production code. Please verify that the following usages are OK,
and email joi@chromium.org if you are seeing false positives:
  chrome/browser/sync/profile_sync_service_harness.cc:224
    service_->GetBackendMigratorForTest(); \
  chrome/browser/sync/profile_sync_service_harness.cc:429
    service()->GetBackendMigratorForTest()-> \
  chrome/browser/sync/profile_sync_service_harness.cc:430
    GetPendingMigrationTypesForTest(); \
  chrome/browser/sync/profile_sync_service_harness.cc:747
    service()->GetBackendMigratorForTest();

Presubmit checks took 5.7s to calculate.