Make FileSystemOperation's lifetime more explicit.

webkit/fileapi/file_system_operation.cc

Index: webkit/fileapi/file_system_operation.cc
diff --git a/webkit/fileapi/file_system_operation.cc b/webkit/fileapi/file_system_operation.cc
index fe077581fb7a4d3ad4da81370ffce8696da61471..88783411b5774a4516d7b731e3a1d7c771b7f7d3 100644
--- a/webkit/fileapi/file_system_operation.cc
+++ b/webkit/fileapi/file_system_operation.cc
@@ -11,11 +11,72 @@
 
 namespace fileapi {
 
+namespace {
+
+// A helper callback dispatcher class to make it easy to control
+// FileSystemOperation's lifetime.  Calling any callback methods of this
+// class would delete the corresponding FileSystemOperation.
+class DispatchAndDeleteCallbackWrapper
+    : public FileSystemCallbackDispatcher {
+ public:
+  DispatchAndDeleteCallbackWrapper(
+      FileSystemOperation* operation,
+      FileSystemCallbackDispatcher* dispatcher)
+    : operation_(operation),
+      dispatcher_(dispatcher) { }
+
+  // Returns the wrapped dispatcher.
+  // This would be useful when the caller does not want to delete the operation.
+  FileSystemCallbackDispatcher* wrapped() const { return dispatcher_.get(); }
+
+  virtual void DidSucceed() {
+    dispatcher_->DidSucceed();
+    delete operation_;
+  }
+
+  virtual void DidReadMetadata(const base::PlatformFileInfo& file_info) {
+    dispatcher_->DidReadMetadata(file_info);
+    delete operation_;
+  }
+
+  virtual void DidReadDirectory(
+      const std::vector<base::FileUtilProxy::Entry>& entries,
+      bool has_more) {
+    dispatcher_->DidReadDirectory(entries, has_more);
+    delete operation_;
+  }
+
+  virtual void DidOpenFileSystem(const std::string& name,
+                                 const FilePath& root_path) {
+    dispatcher_->DidOpenFileSystem(name, root_path);
+    delete operation_;
+  }
+
+  virtual void DidFail(base::PlatformFileError error_code) {
+    dispatcher_->DidFail(error_code);
+    delete operation_;
+  }
+
+  virtual void DidWrite(int64 bytes, bool complete) {
+    dispatcher_->DidWrite(bytes, complete);
+    if (complete)
+      delete operation_;
+  }
+
+ private:
+  FileSystemOperation* operation_;
+  scoped_ptr<FileSystemCallbackDispatcher> dispatcher_;
+};
+
+}  // namespace
+
 FileSystemOperation::FileSystemOperation(
     FileSystemCallbackDispatcher* dispatcher,
     scoped_refptr<base::MessageLoopProxy> proxy)
     : proxy_(proxy),
-      dispatcher_(dispatcher),
+      destructive_dispatcher_(
+          new DispatchAndDeleteCallbackWrapper(
+              ALLOW_THIS_IN_INITIALIZER_LIST(this), dispatcher)),

[ericu, 2010/11/16 19:26:54]

This seems a bit odd to me.  We're creating a wrapper object that will most
likely be used only once, and then if we're actually going to reuse it [call for
a nondestructive one], we need to reach down inside it and return a private
member.  Why not just store the underlying FSCD and create a destructive
dispatcher only when asked for one?  That way the nondestructive accessor would
be simpler.

       callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       cancel_operation_(NULL) {
   DCHECK(dispatcher);
@@ -158,7 +219,7 @@ void FileSystemOperation::OnFileOpenedForWrite(
     base::PassPlatformFile file,
     bool created) {
   if (base::PLATFORM_FILE_OK != rv) {
-    dispatcher_->DidFail(rv);
+    destructive_dispatcher()->DidFail(rv);
     return;
   }
   file_writer_delegate_->Start(file.ReleaseValue(), blob_request_.get());
@@ -204,8 +265,8 @@ void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation) {
       blob_request_->Cancel();
 
     // This deletes us, and by proxy deletes file_writer_delegate_ if any.
-    dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
-    cancel_operation->dispatcher_->DidSucceed();
+    destructive_dispatcher()->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
+    cancel_operation->destructive_dispatcher()->DidSucceed();
   } else {
 #ifndef NDEBUG
     DCHECK(kOperationTruncate == pending_operation_);
@@ -214,15 +275,22 @@ void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation) {
     // since it's been proxied to another thread.  We need to save the
     // cancel_operation so that when the truncate returns, it can see that it's
     // been cancelled, report it, and report that the cancel has succeeded.
+    DCHECK(!cancel_operation_);
     cancel_operation_ = cancel_operation;
   }
 }
 
+FileSystemCallbackDispatcher*
+    FileSystemOperation::non_destructive_dispatcher() const {
+  return static_cast<DispatchAndDeleteCallbackWrapper*>(
+      destructive_dispatcher_.get())->wrapped();
+}
+
 void FileSystemOperation::DidEnsureFileExistsExclusive(
     base::PlatformFileError rv, bool created) {
-  if (rv == base::PLATFORM_FILE_OK && !created)
-    dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_EXISTS);
-  else
+  if (rv == base::PLATFORM_FILE_OK && !created) {
+    destructive_dispatcher()->DidFail(base::PLATFORM_FILE_ERROR_EXISTS);
+  } else
     DidFinishFileOperation(rv);
 }
 
@@ -237,14 +305,15 @@ void FileSystemOperation::DidFinishFileOperation(
 #ifndef NDEBUG
     DCHECK(kOperationTruncate == pending_operation_);
 #endif
-    FileSystemOperation *cancel_op = cancel_operation_;
-    // This call deletes us, so we have to extract cancel_op first.
-    dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
-    cancel_op->dispatcher_->DidSucceed();
+
+    // Call non-destructive dispatcher's method not to delete this instance yet.
+    non_destructive_dispatcher()->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
+    cancel_operation_->destructive_dispatcher()->DidSucceed();
+    delete this;
   } else if (rv == base::PLATFORM_FILE_OK) {
-    dispatcher_->DidSucceed();
+    destructive_dispatcher()->DidSucceed();
   } else {
-    dispatcher_->DidFail(rv);
+    destructive_dispatcher()->DidFail(rv);
   }
 }
 
@@ -253,11 +322,11 @@ void FileSystemOperation::DidDirectoryExists(
     const base::PlatformFileInfo& file_info) {
   if (rv == base::PLATFORM_FILE_OK) {
     if (file_info.is_directory)
-      dispatcher_->DidSucceed();
+      destructive_dispatcher()->DidSucceed();
     else
-      dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_FAILED);
+      destructive_dispatcher()->DidFail(base::PLATFORM_FILE_ERROR_FAILED);
   } else {
-    dispatcher_->DidFail(rv);
+    destructive_dispatcher()->DidFail(rv);
   }
 }
 
@@ -266,11 +335,11 @@ void FileSystemOperation::DidFileExists(
     const base::PlatformFileInfo& file_info) {
   if (rv == base::PLATFORM_FILE_OK) {
     if (file_info.is_directory)
-      dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_FAILED);
+      destructive_dispatcher()->DidFail(base::PLATFORM_FILE_ERROR_FAILED);
     else
-      dispatcher_->DidSucceed();
+      destructive_dispatcher()->DidSucceed();
   } else {
-    dispatcher_->DidFail(rv);
+    destructive_dispatcher()->DidFail(rv);
   }
 }
 
@@ -278,18 +347,18 @@ void FileSystemOperation::DidGetMetadata(
     base::PlatformFileError rv,
     const base::PlatformFileInfo& file_info) {
   if (rv == base::PLATFORM_FILE_OK)
-    dispatcher_->DidReadMetadata(file_info);
+    destructive_dispatcher()->DidReadMetadata(file_info);
   else
-    dispatcher_->DidFail(rv);
+    destructive_dispatcher()->DidFail(rv);
 }
 
 void FileSystemOperation::DidReadDirectory(
     base::PlatformFileError rv,
     const std::vector<base::FileUtilProxy::Entry>& entries) {
   if (rv == base::PLATFORM_FILE_OK)
-    dispatcher_->DidReadDirectory(entries, false /* has_more */);
+    destructive_dispatcher()->DidReadDirectory(entries, false /* has_more */);
   else
-    dispatcher_->DidFail(rv);
+    destructive_dispatcher()->DidFail(rv);
 }
 
 void FileSystemOperation::DidWrite(
@@ -297,16 +366,16 @@ void FileSystemOperation::DidWrite(
     int64 bytes,
     bool complete) {
   if (rv == base::PLATFORM_FILE_OK)
-    dispatcher_->DidWrite(bytes, complete);
+    destructive_dispatcher()->DidWrite(bytes, complete);

[ericu, 2010/11/16 19:26:54]

Given that we explicitly don't want to delete the operation unless complete, it
seems really unintuitive to ask for the destructive_dispatcher, and then not
have it destroy anything.  Why not just use the nondistructive dispatcher when
you don't want to destroy the FSO, and make the destructive dispatcher always
destroy?

   else
-    dispatcher_->DidFail(rv);
+    destructive_dispatcher()->DidFail(rv);
 }
 
 void FileSystemOperation::DidTouchFile(base::PlatformFileError rv) {
   if (rv == base::PLATFORM_FILE_OK)
-    dispatcher_->DidSucceed();
+    destructive_dispatcher()->DidSucceed();
   else
-    dispatcher_->DidFail(rv);
+    destructive_dispatcher()->DidFail(rv);
 }
 
 }  // namespace fileapi