gdata: Convert public synchronous functions in GDataFileSystem to asynchronous.

chrome/browser/chromeos/gdata/gdata_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // 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>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/stringprintf.h"
 #include "base/string_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "net/base/escape.h"
 #include "third_party/libxml/chromium/libxml_utils.h"
@@ skipping 186 matching lines @@
 
   // -1 to include 'drive'.
   FilePath extracted;
   for (size_t i = arraysize(kGDataMountPointPathComponents) - 1;
        i < components.size(); ++i) {
     extracted = extracted.Append(components[i]);
   }
   return extracted;
 }
 
-void InsertGDataCachePathsPermissions(
-    Profile* profile,
-    const FilePath& gdata_path,
-    std::vector<std::pair<FilePath, int> >* cache_paths ) {
-  DCHECK(cache_paths);
+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;
 
-  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;
+  // 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
   // should be sufficient for all cache paths. For the rest of supported
   // operations the file access check is done for drive/ paths.
   cache_paths->push_back(std::make_pair(
       file_system->GetCacheFilePath(resource_id, file_md5,
           GDataCache::CACHE_TYPE_PERSISTENT,
           GDataFileSystem::CACHED_FILE_FROM_SERVER),
       kReadOnlyFilePermissions));
   // TODO(tbarzic): When we start supporting openFile operation, we may have to
   // change permission for localy modified files to match handler's permissions.
   cache_paths->push_back(std::make_pair(
       file_system->GetCacheFilePath(resource_id, file_md5,
           GDataCache::CACHE_TYPE_PERSISTENT,
           GDataFileSystem::CACHED_FILE_LOCALLY_MODIFIED),
      kReadOnlyFilePermissions));
   cache_paths->push_back(std::make_pair(
       file_system->GetCacheFilePath(resource_id, file_md5,
           GDataCache::CACHE_TYPE_PERSISTENT,
           GDataFileSystem::CACHED_FILE_MOUNTED),
      kReadOnlyFilePermissions));
   cache_paths->push_back(std::make_pair(
       file_system->GetCacheFilePath(resource_id, file_md5,
           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) {
   if (!chromeos::UserManager::Get()->IsUserLoggedIn() ||
       chromeos::UserManager::Get()->IsLoggedInAsGuest() ||
       chromeos::UserManager::Get()->IsLoggedInAsDemoUser())
     return false;
 
   // Do not allow GData for incognito windows / guest mode.
   if (profile->IsOffTheRecord())
@@ skipping 66 matching lines @@
   // Assign the extracted extensions to md5 and extra_extension.
   int extension_count = extensions.size();
   *md5 = (extension_count > 0) ? extensions[extension_count - 1] :
                                  std::string();
   *extra_extension = (extension_count > 1) ? extensions[extension_count - 2] :
                                              std::string();
 }
 
 }  // namespace util
 }  // namespace gdata