Implement initialization for GDataCache to use in-memory metadata for tests.

Implement initialization for GDataCache to use in-memory metadata for tests.
Use the in-memory version for GDataFileSystemTest.


BUG=137945
TEST=unit_tests   --gtest_filter='GDataFileSystemTest*'

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

[Haruki Sato, 2012-08-06 08:52:48]

Hello Reviewers,

Could you take a look and advice me if this is the best way to control metadata
implementation for tests?

Thanks.

[kochi, 2012-08-06 09:02:17]

Could you use enum instead of boolean for arguments
in this CL?

http://chromiumcodereview.appspot.com/10830181/diff/3001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.h (right):

http://chromiumcodereview.appspot.com/10830181/diff/3001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_cache_metadata.h:40:
base::SequencedTaskRunner* blocking_task_runner, bool use_map);
We prefer enum to boolean for function arguments.
See
http://dev.chromium.org/developers/coding-style#Code_Formatting

[Haruki Sato, 2012-08-06 09:04:41]

Thank you for the advice.
Will work on it.
Please let me know if you have any other comment on anything in this CL.

Thanks.

On 2012/08/06 09:02:17, Takayoshi Kochi wrote:
> Could you use enum instead of boolean for arguments
> in this CL?
> 
>
http://chromiumcodereview.appspot.com/10830181/diff/3001/chrome/browser/chrom...
> File chrome/browser/chromeos/gdata/gdata_cache_metadata.h (right):
> 
>
http://chromiumcodereview.appspot.com/10830181/diff/3001/chrome/browser/chrom...
> chrome/browser/chromeos/gdata/gdata_cache_metadata.h:40:
> base::SequencedTaskRunner* blocking_task_runner, bool use_map);
> We prefer enum to boolean for function arguments.
> See
> http://dev.chromium.org/developers/coding-style#Code_Formatting

[hshi1, 2012-08-06 14:11:07]

This looks good to me, but I'd suggest Satoru to take a look as he is more
familiar with the code. Thanks!

[achuithb, 2012-08-06 23:22:20]

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.h (right):

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.h:320: // Force std::map
implementaition for metadata.
spelling - implementation.

You could also add a param to RequestInitializeOnUIThread so mistakes are not
made with ordering this function wrt to RequestInitializeOnUIThread (as in, this
function must be called first).

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:593:
scoped_ptr<GDataCacheMetadata> GDataCacheMetadata::CreateGDataCacheMetadata(
It may be better to write a separate function like
CreateGDataCacheMetadataForTesting instead of adding a boolean.

[achuithb, 2012-08-06 23:23:51]

Please don't land this until my concern in the bug is addressed - we need to be
sure we want to create this new dependency on GDataCacheMetadataMap, which is
currently unused.

[satorux1, 2012-08-07 10:50:08]

As mentioned in the bug tracker, making the test faster is a good thing, but
achuith's concern about the maintenance cost is also valid. We should keep this
in mind, and consider removing the in-memory cache implementation if maintenance
becomes a problem.

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.h (right):

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.h:320: // Force std::map
implementaition for metadata.
On 2012/08/06 23:22:20, achuith.bhandarkar wrote:
> spelling - implementation.
> 
> You could also add a param to RequestInitializeOnUIThread so mistakes are not
> made with ordering this function wrt to RequestInitializeOnUIThread (as in,
this
> function must be called first).

Yes, Ordering is nasty. Maybe we should add
RequestInitializeOnUIThreadForTesting()

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:593:
scoped_ptr<GDataCacheMetadata> GDataCacheMetadata::CreateGDataCacheMetadata(
On 2012/08/06 23:22:20, achuith.bhandarkar wrote:
> It may be better to write a separate function like
> CreateGDataCacheMetadataForTesting instead of adding a boolean.

agreed!

[Haruki Sato, 2012-08-07 13:40:39]

Thank you for the review. I understand your concerns.
Achuith, are we planning to remove std::map implementation and use DB
implementation as the only impl for metadata?

If so, I agree that we should use that one-and-only implementation and should
not maintain another version only for tests.

[satorux1, 2012-08-07 15:11:08]

Hmm, I'd rather recommend to pursue this patch. We can revisit if the
maintenance issue becomes a real problem. BTW' we should rename the
in-memory version to something like FakeGDataCacheMetadata to make it clear
it's for testing
  On Aug 7, 2012 6:40 AM, <haruki@chromium.org> wrote:


> Thank you for the review. I understand your concerns.
> Achuith, are we planning to remove std::map implementation and use DB
> implementation as the only impl for metadata?

> If so, I agree that we should use that one-and-only implementation and
> should
> not maintain another version only for tests.

>
http://codereview.chromium.**org/10830181/<http://codereview.chromium.org/108...

[Haruki Sato, 2012-08-08 05:00:42]

Removed the boolean flag and added
RequestInitializeOnUIThreadForTesting() and InitializeForTesting().
and I feel like it's much simpler.
Thank you for the advice!


JFYI, I tried the unit_tests locally and the tests run faster.
$ grep 'GDataFileSystemTest.*ms total' log.*
log.master1:[----------] 55 tests from GDataFileSystemTest (15680 ms total)
log.master2:[----------] 55 tests from GDataFileSystemTest (14909 ms total)
log.master3:[----------] 55 tests from GDataFileSystemTest (17380 ms total)
log.master4:[----------] 55 tests from GDataFileSystemTest (14837 ms total)
log.master5:[----------] 55 tests from GDataFileSystemTest (14490 ms total)
log.new1:[----------] 55 tests from GDataFileSystemTest (9763 ms total)
log.new2:[----------] 55 tests from GDataFileSystemTest (9753 ms total)
log.new3:[----------] 55 tests from GDataFileSystemTest (9827 ms total)
log.new4:[----------] 55 tests from GDataFileSystemTest (9718 ms total)
log.new5:[----------] 55 tests from GDataFileSystemTest (9706 ms total)


Could you take another look?
Please let me know if you think we might want changes about the naming etc.

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.h (right):

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.h:320: // Force std::map
implementaition for metadata.
On 2012/08/07 10:50:08, satorux1 wrote:
> On 2012/08/06 23:22:20, achuith.bhandarkar wrote:
> > spelling - implementation.
> > 
> > You could also add a param to RequestInitializeOnUIThread so mistakes are
not
> > made with ordering this function wrt to RequestInitializeOnUIThread (as in,
> this
> > function must be called first).
> 
> Yes, Ordering is nasty. Maybe we should add
> RequestInitializeOnUIThreadForTesting()
Thank you for the advice!
Removed the flag and added *ForTesting methods.

Could you take a look?

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:593:
scoped_ptr<GDataCacheMetadata> GDataCacheMetadata::CreateGDataCacheMetadata(
On 2012/08/07 10:50:08, satorux1 wrote:
> On 2012/08/06 23:22:20, achuith.bhandarkar wrote:
> > It may be better to write a separate function like
> > CreateGDataCacheMetadataForTesting instead of adding a boolean.
> 
> agreed!

Done!

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.h (right):

http://codereview.chromium.org/10830181/diff/3001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.h:40:
base::SequencedTaskRunner* blocking_task_runner, bool use_map);
On 2012/08/06 09:02:17, Takayoshi Kochi wrote:
> We prefer enum to boolean for function arguments.
> See
> http://dev.chromium.org/developers/coding-style#Code_Formatting

Thank you for the advice.
Removed the argument and added separate *ForTesting methods

[achuithb, 2012-08-08 05:07:31]

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.h (right):

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.h:335: // Initializes the cache with
in-memory cache for testing.
nit: maybe add a comment saying that the in-memory cache is used since it's
faster than the db.

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:599:
scoped_ptr<GDataCacheMetadata>
add a comment:
// static

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.h (right):

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.h:37: // For testing, the
thread assertion can be disabled by passing NULL and
Is this comment still valid?

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.h:42: static
scoped_ptr<GDataCacheMetadata> CreateGDataCacheMetadataForTesting(
Add a function comment please.

[Haruki Sato, 2012-08-08 05:57:38]

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache.h (right):

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache.h:335: // Initializes the cache with
in-memory cache for testing.
On 2012/08/08 05:07:31, achuith.bhandarkar wrote:
> nit: maybe add a comment saying that the in-memory cache is used since it's
> faster than the db.

Done. Thanks.

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:599:
scoped_ptr<GDataCacheMetadata>
On 2012/08/08 05:07:31, achuith.bhandarkar wrote:
> add a comment:
> // static

Done.
Thanks!

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.h (right):

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.h:37: // For testing, the
thread assertion can be disabled by passing NULL and
On 2012/08/08 05:07:31, achuith.bhandarkar wrote:
> Is this comment still valid?

Seems like it's no longer valid. Replaced.

http://codereview.chromium.org/10830181/diff/3002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.h:42: static
scoped_ptr<GDataCacheMetadata> CreateGDataCacheMetadataForTesting(
On 2012/08/08 05:07:31, achuith.bhandarkar wrote:
> Add a function comment please.

Done.
Thanks!

[satorux1, 2012-08-08 15:53:57]

http://codereview.chromium.org/10830181/diff/11002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/11002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:604: new
GDataCacheMetadataMap(blocking_task_runner));
Could you rename GDataCacheMetadataMap to FakeGDataCacheMetadata, as suggested
before?

[Haruki Sato, 2012-08-09 02:04:02]

Sounds like the Map impl of GDataCacheMetadata would be only used for tests.
Renamed it to FakeGDataCacheMetadata as suggested.

http://codereview.chromium.org/10830181/diff/11002/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/11002/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:604: new
GDataCacheMetadataMap(blocking_task_runner));
On 2012/08/08 15:53:57, satorux1 wrote:
> Could you rename GDataCacheMetadataMap to FakeGDataCacheMetadata, as suggested
> before?

Done.

[satorux1, 2012-08-09 08:35:07]

LGTM with a nit

http://codereview.chromium.org/10830181/diff/6004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:290: // Used for testings.
// Used for testing.

[Haruki Sato, 2012-08-10 07:28:11]

Thank you for the review. Adding to CQ.

http://codereview.chromium.org/10830181/diff/6004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_cache_metadata.cc (right):

http://codereview.chromium.org/10830181/diff/6004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_cache_metadata.cc:290: // Used for testings.
On 2012/08/09 08:35:07, satorux1 wrote:
> // Used for testing.

Done.
Thanks.

[commit-bot: I haz the power, 2012-08-10 07:28:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/10830181/7007

[commit-bot: I haz the power, 2012-08-10 09:41:30]

Change committed as 151013