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..895e6dd77cd72f59edff11a88c00f2c89940581e 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;

[yzshen1, 2013/04/11 17:04:57]

I think it is better to use PlatformFileError instead of int32_t here, because:
- it is more consistent, platform in the name indicates that the parameter is
base::PlatformFileError.

- if you make this change, you need to have both
ExecutePlatformGeneralCallback() and ExecuteGeneralCallback().

The previous code looks simpler. I am not sure why we need this change.

[victorhsieh, 2013/04/11 18:24:38]

Actually it's a pp_error.
I started this change when checking other files.  Lots of
FileSystemCallbackDispatchers just implement one or two methods and put
NOTREACHED in others (in this case, 6).  And almost all of them (except this
one) repeat to convert the file from platform error to pp error.  So I extract
the new class.  But turned out this class is different from others.

We can remove Null* if you think it's better.  I'm fine with both ways as
fileapi callback api will be changed later.

 
 class PlatformGeneralCallbackTranslator
-    : public fileapi::FileSystemCallbackDispatcher {
+    : public NullFileSystemCallbackDispatcher {
  public:
   explicit PlatformGeneralCallbackTranslator(
       const PlatformGeneralCallback& callback)
@@ -56,41 +57,11 @@ class PlatformGeneralCallbackTranslator
   virtual ~PlatformGeneralCallbackTranslator() {}
 
   virtual void DidSucceed() OVERRIDE {
-    callback_.Run(base::PLATFORM_FILE_OK);
+    callback_.Run(PP_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 DidFail(base::PlatformFileError platform_error) OVERRIDE {
+    callback_.Run(ppapi::PlatformFileErrorToPepperError(platform_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.