SyncFS: Factor out DriveMetadataDB migration functions

SyncFS: Factor out DriveMetadataDB migration functions

This change factors out DriveMetadataDB V0-V1 migration functions into
metadata_db_migration_utils.{cc,h}, and also moves function to deprecate batch sync origins into the files.


BUG=none
TEST=unit_tests --gtest_filter=\*DriveMetadataDBMigrationUtilTest\*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203126

[nhiroki, 2013-05-29 08:25:08]

Can you review this? Thanks!

[calvinlo, 2013-05-29 08:36:51]

A question for Hiroki and Taiju, 
re:DeprecateBatchSyncOrigins test that I recently added. While it's not exactly
a DB migration in that the old keys are simply dropped instead of changed in
format, I feel our code would be cleaner if we still treated that as a schema
migration. Then we could move the cleanup code I left inside DriveMetadataStore
out of that file and ensure it's only ever used once and the never needed after
after the client DB migrates.

https://codereview.chromium.org/15951007/diff/4001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15951007/diff/4001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:32: bool
ParseV0FormatFileSystemURLString(const GURL& url,
nit:  possible to add a test for this too? It'll also double as an example of
what the input should look like and output should look like. Just looking at the
function it's hard to tell what goes in and what comes out.

https://codereview.chromium.org/15951007/diff/4001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15951007/diff/4001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_metadata_store.cc:56: std::string
RemovePrefix(const std::string& str, const std::string& prefix) {
Can we have a small test for this added?

[tzik, 2013-05-29 08:51:42]

lgtm

[nhiroki, 2013-05-29 08:51:52]

On 2013/05/29 08:36:51, calvinlo wrote:
> A question for Hiroki and Taiju, 
> re:DeprecateBatchSyncOrigins test that I recently added. While it's not
exactly
> a DB migration in that the old keys are simply dropped instead of changed in
> format, I feel our code would be cleaner if we still treated that as a schema
> migration. Then we could move the cleanup code I left inside
DriveMetadataStore
> out of that file and ensure it's only ever used once and the never needed
after
> after the client DB migrates.

SGTM. Probably we could move the cleanup code into the V1-V2 migration function
(or new V2-V3 function?). tzik-san, WDYT?

[tzik, 2013-05-29 09:15:21]

On 2013/05/29 08:51:52, nhiroki wrote:
> On 2013/05/29 08:36:51, calvinlo wrote:
> > A question for Hiroki and Taiju, 
> > re:DeprecateBatchSyncOrigins test that I recently added. While it's not
> exactly
> > a DB migration in that the old keys are simply dropped instead of changed in
> > format, I feel our code would be cleaner if we still treated that as a
schema
> > migration. Then we could move the cleanup code I left inside
> DriveMetadataStore
> > out of that file and ensure it's only ever used once and the never needed
> after
> > after the client DB migrates.
> 
> SGTM. Probably we could move the cleanup code into the V1-V2 migration
function
> (or new V2-V3 function?). tzik-san, WDYT?

Sounds good. Merging it to V1-V2 migration will be cleaner.

[nhiroki, 2013-05-30 06:15:47]

Can you take another look?

Addressed comments and moved the deprecating function into the V1-V2 migration
function.

https://codereview.chromium.org/15951007/diff/4001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc
(right):

https://codereview.chromium.org/15951007/diff/4001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive/metadata_db_migration_util.cc:32: bool
ParseV0FormatFileSystemURLString(const GURL& url,
On 2013/05/29 08:36:52, calvinlo wrote:
> nit:  possible to add a test for this too? It'll also double as an example of
> what the input should look like and output should look like. Just looking at
the
> function it's hard to tell what goes in and what comes out.

Done.

https://codereview.chromium.org/15951007/diff/4001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_metadata_store.cc (right):

https://codereview.chromium.org/15951007/diff/4001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_metadata_store.cc:56: std::string
RemovePrefix(const std::string& str, const std::string& prefix) {
On 2013/05/29 08:36:52, calvinlo wrote:
> Can we have a small test for this added?

I think we don't have to add test since this function is enough small and
straightforward. And probably tests for caller functions can cover this.

[calvinlo, 2013-05-30 06:31:03]

lgtm

[kinuko, 2013-05-30 06:31:46]

lgtm (just in case you want more lgtms)

[commit-bot: I haz the power, 2013-05-30 07:58:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/15951007/26001

[commit-bot: I haz the power, 2013-05-30 12:29:15]

Change committed as 203126