sync: add CleanupDisabledTypesCommand to purge data pertaining to previously...

sync: add CleanupDisabledTypesCommand to purge data pertaining to previously
synced data types that the user has disabled.

Despite my attempt at simplifying the purge code in Directory[BackingStore], I had to go back to my first attempt with DeleteEntries (which was previously reviewed separately), with a few extra gotchas in Directory::PurgeEntriesWithTypeIn.

BUG=40252
TEST=CleanupDisabledTypesCommandTest, SyncableDirectoryTest, DirectoryBackingStoreTest


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

[tim (not reviewing), 2010-06-30 09:13:02]

Hey!

It turns out there are a few cases where my previous approach didn't work, and I
ended up circling back to something resembling the initial attempt. There are a
few more additions, such as taking care to unlink the nodes we're purging from
the ordering, which is necessary for the top level folders as they refer to each
other as siblings, and if you remove one you must update the other nodes (even
though they pertain to a different type).

[tim (not reviewing), 2010-06-30 16:31:28]

+steve for review as albert is sick.
+cc chron for perusal

please ask questions as necessary!
for reference:
http://codereview.chromium.org/2873020/show
http://codereview.chromium.org/2817019/show
http://codereview.chromium.org/2854018

[skrul, 2010-06-30 19:16:14]

LGTM

nice work :)

http://codereview.chromium.org/2865022/diff/9003/15005
File chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc
(right):

http://codereview.chromium.org/2865022/diff/9003/15005#newcode12
chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc:12:
#include "testing/gtest/include/gtest/gtest.h"
#include gmock

http://codereview.chromium.org/2865022/diff/9003/15008
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2865022/diff/9003/15008#newcode1731
chrome/browser/sync/engine/syncapi.cc:1731: 
is this space intentional?

http://codereview.chromium.org/2865022/diff/9003/15014
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/2865022/diff/9003/15014#newcode269
chrome/browser/sync/syncable/directory_backing_store.cc:269:
query.append(Int64ToString(*it));
fyi, sqlite does have a limitation on the size of a sql statement, looks like
chrome is using the default of 1GB.  We are probably safe :)

[tim (not reviewing), 2010-07-02 03:48:08]

Thanks for the review, sorry I took so long to reply back.
Planning on submitting tonight!

http://codereview.chromium.org/2865022/diff/9003/15005
File chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc
(right):

http://codereview.chromium.org/2865022/diff/9003/15005#newcode12
chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc:12:
#include "testing/gtest/include/gtest/gtest.h"
On 2010/06/30 19:16:15, skrul wrote:
> #include gmock

Done.

http://codereview.chromium.org/2865022/diff/9003/15008
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2865022/diff/9003/15008#newcode1731
chrome/browser/sync/engine/syncapi.cc:1731: 
On 2010/06/30 19:16:15, skrul wrote:
> is this space intentional?

nope. reverted file.

http://codereview.chromium.org/2865022/diff/9003/15014
File chrome/browser/sync/syncable/directory_backing_store.cc (right):

http://codereview.chromium.org/2865022/diff/9003/15014#newcode269
chrome/browser/sync/syncable/directory_backing_store.cc:269:
query.append(Int64ToString(*it));
On 2010/06/30 19:16:15, skrul wrote:
> fyi, sqlite does have a limitation on the size of a sql statement, looks like
> chrome is using the default of 1GB.  We are probably safe :)

Interesting :)

[chron, 2010-07-02 17:59:57]

I like this approach. SGTM!

On Thu, Jul 1, 2010 at 8:48 PM, <tim@chromium.org> wrote:

> Thanks for the review, sorry I took so long to reply back.
> Planning on submitting tonight!
>
>
>
> http://codereview.chromium.org/2865022/diff/9003/15005
> File
> chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc
> (right):
>
> http://codereview.chromium.org/2865022/diff/9003/15005#newcode12
> chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc:12:
> #include "testing/gtest/include/gtest/gtest.h"
> On 2010/06/30 19:16:15, skrul wrote:
>
>> #include gmock
>>
>
> Done.
>
>
> http://codereview.chromium.org/2865022/diff/9003/15008
> File chrome/browser/sync/engine/syncapi.cc (right):
>
> http://codereview.chromium.org/2865022/diff/9003/15008#newcode1731
> chrome/browser/sync/engine/syncapi.cc:1731:
> On 2010/06/30 19:16:15, skrul wrote:
>
>> is this space intentional?
>>
>
> nope. reverted file.
>
>
> http://codereview.chromium.org/2865022/diff/9003/15014
> File chrome/browser/sync/syncable/directory_backing_store.cc (right):
>
> http://codereview.chromium.org/2865022/diff/9003/15014#newcode269
> chrome/browser/sync/syncable/directory_backing_store.cc:269:
> query.append(Int64ToString(*it));
> On 2010/06/30 19:16:15, skrul wrote:
>
>> fyi, sqlite does have a limitation on the size of a sql statement,
>>
> looks like
>
>> chrome is using the default of 1GB.  We are probably safe :)
>>
>
> Interesting :)
>
>
> http://codereview.chromium.org/2865022/show
>