gdata: Convert public synchronous functions in GDataFileSystem to asynchronous.

chrome/browser/chromeos/gdata/gdata_util.cc

Index: chrome/browser/chromeos/gdata/gdata_util.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_util.cc b/chrome/browser/chromeos/gdata/gdata_util.cc
index b69f120bfec7f6e622376223e957d1ceb4811cce..edff10a2b20772d9169756280e97ca0d9b92863b 100644
--- a/chrome/browser/chromeos/gdata/gdata_util.cc
+++ b/chrome/browser/chromeos/gdata/gdata_util.cc
@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/chromeos/gdata/gdata_util.h"
-
 #include <string>
 #include <vector>
 #include <utility>
@@ -21,8 +19,10 @@
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/browser/chromeos/gdata/gdata.pb.h"
 #include "chrome/browser/chromeos/gdata/gdata_file_system.h"
 #include "chrome/browser/chromeos/gdata/gdata_system_service.h"
+#include "chrome/browser/chromeos/gdata/gdata_util.h"

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

Please move this back to the original place. 

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

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

why did you move this here??

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

Done.

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

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.

 #include "chrome/browser/chromeos/login/user.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -229,21 +229,21 @@ FilePath ExtractGDataPath(const FilePath& path) {
   return extracted;
 }
 
-void InsertGDataCachePathsPermissions(
-    Profile* profile,
-    const FilePath& gdata_path,
-    std::vector<std::pair<FilePath, int> >* cache_paths ) {
-  DCHECK(cache_paths);
-
-  GDataFileSystem* file_system = GetGDataFileSystem(profile);
-  if (!file_system)
-    return;
-
-  GDataFileProperties file_properties;
-  file_system->GetFileInfoByPath(gdata_path, &file_properties);
-
-  std::string resource_id = file_properties.resource_id;
-  std::string file_md5 = file_properties.file_md5;
+void OnGetFileInfoForInsertGDataCachePathsPermissions(

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

Please move this to anonymous namespace. Please write a function comment.

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

Done.

+    GDataFileSystem* file_system,

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

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..

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

I'm going to change this to obtain file_system pointer from profile instead.
On 2012/06/07 22:47:56, tbarzic wrote:

+    std::vector<std::pair<FilePath, int> >* cache_paths,
+    const base::Closure& callback,
+    base::PlatformFileError error,
+    scoped_ptr<GDataFileProto> file_info) {
+  std::string resource_id;
+  std::string file_md5;
+
+  // TODO(hshi): GetFileInfoByPathAsync may fail when the gdata file is not

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

Are you sure? AFAIK GetFileInfoByPath doesn't check gdata cache (GetFileByPath
does)?

+  // present in cache. For now just grant permission to the entire cache subdir.

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

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..

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

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.

+  if (error == base::PLATFORM_FILE_OK && file_info.get()) {
+    resource_id = file_info->gdata_entry().resource_id();
+    file_md5 = file_info->file_md5();
+  }
 
   // We check permissions for raw cache file paths only for read-only
   // operations (when fileEntry.file() is called), so read only permissions
@@ -271,6 +271,39 @@ void InsertGDataCachePathsPermissions(
           GDataCache::CACHE_TYPE_TMP,
           GDataFileSystem::CACHED_FILE_FROM_SERVER),
       kReadOnlyFilePermissions));
+
+  callback.Run();
+}
+
+void InsertGDataCachePathsPermissions(
+    Profile* profile,
+    scoped_ptr<std::vector<FilePath> > gdata_paths,
+    std::vector<std::pair<FilePath, int> >* cache_paths,
+    const base::Closure& callback) {
+  DCHECK(profile);
+  DCHECK(gdata_paths.get());
+  DCHECK(cache_paths);
+
+  GDataFileSystem* file_system = GetGDataFileSystem(profile);
+  if (!file_system || gdata_paths->empty()) {
+    if (!callback.is_null())
+      callback.Run();
+    return;
+  }
+
+  FilePath gdata_path = gdata_paths->back();
+  gdata_paths->pop_back();
+

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

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]

Done.

+  file_system->GetFileInfoByPathAsync(

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

You are creating a loop using the callback, which is rather tricky. Please write
some comment about it.

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

Done.

+      gdata_path,
+      base::Bind(&OnGetFileInfoForInsertGDataCachePathsPermissions,
+                 file_system,
+                 cache_paths,
+                 base::Bind(&InsertGDataCachePathsPermissions,
+                             profile,
+                             base::Passed(&gdata_paths),
+                             cache_paths,
+                             callback)));
 }
 
 bool IsGDataAvailable(Profile* profile) {