Wired GDataFileSystem::GetFile() method with internal cache.

Wired GDataFileSystem::GetFile() method with internal cache.

BUG=chromium-os:27686
TEST=GDataFileSystemTest.GetFileFromCache, GDataFileSystemTest.GetFileFromDownloads,

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

[zel, 2012-03-20 02:20:50]

first cut, still playing with tests

[kuan, 2012-03-20 03:22:19]

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1044: false  /* is_local */);
nit: align "false" with "resource_id"

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1051: // otherwise.
not sure if u really mean to copy and paste the comments from header file to
here?

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1071: const FilePath&
cache_file_path) {
nit: only 4 spaces needed for indentation

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
chrome/browser/chromeos/gdata/gdata_file_system.h:328: // Defines set of
parameters passes to intermediate callbacks during
nit: s/passes/passed/

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
chrome/browser/chromeos/gdata/gdata_file_system.h:490: void
OnStoreToCache(base::PlatformFileError error,
for internal StoreToCache() calls, there's already a OnStoredToCache; u might
want to change the comments and rename method to something related to
GetFileFromCache?
or alternatively, since your OnStoreToCache does nothing now, maybe u don't need
to pass in callback to StoreToCache?

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
chrome/browser/chromeos/gdata/gdata_file_system.h:731: const FilePath&
cache_file_path);
nit: only 4 spaces needed for indentation.

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

http://codereview.chromium.org/9742002/diff/1/chrome/browser/chromeos/gdata/g...
chrome/browser/chromeos/gdata/gdata_operations.cc:186:
net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES);
these same load flags are used by DownloadFileOperation; browser caching wil not
be disabled without net::LOAD_DISABLE_CACHE flag, resulting in 2 local copies of
the files when DownloadFile is invoked.

[zel, 2012-03-20 04:31:17]

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1044: false  /* is_local */);
On 2012/03/20 03:22:19, kuan wrote:
> nit: align "false" with "resource_id"

Done.

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1051: // otherwise.
On 2012/03/20 03:22:19, kuan wrote:
> not sure if u really mean to copy and paste the comments from header file to
> here?

Removed.

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1071: const FilePath&
cache_file_path) {
On 2012/03/20 03:22:19, kuan wrote:
> nit: only 4 spaces needed for indentation

Done.

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_file_system.h:328: // Defines set of
parameters passes to intermediate callbacks during
On 2012/03/20 03:22:19, kuan wrote:
> nit: s/passes/passed/

Done.

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_file_system.h:490: void
OnStoreToCache(base::PlatformFileError error,
On 2012/03/20 03:22:19, kuan wrote:
> for internal StoreToCache() calls, there's already a OnStoredToCache; u might
> want to change the comments and rename method to something related to
> GetFileFromCache?
> or alternatively, since your OnStoreToCache does nothing now, maybe u don't
need
> to pass in callback to StoreToCache?

Renamed to OnDownloadStoredToCache. We could remove it later if we end up doing
nothing there at the end.

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_file_system.h:731: const FilePath&
cache_file_path);
On 2012/03/20 03:22:19, kuan wrote:
> nit: only 4 spaces needed for indentation.

Done.

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

https://chromiumcodereview.appspot.com/9742002/diff/1/chrome/browser/chromeos...
chrome/browser/chromeos/gdata/gdata_operations.cc:186:
net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES);
On 2012/03/20 03:22:19, kuan wrote:
> these same load flags are used by DownloadFileOperation; browser caching wil
not
> be disabled without net::LOAD_DISABLE_CACHE flag, resulting in 2 local copies
of
> the files when DownloadFile is invoked.

goo catch. i don't think we ever want to cache anything here from the browser
side

[satorux1, 2012-03-20 04:51:17]

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:199: 
nit: remove the blank line.

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1044: false  /* is_local */);
Would be easier to read if it's enum like PINNED_FILE.

http://www.chromium.org/developers/coding-style

Begin to prefer enumerated values (e.g. "DELETE_AFTER_USE/KEEP_ALIVE_AFTER_USE")
to booleans ("bool delete_after_use") as function arguments the more the
following cases apply:

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:278: GURL* content_url);
Having four output parameters looks unclean. Cannot we define a struct that
includes all of them, like

struct GDataFileInfo {
  file_info;
  resource_id;
  md5;
  content_url;
}

this is more like subset of GDataFileBase.

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:610: FilePath
GetCacheFilePath(const std::string& resource_id,
is_local is not documented. what is this for?

[kuan, 2012-03-20 05:22:27]

lgtm, just nit in unittest.

https://chromiumcodereview.appspot.com/9742002/diff/8002/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

https://chromiumcodereview.appspot.com/9742002/diff/8002/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:1555:
message_loop_.RunAllPending();  // Wait to get our result.
nit: don't need this because RunAllPendingForCache has it.

[zel, 2012-03-20 05:24:24]

I've also added new unit tests for testing cached and non-cached GetFile()
calls.

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:199: 
On 2012/03/20 04:51:17, satorux1 wrote:
> nit: remove the blank line.

Done.

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1044: false  /* is_local */);
On 2012/03/20 04:51:17, satorux1 wrote:
> Would be easier to read if it's enum like PINNED_FILE.
> 
> http://www.chromium.org/developers/coding-style
> 
> Begin to prefer enumerated values (e.g.
"DELETE_AFTER_USE/KEEP_ALIVE_AFTER_USE")
> to booleans ("bool delete_after_use") as function arguments the more the
> following cases apply:

Done.

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:278: GURL* content_url);
On 2012/03/20 04:51:17, satorux1 wrote:
> Having four output parameters looks unclean. Cannot we define a struct that
> includes all of them, like
> 
> struct GDataFileInfo {
>   file_info;
>   resource_id;
>   md5;
>   content_url;
> }
> 
> this is more like subset of GDataFileBase.

Then I have to copy way more than I really want every time I look up files. This
way I can avoid extra memory copies from gdata_file_system_proxy.cc layer and
still reuse the same function.

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:610: FilePath
GetCacheFilePath(const std::string& resource_id,
On 2012/03/20 04:51:17, satorux1 wrote:
> is_local is not documented. what is this for?

switched to self-describing enum instead.

[zel, 2012-03-20 05:27:20]

http://codereview.chromium.org/9742002/diff/8002/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

http://codereview.chromium.org/9742002/diff/8002/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:1555:
message_loop_.RunAllPending();  // Wait to get our result.
On 2012/03/20 05:22:27, kuan wrote:
> nit: don't need this because RunAllPendingForCache has it.

Done.

[satorux1, 2012-03-20 05:35:14]

LGTM. I like the new enum!

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9742002/diff/4001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:278: GURL* content_url);
On 2012/03/20 05:24:25, zel wrote:
> On 2012/03/20 04:51:17, satorux1 wrote:
> > Having four output parameters looks unclean. Cannot we define a struct that
> > includes all of them, like
> > 
> > struct GDataFileInfo {
> >   file_info;
> >   resource_id;
> >   md5;
> >   content_url;
> > }
> > 
> > this is more like subset of GDataFileBase.
> 
> Then I have to copy way more than I really want every time I look up files.
This
> way I can avoid extra memory copies from gdata_file_system_proxy.cc layer and
> still reuse the same function.

I see. Then we could define two separate functions 

GetFileInfoFromPath(file_path, file_info); -- the original
GetFileAndCacheInfoFromPath(file_path, file_info, x, y, z); -- the new one

However, the original function doesn't seem to be used from many places so,
having only the complex version would be ok.

[satorux1, 2012-03-20 05:35:16]

LGTM. I like the new enum!

[commit-bot: I haz the power, 2012-03-20 07:27:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/9742002/4025

[commit-bot: I haz the power, 2012-03-20 07:51:10]

Try job failure for 9742002-4025 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...