Make FileSystemOperation's lifetime more explicit.

webkit/fileapi/file_system_operation.cc

 // Copyright (c) 2010 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 "webkit/fileapi/file_system_operation.h"
 
 #include "base/time.h"
 #include "net/url_request/url_request_context.h"
 #include "webkit/fileapi/file_system_callback_dispatcher.h"
 #include "webkit/fileapi/file_writer_delegate.h"
 
 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);
 #ifndef NDEBUG
   pending_operation_ = kOperationNone;
 #endif
 }
 
 FileSystemOperation::~FileSystemOperation() {
   if (file_writer_delegate_.get())
@@ skipping 122 matching lines @@
           base::PLATFORM_FILE_ASYNC,
       callback_factory_.NewCallback(
           &FileSystemOperation::OnFileOpenedForWrite));
 }
 
 void FileSystemOperation::OnFileOpenedForWrite(
     base::PlatformFileError rv,
     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());
 }
 
 void FileSystemOperation::Truncate(const FilePath& path, int64 length) {
 #ifndef NDEBUG
   DCHECK(kOperationNone == pending_operation_);
   pending_operation_ = kOperationTruncate;
 #endif
@@ skipping 25 matching lines @@
     // Writes are done without proxying through FileUtilProxy after the initial
     // opening of the PlatformFile.  All state changes are done on this thread,
     // so we're guaranteed to be able to shut down atomically.  We do need to
     // check that the file has been opened [which means the blob_request_ has
     // been created], so we know how much we need to do.
     if (blob_request_.get())
       // This halts any calls to file_writer_delegate_ from blob_request_.
       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_);
 #endif
     // We're cancelling a truncate operation, but we can't actually stop it
     // 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);
 }
 
 void FileSystemOperation::DidEnsureFileExistsNonExclusive(
     base::PlatformFileError rv, bool /* created */) {
   DidFinishFileOperation(rv);
 }
 
 void FileSystemOperation::DidFinishFileOperation(
     base::PlatformFileError rv) {
   if (cancel_operation_) {
 #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);
   }
 }
 
 void FileSystemOperation::DidDirectoryExists(
     base::PlatformFileError rv,
     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);
   }
 }
 
 void FileSystemOperation::DidFileExists(
     base::PlatformFileError rv,
     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);
   }
 }
 
 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(
     base::PlatformFileError rv,
     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