[sync] Purge directory data when migrating to USS

[sync] Purge directory data when migrating to USS

Make use of the existing PurgeEntriesWithTypeIn to purge the directory
of entries of the type we just migrated. Add a test that ensures this
happens.

R=skym@chromium.org

BUG=716156
Review-Url: https://codereview.chromium.org/2842373004
Cr-Commit-Position: refs/heads/master@{#468170}
Committed: https://chromium.googlesource.com/chromium/src/+/47c2232d3a2cbad52c1f38696f37ece1f0c8154c

[Patrick Noland, 2017-04-27 21:01:10]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 21:02:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 22:35:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 22:35:16]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-04-28 20:17:58]

Sky, PTAL

[skym, 2017-04-28 20:37:58]

lgtm

https://codereview.chromium.org/2842373004/diff/1/components/sync/engine_impl...
File components/sync/engine_impl/model_type_registry.cc (right):

https://codereview.chromium.org/2842373004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:126:
UMA_HISTOGRAM_ENUMERATION("Sync.USSMigrationSuccess", type,
These histograms are wrong. Please wrap with ModelTypeToHistogramInt().

https://codereview.chromium.org/2842373004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:137: if
(migration_completed_successfully) {
Can't you just move this into the success block?

https://codereview.chromium.org/2842373004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:142:
DCHECK(Intersection(GetEnabledDirectoryTypes(), GetEnabledNonBlockingTypes())
Is it reasonable to make this DCHECK on failure? Should the failure case early
return? If this is something that should be DCHECKing here, can you throw on a
comment why this should be maintained through failure?

[Patrick Noland, 2017-04-28 21:08:31]

The CQ bit was checked by pnoland@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 21:09:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2017-04-28 22:23:27]

https://codereview.chromium.org/2842373004/diff/1/components/sync/engine_impl...
File components/sync/engine_impl/model_type_registry.cc (right):

https://codereview.chromium.org/2842373004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:126:
UMA_HISTOGRAM_ENUMERATION("Sync.USSMigrationSuccess", type,
On 2017/04/28 20:37:58, skym wrote:
> These histograms are wrong. Please wrap with ModelTypeToHistogramInt().

Done.

https://codereview.chromium.org/2842373004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:137: if
(migration_completed_successfully) {
On 2017/04/28 20:37:58, skym wrote:
> Can't you just move this into the success block?

Done.

https://codereview.chromium.org/2842373004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:142:
DCHECK(Intersection(GetEnabledDirectoryTypes(), GetEnabledNonBlockingTypes())
On 2017/04/28 20:37:58, skym wrote:
> Is it reasonable to make this DCHECK on failure? Should the failure case early
> return? If this is something that should be DCHECKing here, can you throw on a
> comment why this should be maintained through failure?

If migration fails, our fallback is to do an initial GetUpdates for the type,
not to switch back to the directory implementation. So I think it still makes
sense to make sure that we haven't crossed wires and activated the directory
implementation by accident. I will add a comment to this effect.

[commit-bot: I haz the power, 2017-04-28 22:55:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 22:55:39]

Dry run: This issue passed the CQ dry run.

[Patrick Noland, 2017-04-28 22:56:20]

The CQ bit was checked by pnoland@chromium.org

[Patrick Noland, 2017-04-28 22:56:20]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2842373004/#ps20001
(title: "Fix histograms, add comment")

[commit-bot: I haz the power, 2017-04-28 22:56:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 23:06:43]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493420180395900,
"parent_rev": "f8f753f6d16087b221047b60ff600bb80fe9f3c1", "commit_rev":
"47c2232d3a2cbad52c1f38696f37ece1f0c8154c"}

[commit-bot: I haz the power, 2017-04-28 23:06:57]

Description was changed from

==========
[sync] Purge directory data when migrating to USS

Make use of the existing PurgeEntriesWithTypeIn to purge the directory
of entries of the type we just migrated. Add a test that ensures this
happens.

R=skym@chromium.org

BUG=716156
==========

to

==========
[sync] Purge directory data when migrating to USS

Make use of the existing PurgeEntriesWithTypeIn to purge the directory
of entries of the type we just migrated. Add a test that ensures this
happens.

R=skym@chromium.org

BUG=716156

Review-Url: https://codereview.chromium.org/2842373004
Cr-Commit-Position: refs/heads/master@{#468170}
Committed:
https://chromium.googlesource.com/chromium/src/+/47c2232d3a2cbad52c1f38696f37...
==========

[commit-bot: I haz the power, 2017-04-28 23:06:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/47c2232d3a2cbad52c1f38696f37...