GDataDB support with leveldb.

GDataDB support with leveldb.

* Define GDataDB interface with methods to Put, Get and Delete. Also define a path-based iterator.
* GDataLevelDB implements GDataDB using leveldb. 
* Add methods SerializeToString and FromProtoString to serialize GDataEntry to strings and vice versa.
* GDataDBTests test Put, Get, Delete for files and directories.
* Iterator tests in GDataDBTests.
* GDataDBFactory class to create GDataLevelDB instance.

TODO:
* There is no integration with GDataRootDirectory/GDataFileSystem yet.

BUG=chromium-os:29232
TEST=unittests pass.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133815

[achuithb, 2012-04-21 08:14:56]

As noted in the description, I haven't attempted to integrate with
GDataFileSystem/GDataRootDirectory yet.

I think this is a good chunk of code to land before this patch gets ginormous.

[satorux1, 2012-04-23 17:40:41]

just took a quick look.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:19: class GDataDB {
class comment is missing.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:29: // Factory method to create a
GDataDB instance at |db_path|
period is missing.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:30: static scoped_ptr<GDataDB>
Create(const FilePath& db_path);
You might want to move this factory function out of this class, to make it this
class a pure interface class.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:54: class GDataDBIter {
class comment is missing.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:57: virtual scoped_ptr<GDataEntry>
GetNext() = 0;
put a blank line?

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
shouldn't this be gdata_leveldb_unittest?

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:68: const GDataFile* AsGDataFile()
const;
function overloading should be avoided. can you rename the existing functions to
be AsMutableGDataFile()? or name the new one like AsGDataFileConst()?

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:20: }
having this here looks weird. You might want to introduce gdata_db_factory.cc
and put  gdata::db_factory::Create() there.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:91:
leveldb::Slice(file->file_name()));
what if status1 succeeds but status2 fails? I thought we wanted to ensure that
the two tables to be synchronized.

Maybe we could merge the two tables into one and use the write batch feature as
suggested by kinuko? we should be able to achieve this by adding some prefixes
to the key:

id:<resource_id> for <resource_id> to <proto>
path:<path> for <path> to <resource_id>

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:172: if
(level_db_iter_->key().ToString().find(path_) != 0)
shouldn't this be a prefix match? Use StartsWith() in base/string_util.h?

[zel, 2012-04-23 18:44:51]

http://codereview.chromium.org/10168025/diff/13001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/13001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:66:
leveldb::Slice(file.file_name()),
file_name() is not unique, you should use GetFilePath() instead

http://codereview.chromium.org/10168025/diff/13001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:91:
leveldb::Slice(file->file_name()));
file_name() is not the path, you should use path here

[achuithb, 2012-04-24 08:09:36]

Believe I've addressed all comments. PTAL, Satoru-san and Zel.

http://codereview.chromium.org/10168025/diff/13001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/13001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:66:
leveldb::Slice(file.file_name()),
On 2012/04/23 18:44:52, zel wrote:
> file_name() is not unique, you should use GetFilePath() instead

Done.

http://codereview.chromium.org/10168025/diff/13001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:91:
leveldb::Slice(file->file_name()));
On 2012/04/23 18:44:52, zel wrote:
> file_name() is not the path, you should use path here

Done.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:29: // Factory method to create a
GDataDB instance at |db_path|
On 2012/04/23 17:40:41, satorux1 wrote:
> period is missing.

Done.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:30: static scoped_ptr<GDataDB>
Create(const FilePath& db_path);
On 2012/04/23 17:40:41, satorux1 wrote:
> You might want to move this factory function out of this class, to make it
this
> class a pure interface class. 

Done.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:54: class GDataDBIter {
On 2012/04/23 17:40:41, satorux1 wrote:
> class comment is missing.

Done.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db.h:57: virtual scoped_ptr<GDataEntry>
GetNext() = 0;
On 2012/04/23 17:40:41, satorux1 wrote:
> put a blank line?

Done.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/04/23 17:40:41, satorux1 wrote:
> shouldn't this be gdata_leveldb_unittest?

No, it tests GDataDB. I'm not including gdata_leveldb.h or using any
GDataLevelDB-specific code.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:68: const GDataFile* AsGDataFile()
const;
On 2012/04/23 17:40:41, satorux1 wrote:
> function overloading should be avoided. can you rename the existing functions
to
> be AsMutableGDataFile()? or name the new one like AsGDataFileConst()?

Done.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:20: }
On 2012/04/23 17:40:41, satorux1 wrote:
> having this here looks weird. You might want to introduce gdata_db_factory.cc
> and put  gdata::db_factory::Create() there.

Done.

http://codereview.chromium.org/10168025/diff/2003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:91:
leveldb::Slice(file->file_name()));
On 2012/04/23 17:40:41, satorux1 wrote:
> what if status1 succeeds but status2 fails? I thought we wanted to ensure that
> the two tables to be synchronized.
> 
> Maybe we could merge the two tables into one and use the write batch feature
as
> suggested by kinuko? we should be able to achieve this by adding some prefixes
> to the key:
> 
> id:<resource_id> for <resource_id> to <proto>
> path:<path> for <path> to <resource_id>

Done.

[achuithb, 2012-04-24 08:25:38]

Biggest changes are use of GDataEntry::GetFilePath instead of
GDataEntry::file_name, and use of batch write operations in level db (which
requires merging the two databases, and use of prefixes to disambiguate the 2
different types of keys).

[achuithb, 2012-04-24 10:05:06]

I've started some integration work, and the CL is getting bigger. 

I've moved FindEntryDelegate and friends to a separate file (I didn't want to
include gdata_file_system.h in gdata_files.cc). 

I've moved GDataFileSystem::UnsafeFindEntryByPath to
GDataRootDirectory::FindEntryByPath.

The companion CL with the changes to gdata_file_system.* and
file_entry_delegate.* is here: http://codereview.chromium.org/10204013/

I'll combine both CLs before I land.

[satorux1, 2012-04-24 17:51:06]

On 2012/04/24 10:05:06, achuith.bhandarkar wrote:
> I've started some integration work, and the CL is getting bigger. 
> 
> I've moved FindEntryDelegate and friends to a separate file (I didn't want to
> include gdata_file_system.h in gdata_files.cc). 
> 
> I've moved GDataFileSystem::UnsafeFindEntryByPath to
> GDataRootDirectory::FindEntryByPath.
> 
> The companion CL with the changes to gdata_file_system.* and
> file_entry_delegate.* is here: http://codereview.chromium.org/10204013/
> 
> I'll combine both CLs before I land.

Can you submit two patches separately? With git, it's easy. IIRC, you wanted to
migrate to git once zel did, and he did it already. :)

[satorux1, 2012-04-24 18:26:44]

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:25: DB_NOT_FOUND,
does this mean the db file is not found? or key is not found?

please add some in-line comment.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:26: DB_ERR,
DB_ERROR or DB_FAILED? in favor of less abbreviation.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:50: DISALLOW_COPY_AND_ASSIGN(GDataDB);
nit: In interface class with pure virtual methods, i think we don't put this as
we cannot copy/assign anyway.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:57: virtual scoped_ptr<GDataEntry>
GetNext() = 0;
function comment is missing. I expect it'll return NULL when it terminates.
please clarify in the comment.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_factory.h (right):

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_factory.h:18: class GDataDBFactory {
If all functions are static, we don't need a class. I'd create db_factory
namespace here instead.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...

Rather than creating classes only to group static member functions which do not
share static data, use namespaces instead.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_factory.h:21: };
if we are to use the class, we should have private: and
DISALLOW_COPY_AND_ASSIGN. namespace is more concise. :)

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:64: DCHECK(status.ok());
instead of DCHECK, shouldn't we return false?

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:68: GDataDB::Status
GDataLevelDB::Put(const GDataEntry& file) {
file -> entry

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:75:
batch.Put(gdata_slice.ResourceIdToSlice(file.resource_id()),
I found GDataSlice class to be rather complex. I guess we can simply do:

std::string ResourceIdToKey(const std::string& resource_id) {
  return kResourceIdPrefix + resource_id;
}

  const std::string resource_id_key = ResourceIdToKey(file.resource_id());
  batch.Put(leveldb::Slice(resource_id_key), ...);

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:87: DCHECK(status.ok());
remove DCHECK? I guess it can fail if the disk is full, which should not happen,
but assuming this always succeeds looks somewhat wrong.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:98: return DB_OK;
is it OK? shouldn't we propagate that the resource id was not found?

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:159: return DB_ERR;
we might want to have a function that converts leveldb::Status to our
GDataDB::Status. this can be useful elsewhere in this file.

[achuithb, 2012-04-24 19:43:58]

PTAL, Satoru-san.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:25: DB_NOT_FOUND,
On 2012/04/24 18:26:44, satorux1 wrote:
> does this mean the db file is not found? or key is not found?
> 
> please add some in-line comment.

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:26: DB_ERR,
On 2012/04/24 18:26:44, satorux1 wrote:
> DB_ERROR or DB_FAILED? in favor of less abbreviation.

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:50: DISALLOW_COPY_AND_ASSIGN(GDataDB);
On 2012/04/24 18:26:44, satorux1 wrote:
> nit: In interface class with pure virtual methods, i think we don't put this
as
> we cannot copy/assign anyway.

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:57: virtual scoped_ptr<GDataEntry>
GetNext() = 0;
On 2012/04/24 18:26:44, satorux1 wrote:
> function comment is missing. I expect it'll return NULL when it terminates.
> please clarify in the comment.

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_factory.h (right):

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_factory.h:18: class GDataDBFactory {
On 2012/04/24 18:26:44, satorux1 wrote:
> If all functions are static, we don't need a class. I'd create db_factory
> namespace here instead.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Nonmem...
> 
> Rather than creating classes only to group static member functions which do
not
> share static data, use namespaces instead.

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_factory.h:21: };
On 2012/04/24 18:26:44, satorux1 wrote:
> if we are to use the class, we should have private: and
> DISALLOW_COPY_AND_ASSIGN. namespace is more concise. :)

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:64: DCHECK(status.ok());
On 2012/04/24 18:26:44, satorux1 wrote:
> instead of DCHECK, shouldn't we return false?

I think we should handle the failure, ie,  try to recover corrupt db file, and
failing that, delete the file and fetch the feed from scratch. I'll add a TODO
here.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:68: GDataDB::Status
GDataLevelDB::Put(const GDataEntry& file) {
On 2012/04/24 18:26:44, satorux1 wrote:
> file -> entry

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:75:
batch.Put(gdata_slice.ResourceIdToSlice(file.resource_id()),
On 2012/04/24 18:26:44, satorux1 wrote:
> I found GDataSlice class to be rather complex. I guess we can simply do:
> 
> std::string ResourceIdToKey(const std::string& resource_id) {
>   return kResourceIdPrefix + resource_id;
> }
> 
>   const std::string resource_id_key = ResourceIdToKey(file.resource_id());
>   batch.Put(leveldb::Slice(resource_id_key), ...);

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:87: DCHECK(status.ok());
On 2012/04/24 18:26:44, satorux1 wrote:
> remove DCHECK? I guess it can fail if the disk is full, which should not
happen,
> but assuming this always succeeds looks somewhat wrong.

Done.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:98: return DB_OK;
On 2012/04/24 18:26:44, satorux1 wrote:
> is it OK? shouldn't we propagate that the resource id was not found?
 
I believe not-found is handled as ok by the underlying database, ie, it's not an
error. We're just mirroring that behavior here.

http://codereview.chromium.org/10168025/diff/33002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:159: return DB_ERR;
On 2012/04/24 18:26:44, satorux1 wrote:
> we might want to have a function that converts leveldb::Status to our
> GDataDB::Status. this can be useful elsewhere in this file.

Done.

[achuithb, 2012-04-24 20:08:13]

On 2012/04/24 17:51:06, satorux1 wrote:
> On 2012/04/24 10:05:06, achuith.bhandarkar wrote:
> > I've started some integration work, and the CL is getting bigger. 
> > 
> > I've moved FindEntryDelegate and friends to a separate file (I didn't want
to
> > include gdata_file_system.h in gdata_files.cc). 
> > 
> > I've moved GDataFileSystem::UnsafeFindEntryByPath to
> > GDataRootDirectory::FindEntryByPath.
> > 
> > The companion CL with the changes to gdata_file_system.* and
> > file_entry_delegate.* is here: http://codereview.chromium.org/10204013/
> > 
> > I'll combine both CLs before I land.
> 
> Can you submit two patches separately? With git, it's easy. IIRC, you wanted
to
> migrate to git once zel did, and he did it already. :)

Now I'm going to wait for beng to migrate ;)

I'll take a look at git. For now, I've reverted my find_file_delegate changes to
make this patch smaller. I'll land that stuff in the next patch.

[satorux1, 2012-04-24 20:25:15]

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:25: DB_NOT_FOUND,  // Key not found.
DB_KEY_NOT_FOUND to be more descriptive?

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:33: // Add |entry| to the database.
Add -> Adds

Puts may be better to be consistent with the function name.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:36: // Delete a database entry with key
|resource_id| or |path| respectively.
Deletes. please fix other places too.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:47: virtual scoped_ptr<GDataDBIter>
NewIterator(const FilePath& path) = 0;
maybe CreateIterator()? Create looks more common in chrome.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:50: GDataDirectory*
AddDir(GDataDirectory* parent);
nit: AddDirectory

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:60: int index_;
maybe sequence_id_? index_ sounded like an index of an array.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:148: uint32 file_paths_size)
{
uint32 -> size_t

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:151: for (uint32 i = 0; true;
++i) {
size_t. you can omit true.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:158: // TODO(achuith): Fix
this to check for entire path.
can we fix this now?

Maybe we can modify the iterator class to return the path as well as the entry
contents:

 virtual bool GetNext(std::string* path, scoped_ptr<GDataEntry>* entry)

[satorux1, 2012-04-24 20:25:15]

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:25: DB_NOT_FOUND,  // Key not found.
DB_KEY_NOT_FOUND to be more descriptive?

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:33: // Add |entry| to the database.
Add -> Adds

Puts may be better to be consistent with the function name.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:36: // Delete a database entry with key
|resource_id| or |path| respectively.
Deletes. please fix other places too.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:47: virtual scoped_ptr<GDataDBIter>
NewIterator(const FilePath& path) = 0;
maybe CreateIterator()? Create looks more common in chrome.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:50: GDataDirectory*
AddDir(GDataDirectory* parent);
nit: AddDirectory

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:60: int index_;
maybe sequence_id_? index_ sounded like an index of an array.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:148: uint32 file_paths_size)
{
uint32 -> size_t

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:151: for (uint32 i = 0; true;
++i) {
size_t. you can omit true.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:158: // TODO(achuith): Fix
this to check for entire path.
can we fix this now?

Maybe we can modify the iterator class to return the path as well as the entry
contents:

 virtual bool GetNext(std::string* path, scoped_ptr<GDataEntry>* entry)

[achuithb, 2012-04-24 21:36:25]

PTAL, Satoru-san.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:25: DB_NOT_FOUND,  // Key not found.
On 2012/04/24 20:25:15, satorux1 wrote:
> DB_KEY_NOT_FOUND to be more descriptive?

Done.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:33: // Add |entry| to the database.
On 2012/04/24 20:25:15, satorux1 wrote:
> Add -> Adds
> 
> Puts may be better to be consistent with the function name.

Done.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:36: // Delete a database entry with key
|resource_id| or |path| respectively.
On 2012/04/24 20:25:15, satorux1 wrote:
> Deletes. please fix other places too.

Done.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:47: virtual scoped_ptr<GDataDBIter>
NewIterator(const FilePath& path) = 0;
On 2012/04/24 20:25:15, satorux1 wrote:
> maybe CreateIterator()? Create looks more common in chrome.

Done.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:50: GDataDirectory*
AddDir(GDataDirectory* parent);
On 2012/04/24 20:25:15, satorux1 wrote:
> nit: AddDirectory

Done.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:60: int index_;
On 2012/04/24 20:25:15, satorux1 wrote:
> maybe sequence_id_? index_ sounded like an index of an array.

Done.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:148: uint32 file_paths_size)
{
On 2012/04/24 20:25:15, satorux1 wrote:
> uint32 -> size_t

Done.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:151: for (uint32 i = 0; true;
++i) {
On 2012/04/24 20:25:15, satorux1 wrote:
> size_t. you can omit true.

Done.

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:158: // TODO(achuith): Fix
this to check for entire path.
On 2012/04/24 20:25:15, satorux1 wrote:
> can we fix this now?
> 
> Maybe we can modify the iterator class to return the path as well as the entry
> contents:
> 
>  virtual bool GetNext(std::string* path, scoped_ptr<GDataEntry>* entry)

Done.

[satorux1, 2012-04-24 21:47:57]

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:46: // An iterator to fetch all
GDataEntry's under |path|.
nit: Creates an iterator.

btw, can this be NULL? might want to document about it.

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:103: GDataDirectory* dir1 =
AddDirectory(NULL);
rather than using a member variable, sequence_id_, it'd be clearer to pass the
sequence id, like:

int sequence_id = 1;
AddDirectory(NULL, sequence_id++);
AddDirectory(dir1, sequence_id++);

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:159: // TODO(achuith): Also
test entry->GetFilePath().
why cannot we do this now?

[achuithb, 2012-04-24 22:15:13]

PTAL!

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:46: // An iterator to fetch all
GDataEntry's under |path|.
On 2012/04/24 21:47:57, satorux1 wrote:
> nit: Creates an iterator.
> 
> btw, can this be NULL? might want to document about it.

It cannot be NULL, unless malloc fails. I don't know how to document this.
Should I write - Cannot be NULL?

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:103: GDataDirectory* dir1 =
AddDirectory(NULL);
On 2012/04/24 21:47:57, satorux1 wrote:
> rather than using a member variable, sequence_id_, it'd be clearer to pass the
> sequence id, like:
> 
> int sequence_id = 1;
> AddDirectory(NULL, sequence_id++);
> AddDirectory(dir1, sequence_id++);

Done.

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:159: // TODO(achuith): Also
test entry->GetFilePath().
On 2012/04/24 21:47:57, satorux1 wrote:
> why cannot we do this now?

GetFilePath() traverses the parent_ tree to construct the path, and assumes that
root_ and parent_ links are properly set up, which they are not here. This will
work once the integration into GDataRootDirectory/GDataFileSystem is complete.

[satorux1, 2012-04-24 22:34:50]

LGTM

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:46: // An iterator to fetch all
GDataEntry's under |path|.
On 2012/04/24 22:15:13, achuith.bhandarkar wrote:
> On 2012/04/24 21:47:57, satorux1 wrote:
> > nit: Creates an iterator.
> > 
> > btw, can this be NULL? might want to document about it.
> 
> It cannot be NULL, unless malloc fails. I don't know how to document this.
> Should I write - Cannot be NULL?

I'd say "Will not return NULL".

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:159: // TODO(achuith): Also
test entry->GetFilePath().
On 2012/04/24 22:15:13, achuith.bhandarkar wrote:
> On 2012/04/24 21:47:57, satorux1 wrote:
> > why cannot we do this now?
> 
> GetFilePath() traverses the parent_ tree to construct the path, and assumes
that
> root_ and parent_ links are properly set up, which they are not here. This
will
> work once the integration into GDataRootDirectory/GDataFileSystem is complete.

I see. you might want to move your comment here to the code. :)

[achuithb, 2012-04-24 23:47:20]

Thanks for the review, Satoru-san!

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db.h (right):

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db.h:46: // An iterator to fetch all
GDataEntry's under |path|.
On 2012/04/24 22:34:50, satorux1 wrote:
> On 2012/04/24 22:15:13, achuith.bhandarkar wrote:
> > On 2012/04/24 21:47:57, satorux1 wrote:
> > > nit: Creates an iterator.
> > > 
> > > btw, can this be NULL? might want to document about it.
> > 
> > It cannot be NULL, unless malloc fails. I don't know how to document this.
> > Should I write - Cannot be NULL?
> 
> I'd say "Will not return NULL".

Done.

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_db_unittest.cc (right):

http://codereview.chromium.org/10168025/diff/42013/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_db_unittest.cc:159: // TODO(achuith): Also
test entry->GetFilePath().
On 2012/04/24 22:34:50, satorux1 wrote:
> On 2012/04/24 22:15:13, achuith.bhandarkar wrote:
> > On 2012/04/24 21:47:57, satorux1 wrote:
> > > why cannot we do this now?
> > 
> > GetFilePath() traverses the parent_ tree to construct the path, and assumes
> that
> > root_ and parent_ links are properly set up, which they are not here. This
> will
> > work once the integration into GDataRootDirectory/GDataFileSystem is
complete.
> 
> I see. you might want to move your comment here to the code. :)

Done.

[zel, 2012-04-25 00:49:47]

lgtm

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:39: return
GDataDB::DB_CORRUPTION;
what do we plan to do on corruption detection? can you file a work item to deal
with this case? the safest approach would be to nuke the entire db set and
recreate it with the fresh feed

http://codereview.chromium.org/10168025/diff/35008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/35008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:17: const char kPathPrefix[] =
"path:";
why do you need these prefixes at all?

[achuithb, 2012-04-25 00:57:53]

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/15002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:39: return
GDataDB::DB_CORRUPTION;
On 2012/04/25 00:49:47, zel wrote:
> what do we plan to do on corruption detection? can you file a work item to
deal
> with this case? the safest approach would be to nuke the entire db set and
> recreate it with the fresh feed

I have a TODO below; need to open a bug.

I think leveldb has some corruption recovery tools - we should try those first,
and if they fail, blow away the db as you suggest, and allow it to be populated
from the server.

http://codereview.chromium.org/10168025/diff/35008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/35008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:17: const char kPathPrefix[] =
"path:";
On 2012/04/25 00:49:47, zel wrote:
> why do you need these prefixes at all?

We're storing paths and resource_ids in the same table (this allows us to batch
the writes for atomic updates). We use the prefix to disambiguate the 2 types of
keys.

[zel, 2012-04-25 01:18:33]

lgtm

http://codereview.chromium.org/10168025/diff/35008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/35008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:17: const char kPathPrefix[] =
"path:";
On 2012/04/25 00:57:53, achuith.bhandarkar wrote:
> On 2012/04/25 00:49:47, zel wrote:
> > why do you need these prefixes at all?
> 
> We're storing paths and resource_ids in the same table (this allows us to
batch
> the writes for atomic updates). We use the prefix to disambiguate the 2 types
of
> keys.

you should shorten them to r: and p: instead... also when you wire things with
GFS layer, drop the first part of the path because it's the same for everything
we store "gdata/"

[achuithb, 2012-04-25 01:20:26]

http://codereview.chromium.org/10168025/diff/35008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_leveldb.cc (right):

http://codereview.chromium.org/10168025/diff/35008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_leveldb.cc:17: const char kPathPrefix[] =
"path:";
On 2012/04/25 01:18:33, zel wrote:
> On 2012/04/25 00:57:53, achuith.bhandarkar wrote:
> > On 2012/04/25 00:49:47, zel wrote:
> > > why do you need these prefixes at all?
> > 
> > We're storing paths and resource_ids in the same table (this allows us to
> batch
> > the writes for atomic updates). We use the prefix to disambiguate the 2
types
> of
> > keys.
> 
> you should shorten them to r: and p: instead... also when you wire things with
> GFS layer, drop the first part of the path because it's the same for
everything
> we store "gdata/"

Since the key is generated with GDataEntry::GetFilePath, the /gdata/ portion is
already stripped out. The keys look like
p:dir1/dir2/file1