gdata: Convert public synchronous functions in GDataFileSystem to asynchronous.

gdata: Get rid of GDataFileSystem::GetFileInfoByPath()

This is part 1 of converting public synchronous functions in GDataFileSystem to asynchronous. 

Eliminate the helper class GDataFileProperties and the public synchronous function GetFileInfoByPath().
    
BUG=127048
TEST=unit_tests, browser_tests

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

[hshi1, 2012-06-07 20:28:20]

Hi Satoru, Toni -- this bug (127048) requires refactoring 3 public synchronous
functions of GDataFileSystem, namely GetFileInfoByPath,
FindEntryByResourceIdSync, and AddUploadedFile.

This patch set is part#1 that deals with GetFileInfoByPath only. I had initially
wanted to do all 3 in one patch set but it seems this CL is already fairly big,
so I think maybe it is better if we can land them separately. Thanks!

[satorux1, 2012-06-07 22:47:17]

Nice work!

Please rewrite the patch description so we have the most descriptive information
on the first line:

gdata: Get rid of GDataFileSystem::GetFileInfoByPath()

This is part 1 of converting public synchronous functions in GDataFileSystem to
asynchronous. 

blah blah blah.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:687:
handler_extension));
nit: merge the line to the previous line?

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.h:68: typedef
base::Callback<void(const extensions::Extension*)>
nit: please put the parameter name.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2017:
base::Bind(&GDataFileSystem::GetFileInfoByPathAsync,
Can you keep the function as before? We use this pattern (call FooOnUI from Foo)
in other functions. I'd like to keep consistency.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2050:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
DCHECK(file_proto);

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:25: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
Please move this back to the original place. 

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

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:232: void
OnGetFileInfoForInsertGDataCachePathsPermissions(
Please move this to anonymous namespace. Please write a function comment.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:297:
file_system->GetFileInfoByPathAsync(
You are creating a loop using the callback, which is rather tricky. Please write
some comment about it.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util_unittest.cc:7: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
Please put this on the top.

I guess you don't have to edit the file.

[tbarzic, 2012-06-07 22:47:56]

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:365: executor_,
why do you create callback here instead in ExecuteFileActionsOnUIThread, when we
actually call InitHandlerHostFileAccessPermissions?

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:581: const Extension*
extension) {
what if the extension goes away while we were getting cache paths?

I'd rather just refetch extension from the service again that pass the pointer..

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:682:
gdata::util::InsertGDataCachePathsPermissions(
We don't really have to go to gdata_util if there are no gdata files, so we
could do something like
if (gdata_paths->empty()) {
  run callback (or post callback to the current message loop)
  return;
}

this may be easier to follow.. and we wouldn't have to try getting gdata file
system..

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:686:
base::Bind(callback,
this fits in a single line

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_util.cc:25: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
why did you move this here??

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_util.cc:233: GDataFileSystem* file_system,
I'm not sure how safe is passing file_system pointer in callbacks :)

In this case it is probably ok since the callback is run synchronously on ui
thread from the filesystem itself, but seems kinda fragile..

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_util.cc:241: // TODO(hshi):
GetFileInfoByPathAsync may fail when the gdata file is not
Are you sure? AFAIK GetFileInfoByPath doesn't check gdata cache (GetFileByPath
does)?

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_util.cc:242: // present in cache. For now
just grant permission to the entire cache subdir.
why don't we just ignore the file in that case (or return error)?

If we can't find file, we won't be able to access it later whether we have the
permission or not..

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_util.cc:296: 
Can you add a comment for this?
It is not obvious by looking at the method that this is recursive?

[hshi1, 2012-06-08 00:22:05]

Please see Patch Set #2 (comments are in the review page), thanks!

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:365: executor_,
Because several parameters (such as file_system_name and file_system_root) are
only used by OnInitAccessForExecuteFileActionsOnUIThread but not in
InitHandlerHostFileAccessPermissions. By binding these parameters here I avoid
unnecessarily passing the parameters through ExecuteFileActionsOnUIThread.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:581: const Extension*
extension) {
On 2012/06/07 22:47:56, tbarzic wrote:
> what if the extension goes away while we were getting cache paths?
> 
> I'd rather just refetch extension from the service again that pass the
pointer..

Done.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:682:
gdata::util::InsertGDataCachePathsPermissions(
On 2012/06/07 22:47:56, tbarzic wrote:
> We don't really have to go to gdata_util if there are no gdata files, so we
> could do something like
> if (gdata_paths->empty()) {
>   run callback (or post callback to the current message loop)
>   return;
> }
> 
> this may be easier to follow.. and we wouldn't have to try getting gdata file
> system..

Done.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:686:
base::Bind(callback,
On 2012/06/07 22:47:56, tbarzic wrote:
> this fits in a single line

Done.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:687:
handler_extension));
On 2012/06/07 22:47:17, satorux1 wrote:
> nit: merge the line to the previous line?

Done.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.h:68: typedef
base::Callback<void(const extensions::Extension*)>
On 2012/06/07 22:47:17, satorux1 wrote:
> nit: please put the parameter name.

Removed this definition as Toni suggested to get the Extension* pointer again in
the callback, so I only need base::Closure for my purpose.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2017:
base::Bind(&GDataFileSystem::GetFileInfoByPathAsync,
On 2012/06/07 22:47:17, satorux1 wrote:
> Can you keep the function as before? We use this pattern (call FooOnUI from
Foo)
> in other functions. I'd like to keep consistency. 

Done.

Note that I have to create a common shared function called
"GetResolvedFileByPath" (suggestions welcome for a proper name). This is shared
by OnGetFileInfoCompleteForGetFileByPath and OnGetFileInfoCompleteForOpenFile.
Previously OnGetFileInfoCompleteForOpenFile just calls GetFileByPathOnUIThread()
but after this refactoring I need to invent some new function names :-)

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2050:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/06/07 22:47:17, satorux1 wrote:
> DCHECK(file_proto);

Actually file_proto can be NULL. It is checked in the line below: if error != OK
or !file_proto then make callback with ERROR_NOT_FOUND error.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:25: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
On 2012/06/07 22:47:17, satorux1 wrote:
> Please move this back to the original place. 
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

Done.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:25: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
On 2012/06/07 22:47:56, tbarzic wrote:
> why did you move this here??

Done. I originally moved the line to avoid a compile error with the
base::Closure& type in the gdata_util.h, but now I realize that I just need to
include base/bind.h in the gdata_util.h.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:232: void
OnGetFileInfoForInsertGDataCachePathsPermissions(
On 2012/06/07 22:47:17, satorux1 wrote:
> Please move this to anonymous namespace. Please write a function comment.

Done.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:233: GDataFileSystem* file_system,
I'm going to change this to obtain file_system pointer from profile instead.
On 2012/06/07 22:47:56, tbarzic wrote:
> I'm not sure how safe is passing file_system pointer in callbacks :)
> 
> In this case it is probably ok since the callback is run synchronously on ui
> thread from the filesystem itself, but seems kinda fragile..

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:242: // present in cache. For now
just grant permission to the entire cache subdir.
On 2012/06/07 22:47:56, tbarzic wrote:
> why don't we just ignore the file in that case (or return error)?
> 
> If we can't find file, we won't be able to access it later whether we have the
> permission or not..

There may be bugs elsewhere but my observation at TOT is that this indeed occurs
(for example during browser_test "RemoteMountPoint"), and ignoring the file
would cause a test failure due to lack of access permissions.

We can investigate this next, but I would like to not touch this logic in a
refactoring CL. As far as I can see, this happens even before my refactoring CL.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:296: 
On 2012/06/07 22:47:56, tbarzic wrote:
> Can you add a comment for this?
> It is not obvious by looking at the method that this is recursive?

Done.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:297:
file_system->GetFileInfoByPathAsync(
On 2012/06/07 22:47:17, satorux1 wrote:
> You are creating a loop using the callback, which is rather tricky. Please
write
> some comment about it.

Done.

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util_unittest.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util_unittest.cc:7: #include
"chrome/browser/chromeos/gdata/gdata_util.h"
On 2012/06/07 22:47:17, satorux1 wrote:
> Please put this on the top.
> 
> I guess you don't have to edit the file.

Done.

[hshi1, 2012-06-08 01:10:34]

By the way Toni just identified the problem in file_handler_util.cc where we
should be passing the virtual_path to InsertGDataCachePathsPermissions.

In that case I'll leave the code as-is and ping Toni once this CL lands. After
fixing the problem in file_handler_utils.cc we can safely remove the TODO in
OnGetFileInfoForInsertGDataCachePathsPermissions

[hshi1, 2012-06-08 17:51:59]

Hi please review the new Patch Set #3 (rebased to svn:trunk/src@141224).

This has merged Toni's revision 141220 (file handler executor setting wrong
cache path permissions), but unfortunately also took a few Hashimoto's
refactoring changes. Thanks.

[tbarzic, 2012-06-08 17:59:13]

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:365: executor_,
On 2012/06/08 00:22:05, hshi1 wrote:
> Because several parameters (such as file_system_name and file_system_root) are
> only used by OnInitAccessForExecuteFileActionsOnUIThread but not in
> InitHandlerHostFileAccessPermissions. By binding these parameters here I avoid
> unnecessarily passing the parameters through ExecuteFileActionsOnUIThread.

Yeah, but this way it harder to follow what's going on.
I'd rather if we create the callback near the place the function that owns the
callback is invoked..

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:562: Done(false);
this can be extracted to a separate function..

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.h:110: const
FileDefinitionList& file_list,
this should probably be in the previoius line

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2049: get_file_callback,
indent

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system.cc:5153: GetResolvedFileByPath(
I guess we are safe against deadlock now (kinaba's TODO)?

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_util.cc:303: if (!callback.is_null())
I don't think this method makes much sense without a callback..
maybe DCHECK(!callback.is_null()) would be more suitable, same in
OnGetFileInfoForInsertGDataCachePathsPermissions

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_util.h (right):

https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_util.h:83: // point into the output vector
|cache_paths|.
can you add a comment that the caller has to ensure that |cache_paths| lives at
least until the callback was invoked?

[tbarzic, 2012-06-08 17:59:46]

On 2012/06/08 17:59:13, tbarzic wrote:
>
https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
> File chrome/browser/chromeos/extensions/file_handler_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6005/chrome/browser/chro...
> chrome/browser/chromeos/extensions/file_handler_util.cc:365: executor_,
> On 2012/06/08 00:22:05, hshi1 wrote:
> > Because several parameters (such as file_system_name and file_system_root)
are
> > only used by OnInitAccessForExecuteFileActionsOnUIThread but not in
> > InitHandlerHostFileAccessPermissions. By binding these parameters here I
avoid
> > unnecessarily passing the parameters through ExecuteFileActionsOnUIThread.
> 
> Yeah, but this way it harder to follow what's going on.
> I'd rather if we create the callback near the place the function that owns the
> callback is invoked..
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> File chrome/browser/chromeos/extensions/file_handler_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> chrome/browser/chromeos/extensions/file_handler_util.cc:562: Done(false);
> this can be extracted to a separate function..
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> File chrome/browser/chromeos/extensions/file_handler_util.h (right):
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> chrome/browser/chromeos/extensions/file_handler_util.h:110: const
> FileDefinitionList& file_list,
> this should probably be in the previoius line
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> chrome/browser/chromeos/gdata/gdata_file_system.cc:2049: get_file_callback,
> indent
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> chrome/browser/chromeos/gdata/gdata_file_system.cc:5153:
GetResolvedFileByPath(
> I guess we are safe against deadlock now (kinaba's TODO)?
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> File chrome/browser/chromeos/gdata/gdata_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> chrome/browser/chromeos/gdata/gdata_util.cc:303: if (!callback.is_null())
> I don't think this method makes much sense without a callback..
> maybe DCHECK(!callback.is_null()) would be more suitable, same in
> OnGetFileInfoForInsertGDataCachePathsPermissions
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> File chrome/browser/chromeos/gdata/gdata_util.h (right):
> 
>
https://chromiumcodereview.appspot.com/10543037/diff/6007/chrome/browser/chro...
> chrome/browser/chromeos/gdata/gdata_util.h:83: // point into the output vector
> |cache_paths|.
> can you add a comment that the caller has to ensure that |cache_paths| lives
at
> least until the callback was invoked?

btw. you probably want to specify --revision flag for the try job :)

[hshi1, 2012-06-08 18:21:41]

Please review Patch Set #4. Thanks!

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10543037/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:365: executor_,
On 2012/06/08 17:59:13, tbarzic wrote:
> On 2012/06/08 00:22:05, hshi1 wrote:
> > Because several parameters (such as file_system_name and file_system_root)
are
> > only used by OnInitAccessForExecuteFileActionsOnUIThread but not in
> > InitHandlerHostFileAccessPermissions. By binding these parameters here I
avoid
> > unnecessarily passing the parameters through ExecuteFileActionsOnUIThread.
> 
> Yeah, but this way it harder to follow what's going on.
> I'd rather if we create the callback near the place the function that owns the
> callback is invoked..

Done.

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:562: Done(false);
On 2012/06/08 17:59:13, tbarzic wrote:
> this can be extracted to a separate function..

Done.

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.h:110: const
FileDefinitionList& file_list,
On 2012/06/08 17:59:13, tbarzic wrote:
> this should probably be in the previoius line

Done.

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2049: get_file_callback,
On 2012/06/08 17:59:13, tbarzic wrote:
> indent

Done.

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:5153: GetResolvedFileByPath(
On 2012/06/08 17:59:13, tbarzic wrote:
> I guess we are safe against deadlock now (kinaba's TODO)?

Yes because this is called from the GetFileInfoByPathAsync now.

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:303: if (!callback.is_null())
On 2012/06/08 17:59:13, tbarzic wrote:
> I don't think this method makes much sense without a callback..
> maybe DCHECK(!callback.is_null()) would be more suitable, same in
> OnGetFileInfoForInsertGDataCachePathsPermissions

Done.

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util.h (right):

http://codereview.chromium.org/10543037/diff/6007/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.h:83: // point into the output vector
|cache_paths|.
On 2012/06/08 17:59:13, tbarzic wrote:
> can you add a comment that the caller has to ensure that |cache_paths| lives
at
> least until the callback was invoked?

Done.

[satorux1, 2012-06-08 18:23:52]

GDataFileSystem  changes look good. Toni would be a better reviewer for other
pieces of code. I'd defer to him.

http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1993: file_info.get());
indentation is off.

http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util.h (right):

http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.h:83: // point into the output vector
|cache_paths|.
Please document about |callback|.

[hshi1, 2012-06-08 18:25:09]

These two should be fixed at Patch Set #4. Thanks!

http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1993: file_info.get());
On 2012/06/08 18:23:52, satorux1 wrote:
> indentation is off.

Done.

http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util.h (right):

http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.h:83: // point into the output vector
|cache_paths|.
On 2012/06/08 18:23:52, satorux1 wrote:
> Please document about |callback|.

Done.

[tbarzic, 2012-06-08 18:30:13]

On 2012/06/08 18:25:09, hshi1 wrote:
> These two should be fixed at Patch Set #4. Thanks!
> 
>
http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):
> 
>
http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_file_system.cc:1993: file_info.get());
> On 2012/06/08 18:23:52, satorux1 wrote:
> > indentation is off.
> 
> Done.
> 
>
http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_util.h (right):
> 
>
http://codereview.chromium.org/10543037/diff/7020/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_util.h:83: // point into the output vector
> |cache_paths|.
> On 2012/06/08 18:23:52, satorux1 wrote:
> > Please document about |callback|.
> 
> Done.

LGTM

[commit-bot: I haz the power, 2012-06-08 18:41:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10543037/1014

[commit-bot: I haz the power, 2012-06-08 19:50:06]

Change committed as 141262