Refactor FileSystem

content/renderer/pepper/pepper_file_io_host.cc

Index: content/renderer/pepper/pepper_file_io_host.cc
diff --git a/content/renderer/pepper/pepper_file_io_host.cc b/content/renderer/pepper/pepper_file_io_host.cc
index b96edf509826d4d4d202f2ba8dd1a70495666792..aa5951016816a33160ffa51ceed90b5a8069c6fd 100644
--- a/content/renderer/pepper/pepper_file_io_host.cc
+++ b/content/renderer/pepper/pepper_file_io_host.cc
@@ -5,11 +5,13 @@
 #include "content/renderer/pepper/pepper_file_io_host.h"
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_util_proxy.h"
 #include "content/public/common/content_client.h"
 #include "content/public/renderer/content_renderer_client.h"
+#include "content/renderer/pepper/null_file_system_callback_dispatcher.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/host/dispatch_host_message.h"
 #include "ppapi/host/ppapi_host.h"
@@ -17,7 +19,6 @@
 #include "ppapi/shared_impl/file_type_conversion.h"
 #include "ppapi/shared_impl/time_conversion.h"
 #include "ppapi/thunk/enter.h"
-#include "webkit/fileapi/file_system_callback_dispatcher.h"
 #include "webkit/plugins/ppapi/file_callbacks.h"
 #include "webkit/plugins/ppapi/host_globals.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
@@ -44,10 +45,10 @@ namespace {
 // partial read.
 static const int32_t kMaxReadSize = 32 * 1024 * 1024;  // 32MB
 
-typedef base::Callback<void (base::PlatformFileError)> PlatformGeneralCallback;
+typedef base::Callback<void (int32_t)> PlatformGeneralCallback;
 
 class PlatformGeneralCallbackTranslator
-    : public fileapi::FileSystemCallbackDispatcher {
+    : public NullFileSystemCallbackDispatcher {
  public:
   explicit PlatformGeneralCallbackTranslator(
       const PlatformGeneralCallback& callback)
@@ -59,38 +60,8 @@ class PlatformGeneralCallbackTranslator
     callback_.Run(base::PLATFORM_FILE_OK);
   }
 
-  virtual void DidReadMetadata(const base::PlatformFileInfo& file_info,
-                               const base::FilePath& platform_path) OVERRIDE {
-    NOTREACHED();
-  }
-
-  virtual void DidCreateSnapshotFile(
-      const base::PlatformFileInfo& file_info,
-      const base::FilePath& platform_path) OVERRIDE {
-    NOTREACHED();
-  }
-
-  virtual void DidReadDirectory(
-      const std::vector<base::FileUtilProxy::Entry>& entries,
-      bool has_more) OVERRIDE {
-    NOTREACHED();
-  }
-
-  virtual void DidOpenFileSystem(const std::string& name,
-                                 const GURL& root) OVERRIDE {
-    NOTREACHED();
-  }
-
-  virtual void DidFail(base::PlatformFileError error_code) OVERRIDE {
-    callback_.Run(error_code);
-  }
-
-  virtual void DidWrite(int64 bytes, bool complete) OVERRIDE {
-    NOTREACHED();
-  }
-
-  virtual void DidOpenFile(base::PlatformFile file) OVERRIDE {
-    NOTREACHED();
+  virtual void OnError(int32_t pp_error) OVERRIDE {

[yzshen1, 2013/04/09 17:25:42]

I think this OnError() actually complicates things and is error-prone. For
example, you are mixing PlatformFileError and pp errors (line 60 and line 64).
Besides, you have to have ExecutePlatformGeneralCallback and
ExecuteGeneralCallback.

I suggest to go back to the previous implementation of using
DidFail(base::PlatformFileError).

[victorhsieh, 2013/04/09 18:02:45]

Agree.  I think it's because there are two kinds of access in this file.  One is
to call platform version, and the other calls delegate.  Reverted to original
implementation to make it simple.

+    callback_.Run(pp_error);
   }
 
  private:
@@ -248,7 +219,7 @@ int32_t PepperFileIOHost::OnHostMsgTouch(
             PPTimeToTime(last_access_time),
             PPTimeToTime(last_modified_time),
             new PlatformGeneralCallbackTranslator(
-                base::Bind(&PepperFileIOHost::ExecutePlatformGeneralCallback,
+                base::Bind(&PepperFileIOHost::ExecuteGeneralCallback,
                            weak_factory_.GetWeakPtr(),
                            context->MakeReplyMessageContext()))))
       return PP_ERROR_FAILED;
@@ -353,7 +324,7 @@ int32_t PepperFileIOHost::OnHostMsgSetLength(
     if (!plugin_delegate_->SetLength(
             file_system_url_, length,
             new PlatformGeneralCallbackTranslator(
-                base::Bind(&PepperFileIOHost::ExecutePlatformGeneralCallback,
+                base::Bind(&PepperFileIOHost::ExecuteGeneralCallback,
                            weak_factory_.GetWeakPtr(),
                            context->MakeReplyMessageContext()))))
       return PP_ERROR_FAILED;
@@ -420,7 +391,7 @@ int32_t PepperFileIOHost::OnHostMsgWillWrite(
 
   if (!quota_file_io_->WillWrite(
           offset, bytes_to_write,
-          base::Bind(&PepperFileIOHost::ExecutePlatformWillWriteCallback,
+          base::Bind(&PepperFileIOHost::ExecutePlatformWriteCallback,
                      weak_factory_.GetWeakPtr(),
                      context->MakeReplyMessageContext())))
     return PP_ERROR_FAILED;
@@ -505,6 +476,14 @@ void PepperFileIOHost::ExecutePlatformGeneralCallback(
   state_manager_.SetOperationFinished();
 }
 
+void PepperFileIOHost::ExecuteGeneralCallback(
+    ppapi::host::ReplyMessageContext reply_context,
+    int32_t pp_error) {
+  reply_context.params.set_result(pp_error);
+  host()->SendReply(reply_context, PpapiPluginMsg_FileIO_GeneralReply());
+  state_manager_.SetOperationFinished();
+}
+
 void PepperFileIOHost::ExecutePlatformOpenFileCallback(
     ppapi::host::ReplyMessageContext reply_context,
     base::PlatformFileError error_code,
@@ -580,16 +559,6 @@ void PepperFileIOHost::ExecutePlatformWriteCallback(
     ppapi::host::ReplyMessageContext reply_context,
     base::PlatformFileError error_code,
     int bytes_written) {
-  int32_t pp_error = ::ppapi::PlatformFileErrorToPepperError(error_code);
-  reply_context.params.set_result(ErrorOrByteNumber(pp_error, bytes_written));
-  host()->SendReply(reply_context, PpapiPluginMsg_FileIO_GeneralReply());
-  state_manager_.SetOperationFinished();
-}
-
-void PepperFileIOHost::ExecutePlatformWillWriteCallback(
-    ppapi::host::ReplyMessageContext reply_context,
-    base::PlatformFileError error_code,
-    int bytes_written) {
   // On the plugin side, the callback expects a parameter with different meaning
   // depends on whether is negative or not. It is the result here. We translate
   // for the callback.