[Sync] Remove CleanupDisabledTypes command and move purge logic into SyncManager.

[Sync] Remove CleanupDisabledTypes command and move purge logic into SyncManager.

We were only ever performing a meaningful cleanup on reconfigurations
or restart, so we make that explicit by purging from within the SyncManager's
loading and configuration methods.

BUG=131433, 90868
TEST=manual

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

[Nicolas Zea, 2012-06-08 20:16:20]

PTAL

[Nicolas Zea, 2012-06-11 23:30:59]

Directory changes moved into syncer, PTAL

[Nicolas Zea, 2012-07-18 17:49:01]

PTAL. The cleanup is now down in the two logical places within the sync manager.
Once you land your sync manager unittest refactor I can add some tests that
double check this. For now I verified manually.

[tim (not reviewing), 2012-07-19 22:05:59]

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

http://codereview.chromium.org/10541079/diff/30002/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:1132: }  // namespace browser_sync
huh?

http://codereview.chromium.org/10541079/diff/30002/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10541079/diff/30002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:841: Difference(
nit: I'd move Difference( up one line

http://codereview.chromium.org/10541079/diff/30002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:847:
data_->directory()->PurgeEntriesWithTypeIn(disabled_types);
We should probably bail / callback / use a bigger nuke if this returns false?  

Also, can you merge this code with CleanupDisabledTypes (and call it here)?  I
think the only diff is using ModelTypeSet::All vs ctxt_routing_info.

[Nicolas Zea, 2012-07-20 22:07:01]

PTAL. Also, added some tests in sync_api_unittest.cc

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

http://codereview.chromium.org/10541079/diff/30002/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_unittest.cc:1132: }  // namespace browser_sync
On 2012/07/19 22:05:59, timsteele wrote:
> huh?  

Done.

http://codereview.chromium.org/10541079/diff/30002/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10541079/diff/30002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:841: Difference(
On 2012/07/19 22:05:59, timsteele wrote:
> nit: I'd move Difference( up one line

Done.

http://codereview.chromium.org/10541079/diff/30002/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:847:
data_->directory()->PurgeEntriesWithTypeIn(disabled_types);
On 2012/07/19 22:05:59, timsteele wrote:
> We should probably bail / callback / use a bigger nuke if this returns false? 

> 
> Also, can you merge this code with CleanupDisabledTypes (and call it here)?  I
> think the only diff is using ModelTypeSet::All vs ctxt_routing_info.

Done.

[Nicolas Zea, 2012-07-24 22:51:42]

ping!

[tim (not reviewing), 2012-07-26 21:39:45]

One test comment, and also update CL description since you're really moving
purge logic into SyncManager, not syncer.

http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:483: !CleanupDisabledTypes(
nit - lets use the same verb (Purge/Cleanup) here, my vote goes to purge due to
the underlying API.

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

http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:2554: // should be purged (but only the
types that were just disabled).
I don't see a test that covers a case where both partial and disabled types need
to be purged.  Might be useful?  (Or maybe I missed it).

[Nicolas Zea, 2012-07-26 23:29:59]

PTAL

http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:483: !CleanupDisabledTypes(
On 2012/07/26 21:39:45, timsteele wrote:
> nit - lets use the same verb (Purge/Cleanup) here, my vote goes to purge due
to
> the underlying API.

Done.

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

http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/syncapi_...
sync/internal_api/syncapi_unittest.cc:2554: // should be purged (but only the
types that were just disabled).
On 2012/07/26 21:39:45, timsteele wrote:
> I don't see a test that covers a case where both partial and disabled types
need
> to be purged.  Might be useful?  (Or maybe I missed it).

The purging of both at startup is actually going away once Richard's refactor to
make the registrar aware of loaded data lands (it'll happens as part of the
first configuresyncer call). I added a TODO in the SyncManagerImpl to make that
clear.

That said, went ahead and added verification that partial types aren't purged
unless they're disabled, and updated the tests to also work once Richard's patch
lands.

[tim (not reviewing), 2012-07-27 17:15:30]

On 2012/07/26 23:29:59, nzea wrote:
> PTAL
> 
>
http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/sync_man...
> File sync/internal_api/sync_manager_impl.cc (right):
> 
>
http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/sync_man...
> sync/internal_api/sync_manager_impl.cc:483: !CleanupDisabledTypes(
> On 2012/07/26 21:39:45, timsteele wrote:
> > nit - lets use the same verb (Purge/Cleanup) here, my vote goes to purge due
> to
> > the underlying API.
> 
> Done.
> 
>
http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/syncapi_...
> File sync/internal_api/syncapi_unittest.cc (right):
> 
>
http://codereview.chromium.org/10541079/diff/22015/sync/internal_api/syncapi_...
> sync/internal_api/syncapi_unittest.cc:2554: // should be purged (but only the
> types that were just disabled).
> On 2012/07/26 21:39:45, timsteele wrote:
> > I don't see a test that covers a case where both partial and disabled types
> need
> > to be purged.  Might be useful?  (Or maybe I missed it).
> 
> The purging of both at startup is actually going away once Richard's refactor
to
> make the registrar aware of loaded data lands (it'll happens as part of the
> first configuresyncer call). I added a TODO in the SyncManagerImpl to make
that
> clear.
> 
> That said, went ahead and added verification that partial types aren't purged
> unless they're disabled, and updated the tests to also work once Richard's
patch
> lands.

LGTM!

[commit-bot: I haz the power, 2012-07-27 17:36:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10541079/52001

[commit-bot: I haz the power, 2012-07-27 20:37:56]

Change committed as 148792