Cleanup: Deprecate FileSystemCallbackDispatcher

webkit/plugins/ppapi/ppb_file_ref_impl.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 "webkit/plugins/ppapi/ppb_file_ref_impl.h"
 
 #include "base/platform_file.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/escape.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/shared_impl/file_type_conversion.h"
 #include "ppapi/shared_impl/time_conversion.h"
 #include "ppapi/shared_impl/var.h"
 #include "ppapi/thunk/enter.h"
 #include "ppapi/thunk/ppb_file_system_api.h"
+#include "webkit/fileapi/file_system_util.h"
 #include "webkit/plugins/ppapi/common.h"
-#include "webkit/plugins/ppapi/file_callbacks.h"
 #include "webkit/plugins/ppapi/plugin_delegate.h"
 #include "webkit/plugins/ppapi/plugin_module.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 #include "webkit/plugins/ppapi/resource_helper.h"
 
 using ppapi::HostResource;
 using ppapi::PPB_FileRef_CreateInfo;
 using ppapi::PPTimeToTime;
 using ppapi::StringVar;
 using ppapi::TrackedCallback;
@@ skipping 53 matching lines @@
 
   // There should always be a leading slash at least!
   size_t pos = path.rfind('/');
   CHECK(pos != std::string::npos);
   return path.substr(pos + 1);
 }
 
 void IgnoreCloseCallback(base::PlatformFileError error_code) {
 }
 
+void PlatformFileInfoToPPFileInfo(
+    const base::PlatformFileInfo& file_info,
+    PP_FileSystemType file_system_type,
+    PP_FileInfo* info) {
+  DCHECK(info);
+
+  info->size = file_info.size;
+  info->system_type = file_system_type;
+  info->creation_time = ::ppapi::TimeToPPTime(file_info.creation_time);
+  info->last_access_time = ::ppapi::TimeToPPTime(file_info.last_accessed);
+  info->last_modified_time = ::ppapi::TimeToPPTime(file_info.last_modified);
+
+  if (file_info.is_symbolic_link) {
+    // Only external filesystem may have symbolic link files.
+    DCHECK_EQ(PP_FILESYSTEMTYPE_EXTERNAL, file_system_type);
+    info->type = PP_FILETYPE_OTHER;
+  } else if (file_info.is_directory) {
+    info->type = PP_FILETYPE_DIRECTORY;
+  } else {
+    info->type = PP_FILETYPE_REGULAR;
+  }
+}
+
 void GetFileInfoCallback(
     scoped_refptr<base::TaskRunner> task_runner,
     base::PlatformFile file,
     linked_ptr<PP_FileInfo> info,
     scoped_refptr<TrackedCallback> callback,
     base::PlatformFileError error_code,
     const base::PlatformFileInfo& file_info) {
   base::FileUtilProxy::Close(
       task_runner, file, base::Bind(&IgnoreCloseCallback));
 
   if (!TrackedCallback::IsPending(callback))
     return;
 
   int32_t pp_error = ::ppapi::PlatformFileErrorToPepperError(error_code);
-  if (pp_error != PP_OK)
+  if (pp_error != PP_OK) {
     callback->Run(pp_error);
+    return;
+  }
 
-  info->size = file_info.size;
-  if (file_info.is_symbolic_link)
-    info->type = PP_FILETYPE_OTHER;
-  else if (file_info.is_directory)
-    info->type = PP_FILETYPE_DIRECTORY;
-  else
-    info->type = PP_FILETYPE_REGULAR;
+  PlatformFileInfoToPPFileInfo(
+      file_info, PP_FILESYSTEMTYPE_EXTERNAL, info.get());
 
-  info->system_type = PP_FILESYSTEMTYPE_EXTERNAL;
-  info->creation_time = file_info.creation_time.ToDoubleT();
-  info->last_access_time = file_info.last_accessed.ToDoubleT();
-  info->last_modified_time = file_info.last_modified.ToDoubleT();
   callback->Run(PP_OK);
 }
 
 void QueryCallback(scoped_refptr<base::TaskRunner> task_runner,
                    linked_ptr<PP_FileInfo> info,
                    scoped_refptr<TrackedCallback> callback,
                    base::PlatformFileError error_code,
                    base::PassPlatformFile passed_file) {
   if (!TrackedCallback::IsPending(callback))
     return;
 
   int32_t pp_error = ::ppapi::PlatformFileErrorToPepperError(error_code);
-  if (pp_error != PP_OK)
+  if (pp_error != PP_OK) {
     callback->Run(pp_error);
+    return;

[kinuko, 2013/05/15 13:58:03]

I think this needs return here, but let me know if this's wrong...

[yzshen1, 2013/05/15 18:13:54]

It makes sense to me. Thanks!
On 2013/05/15 13:58:03, kinuko wrote:

+  }
   base::PlatformFile file = passed_file.ReleaseValue();
 
   if (!base::FileUtilProxy::GetFileInfoFromPlatformFile(
           task_runner, file,
           base::Bind(&GetFileInfoCallback, task_runner, file, info,
                      callback))) {
     base::FileUtilProxy::Close(
         task_runner, file, base::Bind(&IgnoreCloseCallback));
     callback->Run(PP_ERROR_FAILED);
   }
 }
 
+void DidReadMetadata(
+    scoped_refptr< ::ppapi::TrackedCallback> callback,
+    linked_ptr<PP_FileInfo> info,
+    PP_FileSystemType file_system_type,
+    const base::PlatformFileInfo& file_info,
+    const base::FilePath& unused) {
+  if (callback->completed())

[yzshen1, 2013/05/15 18:13:54]

nit: it is more common to use IsPending since it also checks null.

[kinuko, 2013/05/16 09:29:38]

Done.

+    return;
+
+  PlatformFileInfoToPPFileInfo(file_info, file_system_type, info.get());
+  callback->Run(PP_OK);
+}
+
+void DidReadDirectory(
+    scoped_refptr< ::ppapi::TrackedCallback> callback,
+    PPB_FileRef_Impl* dir_ref,
+    linked_ptr<std::vector< ::ppapi::PPB_FileRef_CreateInfo> > dir_files,
+    linked_ptr<std::vector<PP_FileType> > dir_file_types,
+    const std::vector<base::FileUtilProxy::Entry>& entries,
+    bool has_more) {
+  if (callback->completed())
+    return;
+
+  // The current filesystem backend always returns false.
+  DCHECK(!has_more);
+
+  DCHECK(dir_ref);
+  DCHECK(dir_files.get());
+  DCHECK(dir_file_types.get());
+
+  std::string dir_path = dir_ref->GetCreateInfo().path;
+  if (dir_path.empty() || dir_path[dir_path.size() - 1] != '/')
+    dir_path += '/';
+
+  for (size_t i = 0; i < entries.size(); ++i) {
+    const base::FileUtilProxy::Entry& entry = entries[i];
+    scoped_refptr<PPB_FileRef_Impl> file_ref(PPB_FileRef_Impl::CreateInternal(
+        dir_ref->pp_instance(),
+        dir_ref->file_system_resource(),
+        dir_path + fileapi::FilePathToString(base::FilePath(entry.name))));
+    dir_files->push_back(file_ref->GetCreateInfo());
+    dir_file_types->push_back(
+        entry.is_directory ? PP_FILETYPE_DIRECTORY : PP_FILETYPE_REGULAR);
+    // Add a ref count on behalf of the plugin side.
+    file_ref->GetReference();
+  }
+  CHECK_EQ(dir_files->size(), dir_file_types->size());
+
+  callback->Run(PP_OK);
+}
+
+void DidFinishFileOperation(
+    scoped_refptr< ::ppapi::TrackedCallback> callback,
+    base::PlatformFileError error_code) {
+  if (callback->completed())
+    return;
+  callback->Run(::ppapi::PlatformFileErrorToPepperError(error_code));
+}
+
 }  // namespace
 
 PPB_FileRef_Impl::PPB_FileRef_Impl(const PPB_FileRef_CreateInfo& info,
                                    PP_Resource file_system)
     : PPB_FileRef_Shared(::ppapi::OBJECT_IS_IMPL, info),
       file_system_(file_system),
       external_file_system_path_() {
 }
 
 PPB_FileRef_Impl::PPB_FileRef_Impl(const PPB_FileRef_CreateInfo& info,
@@ skipping 85 matching lines @@
     PP_Bool make_ancestors,
     scoped_refptr<TrackedCallback> callback) {
   if (!IsValidNonExternalFileSystem())
     return PP_ERROR_NOACCESS;
 
   PluginInstance* plugin_instance = ResourceHelper::GetPluginInstance(this);
   if (!plugin_instance)
     return PP_ERROR_FAILED;
   if (!plugin_instance->delegate()->MakeDirectory(
           GetFileSystemURL(), PP_ToBool(make_ancestors),
-          new FileCallbacks(this, callback)))
+          base::Bind(&DidFinishFileOperation, callback)))
     return PP_ERROR_FAILED;
   return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t PPB_FileRef_Impl::Touch(PP_Time last_access_time,
                                 PP_Time last_modified_time,
                                 scoped_refptr<TrackedCallback> callback) {
   if (!IsValidNonExternalFileSystem())
     return PP_ERROR_NOACCESS;
 
   PluginInstance* plugin_instance = ResourceHelper::GetPluginInstance(this);
   if (!plugin_instance)
     return PP_ERROR_FAILED;
   if (!plugin_instance->delegate()->Touch(
           GetFileSystemURL(),
           PPTimeToTime(last_access_time),
           PPTimeToTime(last_modified_time),
-          new FileCallbacks(this, callback)))
+          base::Bind(&DidFinishFileOperation, callback)))
     return PP_ERROR_FAILED;
   return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t PPB_FileRef_Impl::Delete(scoped_refptr<TrackedCallback> callback) {
   if (!IsValidNonExternalFileSystem())
     return PP_ERROR_NOACCESS;
 
   PluginInstance* plugin_instance = ResourceHelper::GetPluginInstance(this);
   if (!plugin_instance)
     return PP_ERROR_FAILED;
   if (!plugin_instance->delegate()->Delete(
           GetFileSystemURL(),
-          new FileCallbacks(this, callback)))
+          base::Bind(&DidFinishFileOperation, callback)))
     return PP_ERROR_FAILED;
   return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t PPB_FileRef_Impl::Rename(PP_Resource new_pp_file_ref,
                                  scoped_refptr<TrackedCallback> callback) {
   EnterResourceNoLock<PPB_FileRef_API> enter(new_pp_file_ref, true);
   if (enter.failed())
     return PP_ERROR_BADRESOURCE;
   PPB_FileRef_Impl* new_file_ref =
       static_cast<PPB_FileRef_Impl*>(enter.object());
 
   if (!IsValidNonExternalFileSystem() ||
       file_system_ != new_file_ref->file_system_)
     return PP_ERROR_NOACCESS;
 
   // TODO(viettrungluu): Also cancel when the new file ref is destroyed?
   // http://crbug.com/67624
   PluginInstance* plugin_instance = ResourceHelper::GetPluginInstance(this);
   if (!plugin_instance)
     return PP_ERROR_FAILED;
   if (!plugin_instance->delegate()->Rename(
           GetFileSystemURL(), new_file_ref->GetFileSystemURL(),
-          new FileCallbacks(this, callback)))
+          base::Bind(&DidFinishFileOperation, callback)))
     return PP_ERROR_FAILED;
   return PP_OK_COMPLETIONPENDING;
 }
 
 PP_Var PPB_FileRef_Impl::GetAbsolutePath() {
   if (GetFileSystemType() != PP_FILESYSTEMTYPE_EXTERNAL)
     return GetPath();
   if (!external_path_var_) {
     external_path_var_ = new StringVar(
         external_file_system_path_.AsUTF8Unsafe());
@@ skipping 81 matching lines @@
     // Non-external file system
     if (!HasValidFileSystem())
       return PP_ERROR_NOACCESS;
 
     PluginInstance* plugin_instance = ResourceHelper::GetPluginInstance(this);
     PluginDelegate* delegate =
         plugin_instance ? plugin_instance->delegate() : NULL;
     if (!delegate)
       return PP_ERROR_FAILED;
 
+    PP_FileSystemType file_system_type =
+        delegate->GetFileSystemType(pp_instance(), file_system_);
     if (!plugin_instance->delegate()->Query(
             GetFileSystemURL(),
-            new FileCallbacks(this, callback, info,
-                              delegate->GetFileSystemType(pp_instance(),
-                                                          file_system_))))
+            base::Bind(&DidReadMetadata, callback, info, file_system_type),
+            base::Bind(&DidFinishFileOperation, callback)))
       return PP_ERROR_FAILED;
 
   }
   return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t PPB_FileRef_Impl::ReadDirectoryEntries(
     const PP_ArrayOutput& output,
     scoped_refptr<TrackedCallback> callback) {
   NOTREACHED();
   return PP_ERROR_FAILED;
 }
 
 int32_t PPB_FileRef_Impl::ReadDirectoryEntriesInHost(
     linked_ptr<std::vector< ::ppapi::PPB_FileRef_CreateInfo> > files,
     linked_ptr<std::vector<PP_FileType> > file_types,
     scoped_refptr<TrackedCallback> callback) {
   if (!IsValidNonExternalFileSystem())
     return PP_ERROR_NOACCESS;
 
   PluginInstance* plugin_instance = ResourceHelper::GetPluginInstance(this);
   if (!plugin_instance)
     return PP_ERROR_FAILED;
 
-  FileCallbacks::ReadDirectoryEntriesParams params;
-  params.dir_ref = this;
-  params.files = files;
-  params.file_types = file_types;
-
   if (!plugin_instance->delegate()->ReadDirectoryEntries(
           GetFileSystemURL(),
-          new FileCallbacks(this, callback, params)))
+          base::Bind(&DidReadDirectory,
+                     callback, base::Unretained(this), files, file_types),

[yzshen1, 2013/05/15 18:13:54]

Why using unretained here? I think it may die before the callback is run, right?

[michaeln, 2013/05/15 20:18:26]

I don't see where the old FileCallbacks did anytihng other than an 'unretained'
storage of this pointer value either. Was that a bug or valid code?

[yzshen1, 2013/05/15 20:26:49]

I think it is a bug.
But I agree that this is not a new bug introduced by this CL.

On 2013/05/15 20:18:26, michaeln wrote:

[kinuko, 2013/05/16 09:29:38]

Could TrackedCallback::IsPending(callback) catch if 'this' go away earlier? 
Anyway I'm submitting this but added a TODO comment.

+          base::Bind(&DidFinishFileOperation, callback)))
     return PP_ERROR_FAILED;
   return PP_OK_COMPLETIONPENDING;
 }
 
 }  // namespace ppapi
 }  // namespace webkit