[SyncFS] Implement MetadataDatabase initialization

[SyncFS] Implement MetadataDatabase initialization

* Implement database load functions.
* Add accessors of the database contents.
* Add tests.

BUG=240165
NOTRY=true

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

[tzik, 2013-07-04 12:04:24]

PTL

[kinuko, 2013-07-04 15:59:11]

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:260:
SyncStatusCode MetadataDatabase::WriteInitialData(leveldb::DB* db) {
nit: could we name it WriteVersionInfo to make it as straightforward as
possible?

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:317:
LOG(WARNING) << "Failed to parse SyncServiceMetadata";
Use logger?

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:330:
LOG(WARNING) << "Failed to parse a Metadata";
ditto

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:434:
scoped_ptr<InitializeInfo> MetadataDatabase::InitializeOnWorker(
nit: OnWorker -> OnTaskRunner or something

(What Worker means is not clear)

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:441:
&info->created);
indent

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:470: const
SyncStatusCallback& callback) {
indent

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:42: struct
FileIDComparator {
can't these typenames be kept private?

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:63:
FileByParentAndTitle active_file_by_parent_and_title;
Is there a chance we could simplify some initialization code if we drop these
four fields and db, make ConstructDataStructure non-static, and directly
populate the MetadataDatabase's corresponding fields?

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:75: // This
function works asynchronously.
nit: its async-ness seems obvious from the signature

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:178: void
WriteToDB(scoped_ptr<leveldb::WriteBatch> batch,
Do you want to use Database or DB in methods names?  OpenDatabase uses Database.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:26:
bool AreEquivalentProtobuf(const Value& left, const Value& right) {
AreEquivalentProtobuf's' (for all AreEquivalentFoo methods)

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:104:
class SyncFS_MetadataDatabaseTest : public testing::Test {
I'm slightly not sure about this naming.

Does this test name conflict with others without SyncFS_?
SyncFS_DriveBackend_MetadataDatabaseTest?
SyncFS_DriveMetadataDatabaseTest?

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:233:
void VerifyReloadConsistency() {
Is it same if we save all internal maps of metadata_database_, call
metadata_database_->Initialize() again verify we get the same maps?

[tzik, 2013-07-05 07:42:28]

Updated!

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:260:
SyncStatusCode MetadataDatabase::WriteInitialData(leveldb::DB* db) {
On 2013/07/04 15:59:11, kinuko wrote:
> nit: could we name it WriteVersionInfo to make it as straightforward as
> possible?

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:317:
LOG(WARNING) << "Failed to parse SyncServiceMetadata";
On 2013/07/04 15:59:11, kinuko wrote:
> Use logger?

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:330:
LOG(WARNING) << "Failed to parse a Metadata";
On 2013/07/04 15:59:11, kinuko wrote:
> ditto

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:434:
scoped_ptr<InitializeInfo> MetadataDatabase::InitializeOnWorker(
On 2013/07/04 15:59:11, kinuko wrote:
> nit: OnWorker -> OnTaskRunner or something
> 
> (What Worker means is not clear)

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:441:
&info->created);
On 2013/07/04 15:59:11, kinuko wrote:
> indent

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:470: const
SyncStatusCallback& callback) {
On 2013/07/04 15:59:11, kinuko wrote:
> indent

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:42: struct
FileIDComparator {
On 2013/07/04 15:59:11, kinuko wrote:
> can't these typenames be kept private?

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:63:
FileByParentAndTitle active_file_by_parent_and_title;
On 2013/07/04 15:59:11, kinuko wrote:
> Is there a chance we could simplify some initialization code if we drop these
> four fields and db, make ConstructDataStructure non-static, and directly
> populate the MetadataDatabase's corresponding fields?

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:75: // This
function works asynchronously.
On 2013/07/04 15:59:11, kinuko wrote:
> nit: its async-ness seems obvious from the signature

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:178: void
WriteToDB(scoped_ptr<leveldb::WriteBatch> batch,
On 2013/07/04 15:59:11, kinuko wrote:
> Do you want to use Database or DB in methods names?  OpenDatabase uses
Database.

Changed to WriteToDatabase. I prefer using Database.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:26:
bool AreEquivalentProtobuf(const Value& left, const Value& right) {
On 2013/07/04 15:59:11, kinuko wrote:
> AreEquivalentProtobuf's' (for all AreEquivalentFoo methods)

Done.

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:104:
class SyncFS_MetadataDatabaseTest : public testing::Test {
On 2013/07/04 15:59:11, kinuko wrote:
> I'm slightly not sure about this naming.
> 
> Does this test name conflict with others without SyncFS_?
> SyncFS_DriveBackend_MetadataDatabaseTest?
> SyncFS_DriveMetadataDatabaseTest?

It doesn't conflict, but I feel a bit uneasy to put this to global namespace of
the test cases with the generic name.

[kinuko, 2013-07-08 04:18:56]

lgtm

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/18591004/diff/2001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:104:
class SyncFS_MetadataDatabaseTest : public testing::Test {
On 2013/07/05 07:42:28, tzik wrote:
> On 2013/07/04 15:59:11, kinuko wrote:
> > I'm slightly not sure about this naming.
> > 
> > Does this test name conflict with others without SyncFS_?
> > SyncFS_DriveBackend_MetadataDatabaseTest?
> > SyncFS_DriveMetadataDatabaseTest?
> 
> It doesn't conflict, but I feel a bit uneasy to put this to global namespace
of
> the test cases with the generic name.

We're already adding generic test names, if we really care about such cases
let's address them together in a separate bug.

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:176:
SyncStatusCode NormalizeDatabase(DatabaseContents* contents,
Does it make sense to split this into
InitializeServiceMetadata and
RemoveUnreachableEntries ?

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:181: //
Populate |file_by_file_id| and |files_by_parent|.
This comment is obsolete?

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:367:
base::FilePath::StringType concat;
nit: remove line 366-367?

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:386: void
MetadataDatabase::ConstructIndexes(DatabaseContents* contents) {
nit: I once suggested construct, but BuildPath, BuildIndexes might be simpler
(and more common)

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:116:
metadata.set_active(true);
nit: set_dirty(false) ?

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:154:
DriveFileMetadata CreateUnsupportedFile(const std::string& app_id,
nit: not used?

[nhiroki, 2013-07-08 05:14:07]

lgtm

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:116:
NOTIMPLEMENTED();
Can you add a TODO comment here?

[tzik, 2013-07-08 08:46:17]

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:116:
NOTIMPLEMENTED();
On 2013/07/08 05:14:07, nhiroki wrote:
> Can you add a TODO comment here?

Did you mean a TODO comment for what should be done in the migration?

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:176:
SyncStatusCode NormalizeDatabase(DatabaseContents* contents,
On 2013/07/08 04:18:56, kinuko wrote:
> Does it make sense to split this into
> InitializeServiceMetadata and
> RemoveUnreachableEntries ?

Done.

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:181: //
Populate |file_by_file_id| and |files_by_parent|.
On 2013/07/08 04:18:56, kinuko wrote:
> This comment is obsolete?

Done.

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:367:
base::FilePath::StringType concat;
On 2013/07/08 04:18:56, kinuko wrote:
> nit: remove line 366-367?

Done.

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:386: void
MetadataDatabase::ConstructIndexes(DatabaseContents* contents) {
On 2013/07/08 04:18:56, kinuko wrote:
> nit: I once suggested construct, but BuildPath, BuildIndexes might be simpler
> (and more common)

Done.

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:116:
metadata.set_active(true);
On 2013/07/08 04:18:56, kinuko wrote:
> nit: set_dirty(false) ?

Done.

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:154:
DriveFileMetadata CreateUnsupportedFile(const std::string& app_id,
On 2013/07/08 04:18:56, kinuko wrote:
> nit: not used?

Done.

[commit-bot: I haz the power, 2013-07-09 07:35:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/18591004/25006

[nhiroki, 2013-07-09 07:46:47]

still lgtm with nits

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/18591004/diff/17001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:116:
NOTIMPLEMENTED();
On 2013/07/08 08:46:17, tzik wrote:
> On 2013/07/08 05:14:07, nhiroki wrote:
> > Can you add a TODO comment here?
> 
> Did you mean a TODO comment for what should be done in the migration?
>  

Sorry for making you confused. Although I meant a comment about a bug link and
when you start to implement the migration routine, I think comments you added
are also ok.

https://codereview.chromium.org/18591004/diff/25006/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/18591004/diff/25006/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:120: //    
an enabled origin should be a active, dirtf and
s/dirtf/dirty/

https://codereview.chromium.org/18591004/diff/25006/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:124: //   *
Add a file for each file in previous version.
You mean "Add a file *metadata* for each file in previous version."?

[tzik, 2013-07-09 08:02:50]

https://codereview.chromium.org/18591004/diff/25006/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/18591004/diff/25006/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:120: //    
an enabled origin should be a active, dirtf and
On 2013/07/09 07:46:47, nhiroki wrote:
> s/dirtf/dirty/

Done.

https://codereview.chromium.org/18591004/diff/25006/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:124: //   *
Add a file for each file in previous version.
On 2013/07/09 07:46:47, nhiroki wrote:
> You mean "Add a file *metadata* for each file in previous version."?

Done.

[commit-bot: I haz the power, 2013-07-09 08:10:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/18591004/44001

[commit-bot: I haz the power, 2013-07-09 08:51:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/18591004/48001

[commit-bot: I haz the power, 2013-07-09 14:07:33]

Change committed as 210554