Refactor FileSystem

content/renderer/pepper/pepper_file_io_host.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 "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"
 #include "ppapi/proxy/ppapi_messages.h"
 #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"
 #include "webkit/plugins/ppapi/ppb_file_ref_impl.h"
 #include "webkit/plugins/ppapi/quota_file_io.h"
 
 namespace content {
 
 using ppapi::FileIOStateManager;
 using ppapi::PPTimeToTime;
 using ppapi::TimeToPPTime;
 using ppapi::host::ReplyMessageContext;
 using ppapi::thunk::EnterResourceNoLock;
 using ppapi::thunk::PPB_FileRef_API;
 using webkit::ppapi::PPB_FileRef_Impl;
 using webkit::ppapi::PluginDelegate;
 
 namespace {
 
 // The maximum size we'll support reading in one chunk. The renderer process
 // must allocate a buffer sized according to the request of the plugin. To
 // keep things from getting out of control, we cap the read size to this value.
 // This should generally be OK since the API specifies that it may perform a
 // 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)
     : callback_(callback) {}
 
   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:
   PlatformGeneralCallback callback_;
 };
 
 int32_t ErrorOrByteNumber(int32_t pp_error, int32_t byte_number) {
   // On the plugin side, some callbacks expect a parameter that means different
   // things depending on whether is negative or not.  We translate for those
   // callbacks here.
@@ skipping 137 matching lines @@
 
   if (!plugin_delegate_)
     return PP_ERROR_FAILED;
 
   if (file_system_type_ != PP_FILESYSTEMTYPE_EXTERNAL) {
     if (!plugin_delegate_->Touch(
             file_system_url_,
             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;
     state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
     return PP_OK_COMPLETIONPENDING;
   }
 
   // TODO(nhiroki): fix a failure of FileIO.Touch for an external filesystem on
   // Mac and Linux due to sandbox restrictions (http://crbug.com/101128).
   if (!base::FileUtilProxy::Touch(
@@ skipping 84 matching lines @@
   if (rv != PP_OK)
     return rv;
 
   if (!plugin_delegate_)
     return PP_ERROR_FAILED;
 
   if (file_system_type_ != PP_FILESYSTEMTYPE_EXTERNAL) {
     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;
   } else {
     // TODO(nhiroki): fix a failure of FileIO.SetLength for an external
     // filesystem on Mac due to sandbox restrictions (http://crbug.com/156077).
     if (!base::FileUtilProxy::Truncate(
             plugin_delegate_->GetFileThreadMessageLoopProxy(), file_, length,
             base::Bind(&PepperFileIOHost::ExecutePlatformGeneralCallback,
                        weak_factory_.GetWeakPtr(),
@@ skipping 46 matching lines @@
   int32_t rv = state_manager_.CheckOperationState(
       FileIOStateManager::OPERATION_EXCLUSIVE, true);
   if (rv != PP_OK)
     return rv;
 
   if (!quota_file_io_.get())
     return PP_OK;
 
   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;
 
   state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
   return PP_OK_COMPLETIONPENDING;
 }
 
 int32_t PepperFileIOHost::OnHostMsgWillSetLength(
     ppapi::host::HostMessageContext* context,
@@ skipping 64 matching lines @@
 
 void PepperFileIOHost::ExecutePlatformGeneralCallback(
     ppapi::host::ReplyMessageContext reply_context,
     base::PlatformFileError error_code) {
   reply_context.params.set_result(
       ::ppapi::PlatformFileErrorToPepperError(error_code));
   host()->SendReply(reply_context, PpapiPluginMsg_FileIO_GeneralReply());
   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,
     base::PassPlatformFile file) {
   int32_t pp_error = ::ppapi::PlatformFileErrorToPepperError(error_code);
   if (pp_error == PP_OK)
     state_manager_.SetOpenSucceed();
 
   DCHECK(file_ == base::kInvalidPlatformFileValue);
   file_ = file.ReleaseValue();
@@ skipping 55 matching lines @@
     buffer.append(data, bytes_read);
   reply_context.params.set_result(ErrorOrByteNumber(pp_error, bytes_read));
   host()->SendReply(reply_context, PpapiPluginMsg_FileIO_ReadReply(buffer));
   state_manager_.SetOperationFinished();
 }
 
 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.
   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();
 }
 
 }  // namespace content