Refactor FileSystemOperation to take callback for each method.

webkit/tools/test_shell/simple_file_writer.cc

Index: webkit/tools/test_shell/simple_file_writer.cc
diff --git a/webkit/tools/test_shell/simple_file_writer.cc b/webkit/tools/test_shell/simple_file_writer.cc
index cdd36b1d38b5c6bbb1f38c9719555d54ce040aaa..6bb69cda657de884610cbb19bd42196b7cd93905 100644
--- a/webkit/tools/test_shell/simple_file_writer.cc
+++ b/webkit/tools/test_shell/simple_file_writer.cc
@@ -8,13 +8,11 @@
 #include "base/logging.h"
 #include "base/message_loop_proxy.h"
 #include "net/url_request/url_request_context.h"
-#include "webkit/fileapi/file_system_callback_dispatcher.h"
 #include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/file_system_operation_interface.h"
 #include "webkit/glue/webkit_glue.h"
 #include "webkit/tools/test_shell/simple_resource_loader_bridge.h"
 
-using fileapi::FileSystemCallbackDispatcher;
 using fileapi::FileSystemContext;
 using fileapi::FileSystemOperationInterface;
 using fileapi::WebFileWriterBase;
@@ -53,7 +51,8 @@ class SimpleFileWriter::IOThreadProxy
     }
     DCHECK(!operation_);
     operation_ = GetNewOperation(path);
-    operation_->Truncate(path, offset);
+    operation_->Truncate(path, offset,
+                         base::Bind(&IOThreadProxy::DidFinish, this));
   }
 
   void Write(const GURL& path, const GURL& blob_url, int64 offset) {
@@ -66,7 +65,8 @@ class SimpleFileWriter::IOThreadProxy
     DCHECK(request_context_);
     DCHECK(!operation_);
     operation_ = GetNewOperation(path);
-    operation_->Write(request_context_, path, blob_url, offset);
+    operation_->Write(request_context_, path, blob_url, offset,
+                      base::Bind(&IOThreadProxy::DidWrite, this));
   }
 
   void Cancel() {
@@ -80,64 +80,12 @@ class SimpleFileWriter::IOThreadProxy
       DidFail(base::PLATFORM_FILE_ERROR_INVALID_OPERATION);
       return;
     }
-    operation_->Cancel(CallbackDispatcher::Create(this));
+    operation_->Cancel(base::Bind(&IOThreadProxy::DidFinish, this));
   }
 
  private:
-  // Inner class to receive callbacks from FileSystemOperation.
-  class CallbackDispatcher : public FileSystemCallbackDispatcher {
-   public:
-    // An instance of this class must be created by Create()
-    // (so that we do not leak ownerships).
-    static scoped_ptr<FileSystemCallbackDispatcher> Create(
-        IOThreadProxy* proxy) {
-      return scoped_ptr<FileSystemCallbackDispatcher>(
-          new CallbackDispatcher(proxy));
-    }
-
-    ~CallbackDispatcher() {
-      proxy_->ClearOperation();
-    }
-
-    virtual void DidSucceed() {
-      proxy_->DidSucceed();
-    }
-
-    virtual void DidFail(base::PlatformFileError error_code) {
-      proxy_->DidFail(error_code);
-    }
-
-    virtual void DidWrite(int64 bytes, bool complete) {
-      proxy_->DidWrite(bytes, complete);
-    }
-
-    virtual void DidReadMetadata(
-        const base::PlatformFileInfo&,
-        const FilePath&) {
-      NOTREACHED();
-    }
-
-    virtual void DidReadDirectory(
-        const std::vector<base::FileUtilProxy::Entry>& entries,
-        bool has_more) {
-      NOTREACHED();
-    }
-
-    virtual void DidOpenFileSystem(
-        const std::string& name,
-        const GURL& root) {
-      NOTREACHED();
-    }
-
-   private:
-    explicit CallbackDispatcher(IOThreadProxy* proxy) : proxy_(proxy) {}
-    scoped_refptr<IOThreadProxy> proxy_;
-  };
-
   FileSystemOperationInterface* GetNewOperation(const GURL& path) {
-    // The FileSystemOperation takes ownership of the CallbackDispatcher.
-    return file_system_context_->CreateFileSystemOperation(
-        path, CallbackDispatcher::Create(this), io_thread_);
+    return file_system_context_->CreateFileSystemOperation(path, io_thread_);
   }
 
   void DidSucceed() {
@@ -162,11 +110,11 @@ class SimpleFileWriter::IOThreadProxy
       simple_writer_->DidFail(error_code);
   }
 
-  void DidWrite(int64 bytes, bool complete) {
+  void DidWriteMain(int64 bytes, bool complete) {

[kinuko, 2012/02/10 05:34:09]

If this is only called from DidWrite maybe we could just put the body of this
method in DidWrite?

[kinaba, 2012/02/10 08:22:58]

I'd rather keep it in the current form. DidWrite (more specifically,
ClearOperation) is supposed to be called from IO thread, while DidWriteMain need
to forward itself to the main thread. Mixing them up into one method may make
the code hard to understand the thread interaction.

To make the intent (separating methods at the thread boundary), I renamed
DidSucceed, DidFail, DidWriteMain. Hope it improves clarity.

[kinuko, 2012/02/10 08:49:14]

Ah ok.  Could we rename DidWriteMain to DidWriteOnMainThread or something then
(per convention)?  I wasn't very sure what this 'Main' meant at first.

[kinaba, 2012/02/10 09:15:28]

Roger.

     if (!main_thread_->BelongsToCurrentThread()) {
       main_thread_->PostTask(
           FROM_HERE,
-          base::Bind(&IOThreadProxy::DidWrite, this, bytes, complete));
+          base::Bind(&IOThreadProxy::DidWriteMain, this, bytes, complete));
       return;
     }
     if (simple_writer_)
@@ -178,6 +126,25 @@ class SimpleFileWriter::IOThreadProxy
     operation_ = NULL;
   }
 
+  void DidFinish(base::PlatformFileError result) {
+    if (result == base::PLATFORM_FILE_OK)
+      DidSucceed();
+    else
+      DidFail(result);
+    ClearOperation();
+  }
+
+  void DidWrite(base::PlatformFileError result, int64 bytes, bool complete) {
+    if (result == base::PLATFORM_FILE_OK) {
+      DidWriteMain(bytes, complete);
+      if (complete)
+        ClearOperation();
+    } else {
+      DidFail(result);
+      ClearOperation();
+    }
+  }
+
   scoped_refptr<base::MessageLoopProxy> io_thread_;
   scoped_refptr<base::MessageLoopProxy> main_thread_;