drive: Stop returning FilePath from GetResourceEntryById

chrome/browser/chromeos/drive/file_system.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/drive/file_system.h"
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/metrics/histogram.h"
@@ skipping 21 matching lines @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 
 using content::BrowserThread;
 
 namespace drive {
 namespace {
 
 //================================ Helper functions ============================
 
-// Helper function for binding |path| to GetResourceEntryWithFilePathCallback
-// and create GetResourceEntryCallback.
-void RunGetResourceEntryWithFilePathCallback(
-    const GetResourceEntryWithFilePathCallback& callback,
-    const base::FilePath& path,
-    FileError error,
-    scoped_ptr<ResourceEntry> entry) {
-  DCHECK(!callback.is_null());
-  callback.Run(error, path, entry.Pass());
-}
-
 // Callback for ResourceMetadata::GetLargestChangestamp.
 // |callback| must not be null.
 void OnGetLargestChangestamp(
     FileSystemMetadata metadata,  // Will be modified.
     const GetFilesystemMetadataCallback& callback,
     int64 largest_changestamp) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   metadata.largest_changestamp = largest_changestamp;
@@ skipping 109 matching lines @@
   observers_.AddObserver(observer);
 }
 
 void FileSystem::RemoveObserver(FileSystemObserver* observer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   observers_.RemoveObserver(observer);
 }
 
 void FileSystem::GetResourceEntryById(
     const std::string& resource_id,
-    const GetResourceEntryWithFilePathCallback& callback) {
+    const GetResourceEntryCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!resource_id.empty());
   DCHECK(!callback.is_null());
 
   resource_metadata_->GetResourceEntryByIdOnUIThread(
       resource_id,
       base::Bind(&FileSystem::GetResourceEntryByIdAfterGetEntry,
                  weak_ptr_factory_.GetWeakPtr(),
                  callback));
 }
 
 void FileSystem::GetResourceEntryByIdAfterGetEntry(
-    const GetResourceEntryWithFilePathCallback& callback,
+    const GetResourceEntryCallback& callback,
     FileError error,
-    const base::FilePath& file_path,
     scoped_ptr<ResourceEntry> entry) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
   if (error != FILE_ERROR_OK) {
-    callback.Run(error, base::FilePath(), scoped_ptr<ResourceEntry>());
+    callback.Run(error, scoped_ptr<ResourceEntry>());
     return;
   }
   DCHECK(entry.get());
 
-  CheckLocalModificationAndRun(
-      entry.Pass(),
-      base::Bind(&RunGetResourceEntryWithFilePathCallback,
-                 callback,
-                 file_path));
+  CheckLocalModificationAndRun(entry.Pass(), callback);
 }
 
 void FileSystem::TransferFileFromRemoteToLocal(
     const base::FilePath& remote_src_file_path,
     const base::FilePath& local_dest_file_path,
     const FileOperationCallback& callback) {
 
   operations_.TransferFileFromRemoteToLocal(remote_src_file_path,
                                             local_dest_file_path,
                                             callback);
@@ skipping 168 matching lines @@
 
 void FileSystem::GetFileByResourceId(
     const std::string& resource_id,
     const DriveClientContext& context,
     const GetFileCallback& get_file_callback,
     const google_apis::GetContentCallback& get_content_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!resource_id.empty());
   DCHECK(!get_file_callback.is_null());
 
-  resource_metadata_->GetResourceEntryByIdOnUIThread(
-      resource_id,
-      base::Bind(&FileSystem::GetFileByResourceIdAfterGetEntry,
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner_,
+      FROM_HERE,
+      base::Bind(&internal::ResourceMetadata::GetFilePath,

[satorux1, 2013/05/29 06:07:03]

This seems to be awkward. Why do we need to convert a resource ID to a file path
to get a file? IIRC, we use a resource ID to download a file from the server,
and use a resource ID to get a file from the file cache. If so, we don't need to
use a file path at all here.

+                 base::Unretained(resource_metadata_),
+                 resource_id),
+      base::Bind(&FileSystem::GetFileByResourceIdAfterGetFilePath,
                  weak_ptr_factory_.GetWeakPtr(),
                  context,
                  get_file_callback,
                  get_content_callback));
 }
 
-void FileSystem::GetFileByResourceIdAfterGetEntry(
+void FileSystem::GetFileByResourceIdAfterGetFilePath(
     const DriveClientContext& context,
     const GetFileCallback& get_file_callback,
     const google_apis::GetContentCallback& get_content_callback,
-    FileError error,
-    const base::FilePath& file_path,
-    scoped_ptr<ResourceEntry> entry) {
+    const base::FilePath& file_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!get_file_callback.is_null());
 
-  if (error != FILE_ERROR_OK) {
+  if (file_path.empty()) {
     get_file_callback.Run(FILE_ERROR_NOT_FOUND, base::FilePath(),
                           scoped_ptr<ResourceEntry>());
     return;
   }
 
   operations_.EnsureFileDownloaded(
       file_path,

[satorux1, 2013/05/29 06:07:03]

My feeling is that this function taking |file_path| is questionable. Can this
take a resource ID instead? If it's easy enough, you might want to fix this
problem in a separate patch beforehand.

[hashimoto, 2013/05/30 03:45:03]

Done.

[hashimoto, 2013/05/30 03:45:03]

Done.

       context,
       GetFileContentInitializedCallback(),
       get_content_callback,
       get_file_callback);
 }
 
 void FileSystem::GetFileContentByPath(
     const base::FilePath& file_path,
     const GetFileContentInitializedCallback& initialized_callback,
     const google_apis::GetContentCallback& get_content_callback,
@@ skipping 694 matching lines @@
     return;
   }
 
   PlatformFileInfoProto entry_file_info;
   util::ConvertPlatformFileInfoToResourceEntry(*file_info, &entry_file_info);
   *entry->mutable_file_info() = entry_file_info;
   callback.Run(FILE_ERROR_OK, entry.Pass());
 }
 
 }  // namespace drive