Database support for GDataDirectoryService.

Database support for GDataDirectoryService.

* Add methods InitFromDB and SaveToDB to GDataDirectoryService, along with other helper methods to save/load the directory service from level db instead of from a proto file.
* Add a wrapper class GDataDirectoryServiceDB to hold the level db, with methods Create, Save, Read, Truncate, etc. This object lives on the blocking thread.
* Add CreateDBParams to facilitate creation of GDataDirectoryServiceDB on the blocking thread.
* Add GDataDirectoryService::FromProtoString to create a GDataEntry from a string saved in the db.
* Add unit tests for the db methods.
* Move LoadRootFeedParams to gdata_params. Move typedefs before struct definitions.
* Add a timer to measure the time to restore the filesystem from db or proto. For hugefileman, it's 3000 msec for db, and 2500 for proto, with debug code on a z600. Measurements on device TBD.

BUG=127856
TEST=unit tests, manual tests with hugefileman.
TBR=sky@chromium.org

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

[achuithb, 2012-07-24 08:23:02]

This CL is not ready. I need to write unit tests to make sure this code actually
works, but I wanted to get some early feedback on the direction of this CL.

[hashimoto, 2012-07-24 08:41:51]

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.h:45: GDataDirectoryService*
directory_service,
Are you going to inject a mock in tests?

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:562: base::Unretained(this),
Is this Unretained safe?

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

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:388: void SaveResourceMapToDB();
Where is this method used?

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:80:
kGDataResourceMetadataDBPath), GetTaskRunner());
Isn't there any chance for |directory_service_| to be accessed on UI thread
before the initialization completes?

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.h (right):

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.h:69:
scoped_refptr<base::SequencedTaskRunner> GetTaskRunner();
How about replacing |sequence_token_| with a
scoped_refptr<base::SequencedTaskRunner> instead of adding a private getter?

[satorux1, 2012-07-24 16:30:33]

The loading/saving seems to be much more expensive with the DB. Please do some
benchmark test.

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:562: base::Unretained(this),
On 2012/07/24 08:41:51, hashimoto wrote:
> Is this Unretained safe?

should use WeakPtr here.

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:563:
base::Owned(serialized_resources)));
Seems to me that initialization from the DB is significantly more expensive than
from the serialized proto.

Can you do some benchmark test for the load and save time, using the huge
account?

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

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:387: base::SequencedTaskRunner*
blocking_task_runner);
Loading from DB can fail, right? If so, the caller of this function should have
a way to tell if it's failed. Maybe add a callback parameter which is called
upon completion/failure?

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10800092/diff/8009/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:77: #if 1
#if 1 is weird. did you mean to use a command line flag instead?

[achuithb, 2012-08-02 02:25:19]

Satoru-san, please take a look at this CL again. I can close this CL and
re-upload if that's easier, but you don't need to look at the history of the
uploads.

[achuithb, 2012-08-02 09:32:22]

On 2012/08/02 02:25:19, achuith.bhandarkar wrote:
> Satoru-san, please take a look at this CL again. I can close this CL and
> re-upload if that's easier, but you don't need to look at the history of the
> uploads.

Hashimoto-san, if you could also take a look. Thanks!

[satorux1, 2012-08-02 17:24:03]

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/about_flags...
chrome/browser/about_flags.cc:819: },
It'd be nicer to create a separate patch for this with the strings.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:56: static base::Time
kTimerStart;
this is not allowed

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

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2835: #endif
Let's get rid of the #ifdef. Time object is cheap. don't need to worry about it.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2895: #endif
let's remove #ifdef, and pass the start time nicely, rather than using a global
variable.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:454: // Save |serialized_resources|
to the database.
Saves

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:462: // Initialize the database.
Initializes

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:465: // Read the database into
|serialized_resources|.
ditto

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:468: // Truncate the database.
ditto

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:469: void Truncate();
Truncating a DB sounds rather awkward, as Truncate() usually means truncating to
a smaller size, and truncating to zero is a special form. Maybe Clear()?

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:484: void
GDataDirectoryServiceDB::Create(CreateDBParams* params) {
I was confused about this function because of the name. Usually Create() is a
factory method, that returns an object, here, this function is void, and used to
create an object in another thread. 

I think we can rename this to something like
CreateDirectoryServiceDbOnBlockingPool(), and put it in anonymous function. we
need to make the constructor public, but it shouldn't be a problem.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:516: if (serialized_resources) {
can this be null?

DCHECK(serialized_resources); instead?

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:837: std::make_pair("version",
the indentation is awkard. 4 spaces from the previous line.

  serialized_resources.insert(
      std::make_pair("version",

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:840:
std::make_pair("largest_changestamp",
ditto.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:99: // TODO(achuith): Get rid of
this.
Please add a url to the bug

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:344: //
TODO(achuith,hashimoto,satorux): Move this to a separate file.
could you file a bug and add a url to it? I'll find someone to work on this.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:412: // Restore from and save to
database.
Restores/saves

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:423: // Initialize the resource map
using serialized_resources fetched from the db.
please fix other paces too

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:448:
base::WeakPtrFactory<GDataDirectoryService> ui_weak_ptr_factory_;
omit "ui_"?

[achuithb, 2012-08-02 20:26:25]

Thanks for the detailed feedback! Please diff against patch 21.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/about_flags...
chrome/browser/about_flags.cc:819: },
On 2012/08/02 17:24:03, satorux1 wrote:
> It'd be nicer to create a separate patch for this with the strings.

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:56: static base::Time
kTimerStart;
On 2012/08/02 17:24:03, satorux1 wrote:
> this is not allowed
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2835: #endif
On 2012/08/02 17:24:03, satorux1 wrote:
> Let's get rid of the #ifdef. Time object is cheap. don't need to worry about
it.

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2895: #endif
On 2012/08/02 17:24:03, satorux1 wrote:
> let's remove #ifdef, and pass the start time nicely, rather than using a
global
> variable.

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:454: // Save |serialized_resources|
to the database.
On 2012/08/02 17:24:03, satorux1 wrote:
> Saves

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:462: // Initialize the database.
On 2012/08/02 17:24:03, satorux1 wrote:
> Initializes

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:465: // Read the database into
|serialized_resources|.
On 2012/08/02 17:24:03, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:468: // Truncate the database.
On 2012/08/02 17:24:03, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:469: void Truncate();
On 2012/08/02 17:24:03, satorux1 wrote:
> Truncating a DB sounds rather awkward, as Truncate() usually means truncating
to
> a smaller size, and truncating to zero is a special form. Maybe Clear()?

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:484: void
GDataDirectoryServiceDB::Create(CreateDBParams* params) {
On 2012/08/02 17:24:03, satorux1 wrote:
> I was confused about this function because of the name. Usually Create() is a
> factory method, that returns an object, here, this function is void, and used
to
> create an object in another thread. 
> 
> I think we can rename this to something like
> CreateDirectoryServiceDbOnBlockingPool(), and put it in anonymous function. we
> need to make the constructor public, but it shouldn't be a problem.

I renamed the function, but it needs to be defined after the definitions of
GDataDirectoryServiceDB and CreateDBParams, so I had to make it static instead
of in anonymous namespace. I added a comment.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:516: if (serialized_resources) {
On 2012/08/02 17:24:03, satorux1 wrote:
> can this be null?
> 
> DCHECK(serialized_resources); instead?

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:837: std::make_pair("version",
On 2012/08/02 17:24:03, satorux1 wrote:
> the indentation is awkard. 4 spaces from the previous line.
> 
>   serialized_resources.insert(
>       std::make_pair("version",

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:840:
std::make_pair("largest_changestamp",
On 2012/08/02 17:24:03, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:99: // TODO(achuith): Get rid of
this.
On 2012/08/02 17:24:03, satorux1 wrote:
> Please add a url to the bug

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:344: //
TODO(achuith,hashimoto,satorux): Move this to a separate file.
On 2012/08/02 17:24:03, satorux1 wrote:
> could you file a bug and add a url to it? I'll find someone to work on this.

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:412: // Restore from and save to
database.
On 2012/08/02 17:24:03, satorux1 wrote:
> Restores/saves

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:423: // Initialize the resource map
using serialized_resources fetched from the db.
On 2012/08/02 17:24:03, satorux1 wrote:
> please fix other paces too

Done.

http://codereview.chromium.org/10800092/diff/21008/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:448:
base::WeakPtrFactory<GDataDirectoryService> ui_weak_ptr_factory_;
On 2012/08/02 17:24:03, satorux1 wrote:
> omit "ui_"?

Done.

[achuithb, 2012-08-02 20:29:25]

I'm going to need to rebase as well, but I'll do it after you're done with this
round of review to make your job easier.

[achuithb, 2012-08-02 21:53:02]

I've rebased.

[hashimoto, 2012-08-03 05:17:06]

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:3000: 
nit: Remove this line.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:1088: return entry.Pass();
Why don't you simply pass scoped_ptr<GDataEntry>()?

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_params.h (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_params.h:149: base::Time load_start_time_;
load_start_time

[satorux1, 2012-08-03 05:44:44]

http://codereview.chromium.org/10800092/diff/21032/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:375: extern const char kUseLevelDBForGData[];
You might want to move the switch change to the about flag patch. that'll be
logically better?

[satorux1, 2012-08-03 06:32:49]

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:449: class GDataDirectoryServiceDB
{
From this name, I originally thought this is a sub class of
GDataDirectoryService. Maybe rename it to something more generic like
"ResourceMetadataDB", which matches the file name "resource_metadata.db".

BTW, we should also move this into a set of separate files. Might want to do it
in this patch or add a TODO with a bug URL

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:532:
level_db_->Put(leveldb::WriteOptions(),
What if Put fails? Shouldn't we check the return value and handle the error (ex
remove the files, and emit  error log).

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:564:
blocking_task_runner_->PostTask(FROM_HERE,
I think you can use DelteSoon() instead

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:749:
create_params->serialized_resources.begin();
indentation

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:753: version != kProtoVersion) {
shouldn't we set the load_error to GDATA_FILE_ERROR_FAILED?

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:762: << largest_changestamp_;
ditto. load_error?

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:836:
serialized_resources.insert(std::make_pair("largest_changestamp",
Let's define constants for "version" and "largest_changestamp"

btw, we are assuming that these won't be used as resource IDs, but the
assumption is rather fragile. We might want to use a short prefix to ensure
uniqueness. like:

m:version
m:largest_changestamp
r:<resource_id>

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:1088: return entry.Pass();
On 2012/08/03 05:17:07, hashimoto wrote:
> Why don't you simply pass scoped_ptr<GDataEntry>()?

Agreed with hashimoto. That'll be clearer

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:1090: if
(entry_proto.file_info().is_directory()) {
move GDataEntryProto entry_proto; right before 'if'

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:1092: // Call
GDataEntry::FromProto.
Please add some more comment that why GDtaDirectory::FromProto() is not used.
the reason is, Here, we don't need to handle children, right?

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:455:
base::WeakPtrFactory<GDataDirectoryService> weak_ptr_factory_;
Please add


  // Note: This should remain the last member so it'll be destroyed and         
  // invalidate its weak pointers before any other members are destroyed.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:29: GDataDirectory*
AddDirectory(GDataDirectory* parent,
please write some comment. what's sequence_id, for instance.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:48: GDataFile*
AddFile(GDataDirectory* parent,
ditto

[achuithb, 2012-08-03 19:15:59]

Thanks for the comments! PTAL.

I've reworked the InitResourceMap logic to first do a version check and extract
the largest_changestamp. I think this is better than handling those special
fields whilst iterating through the map.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:449: class GDataDirectoryServiceDB
{
On 2012/08/03 06:32:50, satorux1 wrote:
> From this name, I originally thought this is a sub class of
> GDataDirectoryService. Maybe rename it to something more generic like
> "ResourceMetadataDB", which matches the file name "resource_metadata.db".
> 
> BTW, we should also move this into a set of separate files. Might want to do
it
> in this patch or add a TODO with a bug URL

I like ResourceMetadataDB. I've done the rename. 

I think it makes sense for GDataDirectoryResource and ResourceMetadataDB to be
in the same file because they are quite interwoven and call back into one
another. The only reason they are split into two entities is because
ResourceMetadataDB must live on the blocking pool, and GDataDirectoryResource
must live on the UI thread.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:532:
level_db_->Put(leveldb::WriteOptions(),
On 2012/08/03 06:32:50, satorux1 wrote:
> What if Put fails? Shouldn't we check the return value and handle the error
(ex
> remove the files, and emit  error log).

I've added a LOG(error), but I'm not sure what else really needs to be done. I
can't think of why Put would fail, unless the disk was full or there were disk
errors of some other kind.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:564:
blocking_task_runner_->PostTask(FROM_HERE,
On 2012/08/03 06:32:50, satorux1 wrote:
> I think you can use DelteSoon() instead

Done.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:749:
create_params->serialized_resources.begin();
On 2012/08/03 06:32:50, satorux1 wrote:
> indentation

Done.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:753: version != kProtoVersion) {
On 2012/08/03 06:32:50, satorux1 wrote:
> shouldn't we set the load_error to GDATA_FILE_ERROR_FAILED?

Done.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:762: << largest_changestamp_;
On 2012/08/03 06:32:50, satorux1 wrote:
> ditto. load_error?

I think this is an error but it's not fatal, right?

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:836:
serialized_resources.insert(std::make_pair("largest_changestamp",
On 2012/08/03 06:32:50, satorux1 wrote:
> Let's define constants for "version" and "largest_changestamp"
> 
> btw, we are assuming that these won't be used as resource IDs, but the
> assumption is rather fragile. We might want to use a short prefix to ensure
> uniqueness. like:
> 
> m:version
> m:largest_changestamp
> r:<resource_id>

Done.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:1088: return entry.Pass();
On 2012/08/03 06:32:50, satorux1 wrote:
> On 2012/08/03 05:17:07, hashimoto wrote:
> > Why don't you simply pass scoped_ptr<GDataEntry>()?
> 
> Agreed with hashimoto. That'll be clearer

Done.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:1090: if
(entry_proto.file_info().is_directory()) {
On 2012/08/03 06:32:50, satorux1 wrote:
> move GDataEntryProto entry_proto; right before 'if'

I don't understand? It's needed for ParseFromString. Did you mean
scoped_ptr<GDataEntry> entry?

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:1092: // Call
GDataEntry::FromProto.
On 2012/08/03 06:32:50, satorux1 wrote:
> Please add some more comment that why GDtaDirectory::FromProto() is not used.
> the reason is, Here, we don't need to handle children, right?

Correct. Comment added.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:455:
base::WeakPtrFactory<GDataDirectoryService> weak_ptr_factory_;
On 2012/08/03 06:32:50, satorux1 wrote:
> Please add
> 
> 
>   // Note: This should remain the last member so it'll be destroyed and       
 
>   // invalidate its weak pointers before any other members are destroyed.     
 

Done.

http://codereview.chromium.org/10800092/diff/21032/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:375: extern const char kUseLevelDBForGData[];
On 2012/08/03 05:44:44, satorux1 wrote:
> You might want to move the switch change to the about flag patch. that'll be
> logically better?

I'm using this constant in UseLevelDB.

[satorux1, 2012-08-03 20:19:15]

almost there!

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:1090: if
(entry_proto.file_info().is_directory()) {
On 2012/08/03 19:15:59, achuith.bhandarkar wrote:
> On 2012/08/03 06:32:50, satorux1 wrote:
> > move GDataEntryProto entry_proto; right before 'if'
> 
> I don't understand? It's needed for ParseFromString. Did you mean
> scoped_ptr<GDataEntry> entry?

my bad. i was wrong.

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:31: const char
kDBKeyLargestChangestamp[] = "m:largest_changestamp";
Maybe add

// m: prefix is used for global metadata of the file system
// r: prefix is used for resource id to entry proto pairs

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:760: if (load_params)
can it be NULL? Can we add DCHECK(load_params) at the beginning of the function
instead?

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:761: load_params->load_error =
GDATA_FILE_ERROR_NOT_FOUND;
maybe GDATA_FILE_ERROR_FAILED?

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:774: NOTREACHED() << "Could not
find largest_changestamp";
Add this?

load_params->load_error = GDATA_FILE_ERROR_FAILED;
return;

[achuithb, 2012-08-03 21:41:04]

Apologize for the rebase, but the diffs should be easy to see.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_params.h (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_params.h:149: base::Time load_start_time_;
On 2012/08/03 05:17:07, hashimoto wrote:
> load_start_time

Done.

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:31: const char
kDBKeyLargestChangestamp[] = "m:largest_changestamp";
On 2012/08/03 20:19:16, satorux1 wrote:
> Maybe add
> 
> // m: prefix is used for global metadata of the file system
> // r: prefix is used for resource id to entry proto pairs

Done.

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:760: if (load_params)
On 2012/08/03 20:19:16, satorux1 wrote:
> can it be NULL? Can we add DCHECK(load_params) at the beginning of the
function
> instead?

It can be NULL for tests. 

I'm going to change this, so it's not supposed to be NULL, and add a DCHECK.
Also, we don't really need all the LoadRootParams, just the error, so I'm only
going to pass this in.

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:761: load_params->load_error =
GDATA_FILE_ERROR_NOT_FOUND;
On 2012/08/03 20:19:16, satorux1 wrote:
> maybe GDATA_FILE_ERROR_FAILED?

Done.

http://codereview.chromium.org/10800092/diff/27018/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.cc:774: NOTREACHED() << "Could not
find largest_changestamp";
On 2012/08/03 20:19:16, satorux1 wrote:
> Add this?
> 
> load_params->load_error = GDATA_FILE_ERROR_FAILED;
> return;

Done.

[achuithb, 2012-08-03 21:41:16]

PTAL!

[satorux1, 2012-08-03 21:45:54]

LGTM with nits. You'll need approval for chrome_siwtches.h from someone else.

http://codereview.chromium.org/10800092/diff/27019/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

http://codereview.chromium.org/10800092/diff/27019/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:439: GDataFileError error;
initialize this to something like GDATA_FILE_FAILED, then...

http://codereview.chromium.org/10800092/diff/27019/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:442:
test_util::RunBlockingPoolTask();
add this?

EXPECT_EQ(GDATA_FILE_OK, error);

http://codereview.chromium.org/10800092/diff/27019/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:452:
test_util::RunBlockingPoolTask();
ditto.

[achuithb, 2012-08-03 22:05:34]

PTAL!

http://codereview.chromium.org/10800092/diff/27019/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

http://codereview.chromium.org/10800092/diff/27019/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:439: GDataFileError error;
On 2012/08/03 21:45:54, satorux1 wrote:
> initialize this to something like GDATA_FILE_FAILED, then...

I need to initialize this to GDATA_FILE_OK so InitFromDB can continue.

http://codereview.chromium.org/10800092/diff/27019/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:442:
test_util::RunBlockingPoolTask();
On 2012/08/03 21:45:54, satorux1 wrote:
> add this?
> 
> EXPECT_EQ(GDATA_FILE_OK, error);

It's actually GDATA_FILE_ERROR_NOT_FOUND in this case. Done.

http://codereview.chromium.org/10800092/diff/27019/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:452:
test_util::RunBlockingPoolTask();
On 2012/08/03 21:45:54, satorux1 wrote:
> ditto.

Done.

[satorux1, 2012-08-03 22:17:26]

http://codereview.chromium.org/10800092/diff/35001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10800092/diff/35001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2878: &params->load_error,
base::Bind(
I think I was confused about this |load_error| parameter.

Can you check params->load_error before calling InitFromDB? If this is not OK
here, we don't have to call InitFromDB, right?

Then, I think we can get rid of the error parameter from InitFromDB, because

[achuithb, 2012-08-03 22:25:58]

http://codereview.chromium.org/10800092/diff/35001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10800092/diff/35001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2878: &params->load_error,
base::Bind(
On 2012/08/03 22:17:26, satorux1 wrote:
> I think I was confused about this |load_error| parameter.
> 
> Can you check params->load_error before calling InitFromDB? If this is not OK
> here, we don't have to call InitFromDB, right?
> 
> Then, I think we can get rid of the error parameter from InitFromDB, because 

because? The suspense is killing me ;)

We need to set the error in the case where the db is not found because we use
this as a signal to let the user know we are refreshing drive for the first
time.

error is definitely OK over here since we just created the param. I'll replace
the runtime check with a DCHECK instead.

[satorux1, 2012-08-03 22:33:30]

http://codereview.chromium.org/10800092/diff/35001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10800092/diff/35001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2878: &params->load_error,
base::Bind(
On 2012/08/03 22:25:58, achuith.bhandarkar wrote:
> On 2012/08/03 22:17:26, satorux1 wrote:
> > I think I was confused about this |load_error| parameter.
> > 
> > Can you check params->load_error before calling InitFromDB? If this is not
OK
> > here, we don't have to call InitFromDB, right?
> > 
> > Then, I think we can get rid of the error parameter from InitFromDB, because

> 
> because? The suspense is killing me ;)
> 
> We need to set the error in the case where the db is not found because we use
> this as a signal to let the user know we are refreshing drive for the first
> time.
> 
> error is definitely OK over here since we just created the param. I'll replace
> the runtime check with a DCHECK instead.

oops, here's what I was thinking:

Instead of passing GDataFileError* to InitFromDB(), I think the callback should
get the error as a parameter, then modify params->load_error as needed in the
callbck function

[achuithb, 2012-08-03 23:27:35]

Ok, switched to using a different callback. 

Also added some function comments that I missed earlier. 

PTAL!

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:29: GDataDirectory*
AddDirectory(GDataDirectory* parent,
On 2012/08/03 06:32:50, satorux1 wrote:
> please write some comment. what's sequence_id, for instance.

Done.

http://codereview.chromium.org/10800092/diff/21032/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:48: GDataFile*
AddFile(GDataDirectory* parent,
On 2012/08/03 06:32:50, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/10800092/diff/35001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10800092/diff/35001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2878: &params->load_error,
base::Bind(
On 2012/08/03 22:33:30, satorux1 wrote:
> On 2012/08/03 22:25:58, achuith.bhandarkar wrote:
> > On 2012/08/03 22:17:26, satorux1 wrote:
> > > I think I was confused about this |load_error| parameter.
> > > 
> > > Can you check params->load_error before calling InitFromDB? If this is not
> OK
> > > here, we don't have to call InitFromDB, right?
> > > 
> > > Then, I think we can get rid of the error parameter from InitFromDB,
because
> 
> > 
> > because? The suspense is killing me ;)
> > 
> > We need to set the error in the case where the db is not found because we
use
> > this as a signal to let the user know we are refreshing drive for the first
> > time.
> > 
> > error is definitely OK over here since we just created the param. I'll
replace
> > the runtime check with a DCHECK instead.
> 
> oops, here's what I was thinking:
> 
> Instead of passing GDataFileError* to InitFromDB(), I think the callback
should
> get the error as a parameter, then modify params->load_error as needed in the
> callbck function

Done.

[satorux1, 2012-08-04 00:00:38]

LGTM with nits.

Looks much better! Please submit shortly, so I can start creating a patch for
moving GDataWapiFeedLoader class to new files. :)

http://codereview.chromium.org/10800092/diff/23063/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

http://codereview.chromium.org/10800092/diff/23063/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:452:
AppendASCII("meta").AppendASCII("resource_metadata.db"));
Please add some comment like:

// InitFromDB should fail as the DB is not present.

http://codereview.chromium.org/10800092/diff/23063/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:463:
directory_service2.InitFromDB(db_path, blocking_task_runner,
like:

// InitFromDB Should succeed as the DB is now present.

[achuithb, 2012-08-04 00:24:11]

Thanks for the review!

http://codereview.chromium.org/10800092/diff/23063/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

http://codereview.chromium.org/10800092/diff/23063/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:452:
AppendASCII("meta").AppendASCII("resource_metadata.db"));
On 2012/08/04 00:00:38, satorux1 wrote:
> Please add some comment like:
> 
> // InitFromDB should fail as the DB is not present.

Done.

http://codereview.chromium.org/10800092/diff/23063/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:463:
directory_service2.InitFromDB(db_path, blocking_task_runner,
On 2012/08/04 00:00:38, satorux1 wrote:
> like:
> 
> // InitFromDB Should succeed as the DB is now present.

Done.

[commit-bot: I haz the power, 2012-08-04 03:11:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/10800092/35011

[commit-bot: I haz the power, 2012-08-04 03:12:00]

Presubmit check for 10800092-35011 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.1s to calculate.

[achuithb, 2012-08-04 03:14:07]

TBR Scott for chrome_switches changes. Please take a look when you get a chance.

[commit-bot: I haz the power, 2012-08-04 03:14:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/10800092/35011

[commit-bot: I haz the power, 2012-08-04 03:37:54]

Try job failure for 10800092-35011 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-04 05:48:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/10800092/35011

[commit-bot: I haz the power, 2012-08-04 06:06:39]

Try job failure for 10800092-35011 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-04 06:08:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/10800092/35011

[commit-bot: I haz the power, 2012-08-04 06:35:12]

Try job failure for 10800092-35011 (retry) (retry) on win for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-04 06:43:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/10800092/35011

[commit-bot: I haz the power, 2012-08-04 07:04:45]

Try job failure for 10800092-35011 (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[satorux1, 2012-08-04 07:32:58]

submitted on behalf as CQ was failing forever for an unrelated error in win bot.