From 103c8d4b8d27edf535d8a3e7836fd1bffba28bfc Mon Sep 17 00:00:00 2001
From: "kinuko@chromium.org"
 <kinuko@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Tue, 30 Nov 2010 18:58:13 +0000
Subject: [PATCH] Make FileSystemOperation's lifetime more explicit.

In the current code calling dispatcher->DidXxx in an operation's DidXxx method MAY indirectly delete the operation itself depending on the dispatcher's implementation.  I was confused by this several times and I want to make this flow more explicit.

This patch lets FileSystemOperation control its lifetime by itself so that each callback dispatcher implementation does not need to take care of it.

Also moved BrowserFileSystemCallbackDispatcher into file_system_dispatcher_host.cc as it's only used in it and its implementation is tightly coupled with the DispatcherHost.

BUG=60243
TEST=none

Review URL: http://codereview.chromium.org/4821005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@67732 0039d316-1c4b-4281-b951-d872f2087c98
---
 ...browser_file_system_callback_dispatcher.cc |  60 ----
 .../browser_file_system_callback_dispatcher.h |  35 ---
 .../file_system_dispatcher_host.cc            |  56 +++-
 .../file_system/file_system_dispatcher_host.h |   5 +-
 chrome/chrome_browser.gypi                    |   2 -
 webkit/fileapi/file_system_operation.cc       |  34 ++-
 webkit/fileapi/file_system_operation.h        |  16 +-
 .../fileapi/file_system_operation_unittest.cc | 274 +++++++-----------
 .../sandboxed_file_system_operation.cc        |  49 +++-
 .../fileapi/sandboxed_file_system_operation.h |   4 +
 webkit/tools/test_shell/simple_file_system.cc |  24 +-
 webkit/tools/test_shell/simple_file_writer.cc |  32 +-
 12 files changed, 256 insertions(+), 335 deletions(-)
 delete mode 100644 chrome/browser/file_system/browser_file_system_callback_dispatcher.cc
 delete mode 100644 chrome/browser/file_system/browser_file_system_callback_dispatcher.h

diff --git a/chrome/browser/file_system/browser_file_system_callback_dispatcher.cc b/chrome/browser/file_system/browser_file_system_callback_dispatcher.cc
deleted file mode 100644
index 810bf3f8bafe2..0000000000000
--- a/chrome/browser/file_system/browser_file_system_callback_dispatcher.cc
+++ /dev/null
@@ -1,60 +0,0 @@
-// 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 "chrome/browser/file_system/browser_file_system_callback_dispatcher.h"
-
-#include "chrome/browser/file_system/file_system_dispatcher_host.h"
-#include "chrome/common/render_messages.h"
-
-BrowserFileSystemCallbackDispatcher::BrowserFileSystemCallbackDispatcher(
-    FileSystemDispatcherHost* dispatcher_host, int request_id)
-    : dispatcher_host_(dispatcher_host),
-      request_id_(request_id) {
-  DCHECK(dispatcher_host_);
-}
-
-BrowserFileSystemCallbackDispatcher::~BrowserFileSystemCallbackDispatcher() {}
-
-void BrowserFileSystemCallbackDispatcher::DidSucceed() {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidSucceed(request_id_));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidReadMetadata(
-    const base::PlatformFileInfo& info) {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidReadMetadata(
-      request_id_, info));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidReadDirectory(
-    const std::vector<base::FileUtilProxy::Entry>& entries, bool has_more) {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidReadDirectory(
-      request_id_, entries, has_more));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidOpenFileSystem(
-    const std::string& name, const FilePath& path) {
-  dispatcher_host_->Send(
-      new ViewMsg_OpenFileSystemRequest_Complete(
-          request_id_, !path.empty(), name, path));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidFail(
-    base::PlatformFileError error_code) {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidFail(
-      request_id_, error_code));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidWrite(
-    int64 bytes,
-    bool complete) {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidWrite(
-      request_id_, bytes, complete));
-  if (complete)
-    dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
diff --git a/chrome/browser/file_system/browser_file_system_callback_dispatcher.h b/chrome/browser/file_system/browser_file_system_callback_dispatcher.h
deleted file mode 100644
index 7e85b41d73ce5..0000000000000
--- a/chrome/browser/file_system/browser_file_system_callback_dispatcher.h
+++ /dev/null
@@ -1,35 +0,0 @@
-// 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.
-
-#ifndef CHROME_BROWSER_FILE_SYSTEM_BROWSER_FILE_SYSTEM_CALLBACK_DISPATCHER_H_
-#define CHROME_BROWSER_FILE_SYSTEM_BROWSER_FILE_SYSTEM_CALLBACK_DISPATCHER_H_
-
-#include "webkit/fileapi/file_system_callback_dispatcher.h"
-
-class FileSystemDispatcherHost;
-
-class BrowserFileSystemCallbackDispatcher
-    : public fileapi::FileSystemCallbackDispatcher {
- public:
-  BrowserFileSystemCallbackDispatcher(FileSystemDispatcherHost* dispatcher_host,
-                                      int request_id);
-  virtual ~BrowserFileSystemCallbackDispatcher();
-
-  // FileSystemCallbackDispatcher implementation.
-  virtual void DidSucceed();
-  virtual void DidReadMetadata(const base::PlatformFileInfo& file_info);
-  virtual void DidReadDirectory(
-      const std::vector<base::FileUtilProxy::Entry>& entries,
-      bool has_more);
-  virtual void DidOpenFileSystem(const std::string& name,
-                                 const FilePath& root_path);
-  virtual void DidFail(base::PlatformFileError error_code);
-  virtual void DidWrite(int64 bytes, bool complete);
-
- private:
-  scoped_refptr<FileSystemDispatcherHost> dispatcher_host_;
-  int request_id_;
-};
-
-#endif  // CHROME_BROWSER_FILE_SYSTEM_BROWSER_FILE_SYSTEM_CALLBACK_DISPATCHER_H_
diff --git a/chrome/browser/file_system/file_system_dispatcher_host.cc b/chrome/browser/file_system/file_system_dispatcher_host.cc
index 2668a78b6e050..6807f285db0e2 100644
--- a/chrome/browser/file_system/file_system_dispatcher_host.cc
+++ b/chrome/browser/file_system/file_system_dispatcher_host.cc
@@ -10,7 +10,6 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
-#include "chrome/browser/file_system/browser_file_system_callback_dispatcher.h"
 #include "chrome/browser/file_system/browser_file_system_context.h"
 #include "chrome/browser/net/chrome_url_request_context.h"
 #include "chrome/browser/profile.h"
@@ -20,14 +19,67 @@
 #include "chrome/common/render_messages_params.h"
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_request_context.h"
+#include "webkit/fileapi/file_system_callback_dispatcher.h"
 #include "webkit/fileapi/file_system_operation.h"
 #include "webkit/fileapi/file_system_path_manager.h"
 #include "webkit/fileapi/file_system_quota_manager.h"
 #include "webkit/fileapi/sandboxed_file_system_operation.h"
 
+using fileapi::FileSystemCallbackDispatcher;
 using fileapi::FileSystemQuotaManager;
 using fileapi::SandboxedFileSystemOperation;
 
+class BrowserFileSystemCallbackDispatcher
+    : public FileSystemCallbackDispatcher {
+ public:
+  BrowserFileSystemCallbackDispatcher(
+      FileSystemDispatcherHost* dispatcher_host, int request_id)
+      : dispatcher_host_(dispatcher_host),
+        request_id_(request_id) {
+    DCHECK(dispatcher_host_);
+  }
+
+  virtual ~BrowserFileSystemCallbackDispatcher() {
+    dispatcher_host_->UnregisterOperation(request_id_);
+  }
+
+  virtual void DidSucceed() {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidSucceed(request_id_));
+  }
+
+  virtual void DidReadMetadata(const base::PlatformFileInfo& info) {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidReadMetadata(
+        request_id_, info));
+  }
+
+  virtual void DidReadDirectory(
+      const std::vector<base::FileUtilProxy::Entry>& entries, bool has_more) {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidReadDirectory(
+        request_id_, entries, has_more));
+  }
+
+  virtual void DidOpenFileSystem(const std::string& name,
+                                 const FilePath& path) {
+    dispatcher_host_->Send(
+        new ViewMsg_OpenFileSystemRequest_Complete(
+            request_id_, !path.empty(), name, path));
+  }
+
+  virtual void DidFail(base::PlatformFileError error_code) {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidFail(
+        request_id_, error_code));
+  }
+
+  virtual void DidWrite(int64 bytes, bool complete) {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidWrite(
+        request_id_, bytes, complete));
+  }
+
+ private:
+  scoped_refptr<FileSystemDispatcherHost> dispatcher_host_;
+  int request_id_;
+};
+
 FileSystemDispatcherHost::FileSystemDispatcherHost(
     IPC::Message::Sender* sender, Profile* profile)
     : message_sender_(sender),
@@ -217,7 +269,7 @@ SandboxedFileSystemOperation* FileSystemDispatcherHost::GetNewOperation(
   return operation;
 }
 
-void FileSystemDispatcherHost::RemoveCompletedOperation(int request_id) {
+void FileSystemDispatcherHost::UnregisterOperation(int request_id) {
   DCHECK(operations_.Lookup(request_id));
   operations_.Remove(request_id);
 }
diff --git a/chrome/browser/file_system/file_system_dispatcher_host.h b/chrome/browser/file_system/file_system_dispatcher_host.h
index 2cfe4f634d85b..fdbe8f6ec934e 100644
--- a/chrome/browser/file_system/file_system_dispatcher_host.h
+++ b/chrome/browser/file_system/file_system_dispatcher_host.h
@@ -79,7 +79,7 @@ class FileSystemDispatcherHost
                    const base::Time& last_modified_time);
   void OnCancel(int request_id, int request_to_cancel);
   void Send(IPC::Message* message);
-  void RemoveCompletedOperation(int request_id);
+  void UnregisterOperation(int request_id);
 
  private:
   // Creates a new SandboxedFileSystemOperation.
@@ -99,8 +99,7 @@ class FileSystemDispatcherHost
   scoped_refptr<HostContentSettingsMap> host_content_settings_map_;
 
   // Keeps ongoing file system operations.
-  typedef IDMap<fileapi::SandboxedFileSystemOperation, IDMapOwnPointer>
-      OperationsMap;
+  typedef IDMap<fileapi::SandboxedFileSystemOperation> OperationsMap;
   OperationsMap operations_;
 
   // This holds the URLRequestContextGetter until Init() can be called from the
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index a5f6f165c171c..b3d85770c98e6 100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -1656,8 +1656,6 @@
         'browser/file_path_watcher_win.cc',
         'browser/file_select_helper.cc',
         'browser/file_select_helper.h',
-        'browser/file_system/browser_file_system_callback_dispatcher.cc',
-        'browser/file_system/browser_file_system_callback_dispatcher.h',
         'browser/file_system/browser_file_system_context.cc',
         'browser/file_system/browser_file_system_context.h',
         'browser/file_system/file_system_dispatcher_host.cc',
diff --git a/webkit/fileapi/file_system_operation.cc b/webkit/fileapi/file_system_operation.cc
index fe077581fb7a4..97e2bfe4f61b8 100644
--- a/webkit/fileapi/file_system_operation.cc
+++ b/webkit/fileapi/file_system_operation.cc
@@ -16,8 +16,7 @@ FileSystemOperation::FileSystemOperation(
     scoped_refptr<base::MessageLoopProxy> proxy)
     : proxy_(proxy),
       dispatcher_(dispatcher),
-      callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
-      cancel_operation_(NULL) {
+      callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   DCHECK(dispatcher);
 #ifndef NDEBUG
   pending_operation_ = kOperationNone;
@@ -159,6 +158,7 @@ void FileSystemOperation::OnFileOpenedForWrite(
     bool created) {
   if (base::PLATFORM_FILE_OK != rv) {
     dispatcher_->DidFail(rv);
+    delete this;
     return;
   }
   file_writer_delegate_->Start(file.ReleaseValue(), blob_request_.get());
@@ -189,7 +189,8 @@ void FileSystemOperation::TouchFile(const FilePath& path,
 
 // We can only get here on a write or truncate that's not yet completed.
 // We don't support cancelling any other operation at this time.
-void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation) {
+void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation_ptr) {
+  scoped_ptr<FileSystemOperation> cancel_operation(cancel_operation_ptr);
   if (file_writer_delegate_.get()) {
 #ifndef NDEBUG
     DCHECK(kOperationWrite == pending_operation_);
@@ -203,9 +204,9 @@ void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation) {
       // 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();
+    cancel_operation->dispatcher()->DidSucceed();
+    delete this;
   } else {
 #ifndef NDEBUG
     DCHECK(kOperationTruncate == pending_operation_);
@@ -214,15 +215,17 @@ 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.
-    cancel_operation_ = cancel_operation;
+    DCHECK(!cancel_operation_.get());
+    cancel_operation_.swap(cancel_operation);
   }
 }
 
 void FileSystemOperation::DidEnsureFileExistsExclusive(
     base::PlatformFileError rv, bool created) {
-  if (rv == base::PLATFORM_FILE_OK && !created)
+  if (rv == base::PLATFORM_FILE_OK && !created) {
     dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_EXISTS);
-  else
+    delete this;
+  } else
     DidFinishFileOperation(rv);
 }
 
@@ -233,19 +236,19 @@ void FileSystemOperation::DidEnsureFileExistsNonExclusive(
 
 void FileSystemOperation::DidFinishFileOperation(
     base::PlatformFileError rv) {
-  if (cancel_operation_) {
+  if (cancel_operation_.get()) {
 #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();
+    cancel_operation_->dispatcher()->DidSucceed();
   } else if (rv == base::PLATFORM_FILE_OK) {
     dispatcher_->DidSucceed();
   } else {
     dispatcher_->DidFail(rv);
   }
+  delete this;
 }
 
 void FileSystemOperation::DidDirectoryExists(
@@ -259,6 +262,7 @@ void FileSystemOperation::DidDirectoryExists(
   } else {
     dispatcher_->DidFail(rv);
   }
+  delete this;
 }
 
 void FileSystemOperation::DidFileExists(
@@ -272,6 +276,7 @@ void FileSystemOperation::DidFileExists(
   } else {
     dispatcher_->DidFail(rv);
   }
+  delete this;
 }
 
 void FileSystemOperation::DidGetMetadata(
@@ -281,6 +286,7 @@ void FileSystemOperation::DidGetMetadata(
     dispatcher_->DidReadMetadata(file_info);
   else
     dispatcher_->DidFail(rv);
+  delete this;
 }
 
 void FileSystemOperation::DidReadDirectory(
@@ -290,6 +296,7 @@ void FileSystemOperation::DidReadDirectory(
     dispatcher_->DidReadDirectory(entries, false /* has_more */);
   else
     dispatcher_->DidFail(rv);
+  delete this;
 }
 
 void FileSystemOperation::DidWrite(
@@ -300,6 +307,8 @@ void FileSystemOperation::DidWrite(
     dispatcher_->DidWrite(bytes, complete);
   else
     dispatcher_->DidFail(rv);
+  if (complete || rv != base::PLATFORM_FILE_OK)
+    delete this;
 }
 
 void FileSystemOperation::DidTouchFile(base::PlatformFileError rv) {
@@ -307,6 +316,7 @@ void FileSystemOperation::DidTouchFile(base::PlatformFileError rv) {
     dispatcher_->DidSucceed();
   else
     dispatcher_->DidFail(rv);
+  delete this;
 }
 
 }  // namespace fileapi
diff --git a/webkit/fileapi/file_system_operation.h b/webkit/fileapi/file_system_operation.h
index 6614b5e9cec03..e260e24806767 100644
--- a/webkit/fileapi/file_system_operation.h
+++ b/webkit/fileapi/file_system_operation.h
@@ -36,8 +36,11 @@ class FileWriterDelegate;
 // Only one method(CreateFile, CreateDirectory, Copy, Move, DirectoryExists,
 // GetMetadata, ReadDirectory and Remove) may be called during the lifetime of
 // this object and it should be called no more than once.
+// This class is self-destructed and an instance automatically gets deleted
+// when its operation is finished.
 class FileSystemOperation {
  public:
+  // |dispatcher| will be owned by this class.
   FileSystemOperation(FileSystemCallbackDispatcher* dispatcher,
                       scoped_refptr<base::MessageLoopProxy> proxy);
   virtual ~FileSystemOperation();
@@ -65,15 +68,16 @@ class FileSystemOperation {
 
   virtual void Remove(const FilePath& path, bool recursive);
 
-  virtual void Write(
-      scoped_refptr<URLRequestContext> url_request_context,
-      const FilePath& path, const GURL& blob_url, int64 offset);
+  virtual void Write(scoped_refptr<URLRequestContext> url_request_context,
+                     const FilePath& path,
+                     const GURL& blob_url,
+                     int64 offset);
 
   virtual void Truncate(const FilePath& path, int64 length);
 
   virtual void TouchFile(const FilePath& path,
-                 const base::Time& last_access_time,
-                 const base::Time& last_modified_time);
+                         const base::Time& last_access_time,
+                         const base::Time& last_modified_time);
 
   // Try to cancel the current operation [we support cancelling write or
   // truncate only].  Report failure for the current operation, then tell the
@@ -154,7 +158,7 @@ class FileSystemOperation {
   friend class FileWriterDelegate;
   scoped_ptr<FileWriterDelegate> file_writer_delegate_;
   scoped_ptr<net::URLRequest> blob_request_;
-  FileSystemOperation* cancel_operation_;
+  scoped_ptr<FileSystemOperation> cancel_operation_;
 
   DISALLOW_COPY_AND_ASSIGN(FileSystemOperation);
 };
diff --git a/webkit/fileapi/file_system_operation_unittest.cc b/webkit/fileapi/file_system_operation_unittest.cc
index 370d95c119884..dffcc33e57667 100644
--- a/webkit/fileapi/file_system_operation_unittest.cc
+++ b/webkit/fileapi/file_system_operation_unittest.cc
@@ -14,40 +14,69 @@
 
 namespace fileapi {
 
-const int kInvalidRequestId = -1;
-const int kFileOperationStatusNotSet = 0;
-const int kFileOperationSucceeded = 1;
-
-static int last_request_id = -1;
+const int kFileOperationStatusNotSet = 1;
+const int kFileOperationSucceeded = 0;
 
 static bool FileExists(FilePath path) {
   return file_util::PathExists(path) && !file_util::DirectoryExists(path);
 }
 
-class MockDispatcher : public FileSystemCallbackDispatcher {
+class MockDispatcher;
+
+class FileSystemOperationTest : public testing::Test {
  public:
-  MockDispatcher(int request_id)
-      : status_(kFileOperationStatusNotSet),
-        request_id_(request_id) {
+  FileSystemOperationTest()
+      : status_(kFileOperationStatusNotSet) {
+    base_.CreateUniqueTempDir();
+    EXPECT_TRUE(base_.IsValid());
   }
 
+  FileSystemOperation* operation();
+
+  void set_status(int status) { status_ = status; }
+  int status() const { return status_; }
+  void set_info(const base::PlatformFileInfo& info) { info_ = info; }
+  const base::PlatformFileInfo& info() const { return info_; }
+  void set_entries(const std::vector<base::FileUtilProxy::Entry>& entries) {
+    entries_ = entries;
+  }
+  const std::vector<base::FileUtilProxy::Entry>& entries() const {
+    return entries_;
+  }
+
+ protected:
+  // Common temp base for nondestructive uses.
+  ScopedTempDir base_;
+
+  // For post-operation status.
+  int status_;
+  base::PlatformFileInfo info_;
+  std::vector<base::FileUtilProxy::Entry> entries_;
+
+  DISALLOW_COPY_AND_ASSIGN(FileSystemOperationTest);
+};
+
+class MockDispatcher : public FileSystemCallbackDispatcher {
+ public:
+  MockDispatcher(FileSystemOperationTest* test) : test_(test) { }
+
   virtual void DidFail(base::PlatformFileError status) {
-    status_ = status;
+    test_->set_status(status);
   }
 
   virtual void DidSucceed() {
-    status_ = kFileOperationSucceeded;
+    test_->set_status(kFileOperationSucceeded);
   }
 
   virtual void DidReadMetadata(const base::PlatformFileInfo& info) {
-    info_ = info;
-    status_ = kFileOperationSucceeded;
+    test_->set_info(info);
+    test_->set_status(kFileOperationSucceeded);
   }
 
   virtual void DidReadDirectory(
       const std::vector<base::FileUtilProxy::Entry>& entries,
       bool /* has_more */) {
-    entries_ = entries;
+    test_->set_entries(entries);
   }
 
   virtual void DidOpenFileSystem(const std::string&, const FilePath&) {
@@ -58,58 +87,22 @@ class MockDispatcher : public FileSystemCallbackDispatcher {
     NOTREACHED();
   }
 
-  // Helpers for testing.
-  int status() const { return status_; }
-  int request_id() const { return request_id_; }
-  const base::PlatformFileInfo& info() const { return info_; }
-  const std::vector<base::FileUtilProxy::Entry>& entries() const {
-    return entries_;
-  }
-
  private:
-  int status_;
-  int request_id_;
-  base::PlatformFileInfo info_;
-  std::vector<base::FileUtilProxy::Entry> entries_;
+  FileSystemOperationTest* test_;
 };
 
-class FileSystemOperationTest : public testing::Test {
- public:
-  FileSystemOperationTest()
-      : request_id_(kInvalidRequestId),
-        operation_(NULL) {
-    base_.CreateUniqueTempDir();
-    EXPECT_TRUE(base_.IsValid());
-  }
-
-  FileSystemOperation* operation() {
-    request_id_ = ++last_request_id;
-    mock_dispatcher_ = new MockDispatcher(request_id_);
-    operation_.reset(new FileSystemOperation(
-        mock_dispatcher_, base::MessageLoopProxy::CreateForCurrentThread()));
-    return operation_.get();
-  }
-
- protected:
-  // Common temp base for nondestructive uses.
-  ScopedTempDir base_;
-
-  int request_id_;
-  scoped_ptr<FileSystemOperation> operation_;
-
-  // Owned by |operation_|.
-  MockDispatcher* mock_dispatcher_;
-
-  DISALLOW_COPY_AND_ASSIGN(FileSystemOperationTest);
-};
+FileSystemOperation* FileSystemOperationTest::operation() {
+  return new FileSystemOperation(
+      new MockDispatcher(this),
+      base::MessageLoopProxy::CreateForCurrentThread());
+}
 
 TEST_F(FileSystemOperationTest, TestMoveFailureSrcDoesntExist) {
   FilePath src(base_.path().Append(FILE_PATH_LITERAL("a")));
   FilePath dest(base_.path().Append(FILE_PATH_LITERAL("b")));
   operation()->Move(src, dest);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 }
 
 TEST_F(FileSystemOperationTest, TestMoveFailureContainsPath) {
@@ -121,9 +114,7 @@ TEST_F(FileSystemOperationTest, TestMoveFailureContainsPath) {
                                                  &dest_dir_path));
   operation()->Move(src_dir.path(), dest_dir_path);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_INVALID_OPERATION,
-            mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_INVALID_OPERATION, status());
 }
 
 TEST_F(FileSystemOperationTest, TestMoveFailureSrcDirExistsDestFile) {
@@ -138,9 +129,7 @@ TEST_F(FileSystemOperationTest, TestMoveFailureSrcDirExistsDestFile) {
 
   operation()->Move(src_dir.path(), dest_file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY,
-            mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY, status());
 }
 
 TEST_F(FileSystemOperationTest, TestMoveFailureSrcFileExistsDestNonEmptyDir) {
@@ -155,8 +144,7 @@ TEST_F(FileSystemOperationTest, TestMoveFailureSrcFileExistsDestNonEmptyDir) {
 
   operation()->Move(src_dir.path(), dest_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_EMPTY, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_EMPTY, status());
 }
 
 TEST_F(FileSystemOperationTest, TestMoveFailureSrcFileExistsDestDir) {
@@ -171,9 +159,7 @@ TEST_F(FileSystemOperationTest, TestMoveFailureSrcFileExistsDestDir) {
 
   operation()->Move(src_file, dest_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_FILE,
-            mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_FILE, status());
 }
 
 TEST_F(FileSystemOperationTest, TestMoveFailureDestParentDoesntExist) {
@@ -186,8 +172,7 @@ TEST_F(FileSystemOperationTest, TestMoveFailureDestParentDoesntExist) {
 
   operation()->Move(src_dir.path(), nonexisting_file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 }
 
 TEST_F(FileSystemOperationTest, TestMoveSuccessSrcFileAndOverwrite) {
@@ -203,9 +188,8 @@ TEST_F(FileSystemOperationTest, TestMoveSuccessSrcFileAndOverwrite) {
 
   operation()->Move(src_file, dest_file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(FileExists(dest_file));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 TEST_F(FileSystemOperationTest, TestMoveSuccessSrcFileAndNew) {
@@ -220,9 +204,8 @@ TEST_F(FileSystemOperationTest, TestMoveSuccessSrcFileAndNew) {
 
   operation()->Move(src_file, dest_file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(FileExists(dest_file));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 TEST_F(FileSystemOperationTest, TestMoveSuccessSrcDirAndOverwrite) {
@@ -234,8 +217,7 @@ TEST_F(FileSystemOperationTest, TestMoveSuccessSrcDirAndOverwrite) {
 
   operation()->Move(src_dir.path(), dest_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_FALSE(file_util::DirectoryExists(src_dir.path()));
 
   // Make sure we've overwritten but not moved the source under the |dest_dir|.
@@ -254,8 +236,7 @@ TEST_F(FileSystemOperationTest, TestMoveSuccessSrcDirAndNew) {
 
   operation()->Move(src_dir.path(), dest_dir_path);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_FALSE(file_util::DirectoryExists(src_dir.path()));
   EXPECT_TRUE(file_util::DirectoryExists(dest_dir_path));
 }
@@ -271,8 +252,7 @@ TEST_F(FileSystemOperationTest, TestMoveSuccessSrcDirRecursive) {
 
   operation()->Move(src_dir.path(), dest_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(FileExists(dest_dir.path().Append(child_file.BaseName())));
 }
 
@@ -281,8 +261,7 @@ TEST_F(FileSystemOperationTest, TestCopyFailureSrcDoesntExist) {
   FilePath dest(base_.path().Append(FILE_PATH_LITERAL("b")));
   operation()->Copy(src, dest);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCopyFailureContainsPath) {
@@ -294,9 +273,7 @@ TEST_F(FileSystemOperationTest, TestCopyFailureContainsPath) {
                                                  &dest_dir_path));
   operation()->Copy(src_dir.path(), dest_dir_path);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_INVALID_OPERATION,
-            mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_INVALID_OPERATION, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCopyFailureSrcDirExistsDestFile) {
@@ -311,9 +288,7 @@ TEST_F(FileSystemOperationTest, TestCopyFailureSrcDirExistsDestFile) {
 
   operation()->Copy(src_dir.path(), dest_file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY,
-            mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCopyFailureSrcFileExistsDestNonEmptyDir) {
@@ -328,8 +303,7 @@ TEST_F(FileSystemOperationTest, TestCopyFailureSrcFileExistsDestNonEmptyDir) {
 
   operation()->Copy(src_dir.path(), dest_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_EMPTY, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_EMPTY, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCopyFailureSrcFileExistsDestDir) {
@@ -344,9 +318,7 @@ TEST_F(FileSystemOperationTest, TestCopyFailureSrcFileExistsDestDir) {
 
   operation()->Copy(src_file, dest_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_FILE,
-            mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_FILE, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCopyFailureDestParentDoesntExist) {
@@ -362,8 +334,7 @@ TEST_F(FileSystemOperationTest, TestCopyFailureDestParentDoesntExist) {
 
   operation()->Copy(src_dir, nonexisting_file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCopySuccessSrcFileAndOverwrite) {
@@ -379,9 +350,8 @@ TEST_F(FileSystemOperationTest, TestCopySuccessSrcFileAndOverwrite) {
 
   operation()->Copy(src_file, dest_file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(FileExists(dest_file));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 TEST_F(FileSystemOperationTest, TestCopySuccessSrcFileAndNew) {
@@ -396,9 +366,8 @@ TEST_F(FileSystemOperationTest, TestCopySuccessSrcFileAndNew) {
 
   operation()->Copy(src_file, dest_file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(FileExists(dest_file));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 TEST_F(FileSystemOperationTest, TestCopySuccessSrcDirAndOverwrite) {
@@ -410,8 +379,7 @@ TEST_F(FileSystemOperationTest, TestCopySuccessSrcDirAndOverwrite) {
 
   operation()->Copy(src_dir.path(), dest_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
 
   // Make sure we've overwritten but not copied the source under the |dest_dir|.
   EXPECT_TRUE(file_util::DirectoryExists(dest_dir.path()));
@@ -429,8 +397,7 @@ TEST_F(FileSystemOperationTest, TestCopySuccessSrcDirAndNew) {
 
   operation()->Copy(src_dir.path(), dest_dir);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(file_util::DirectoryExists(dest_dir));
 }
 
@@ -445,8 +412,7 @@ TEST_F(FileSystemOperationTest, TestCopySuccessSrcDirRecursive) {
 
   operation()->Copy(src_dir.path(), dest_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(FileExists(dest_dir.path().Append(child_file.BaseName())));
 }
 
@@ -459,8 +425,7 @@ TEST_F(FileSystemOperationTest, TestCreateFileFailure) {
   file_util::CreateTemporaryFileInDir(dir.path(), &file);
   operation()->CreateFile(file, true);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_EXISTS, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_EXISTS, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCreateFileSuccessFileExists) {
@@ -472,9 +437,8 @@ TEST_F(FileSystemOperationTest, TestCreateFileSuccessFileExists) {
 
   operation()->CreateFile(file, false);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(FileExists(file));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 TEST_F(FileSystemOperationTest, TestCreateFileSuccessExclusive) {
@@ -484,9 +448,8 @@ TEST_F(FileSystemOperationTest, TestCreateFileSuccessExclusive) {
   FilePath file = dir.path().Append(FILE_PATH_LITERAL("FileDoesntExist"));
   operation()->CreateFile(file, true);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(FileExists(file));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 TEST_F(FileSystemOperationTest, TestCreateFileSuccessFileDoesntExist) {
@@ -496,8 +459,7 @@ TEST_F(FileSystemOperationTest, TestCreateFileSuccessFileDoesntExist) {
   FilePath file = dir.path().Append(FILE_PATH_LITERAL("FileDoesntExist"));
   operation()->CreateFile(file, false);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
 }
 
 TEST_F(FileSystemOperationTest,
@@ -509,8 +471,7 @@ TEST_F(FileSystemOperationTest,
       FILE_PATH_LITERAL("FileDoesntExist"));
   operation()->CreateDirectory(nonexisting_file, false, false);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCreateDirFailureDirExists) {
@@ -519,8 +480,7 @@ TEST_F(FileSystemOperationTest, TestCreateDirFailureDirExists) {
   ASSERT_TRUE(src_dir.CreateUniqueTempDir());
   operation()->CreateDirectory(src_dir.path(), true, false);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_EXISTS, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_EXISTS, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCreateDirFailureFileExists) {
@@ -531,8 +491,7 @@ TEST_F(FileSystemOperationTest, TestCreateDirFailureFileExists) {
   file_util::CreateTemporaryFileInDir(dir.path(), &file);
   operation()->CreateDirectory(file, true, false);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_EXISTS, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_EXISTS, status());
 }
 
 TEST_F(FileSystemOperationTest, TestCreateDirSuccess) {
@@ -541,17 +500,15 @@ TEST_F(FileSystemOperationTest, TestCreateDirSuccess) {
   ASSERT_TRUE(dir.CreateUniqueTempDir());
   operation()->CreateDirectory(dir.path(), false, false);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
 
   // Dir doesn't exist.
   FilePath nonexisting_dir_path(base_.path().Append(
       FILE_PATH_LITERAL("nonexistingdir")));
   operation()->CreateDirectory(nonexisting_dir_path, false, false);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(file_util::DirectoryExists(nonexisting_dir_path));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 TEST_F(FileSystemOperationTest, TestCreateDirSuccessExclusive) {
@@ -561,9 +518,8 @@ TEST_F(FileSystemOperationTest, TestCreateDirSuccessExclusive) {
 
   operation()->CreateDirectory(nonexisting_dir_path, true, false);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_TRUE(file_util::DirectoryExists(nonexisting_dir_path));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 TEST_F(FileSystemOperationTest, TestExistsAndMetadataFailure) {
@@ -571,18 +527,16 @@ TEST_F(FileSystemOperationTest, TestExistsAndMetadataFailure) {
       FILE_PATH_LITERAL("nonexistingdir")));
   operation()->GetMetadata(nonexisting_dir_path);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 
   operation()->FileExists(nonexisting_dir_path);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 
   file_util::EnsureEndsWithSeparator(&nonexisting_dir_path);
   operation()->DirectoryExists(nonexisting_dir_path);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 }
 
 TEST_F(FileSystemOperationTest, TestExistsAndMetadataSuccess) {
@@ -591,27 +545,23 @@ TEST_F(FileSystemOperationTest, TestExistsAndMetadataSuccess) {
 
   operation()->DirectoryExists(dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
 
   operation()->GetMetadata(dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_TRUE(mock_dispatcher_->info().is_directory);
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
+  EXPECT_TRUE(info().is_directory);
 
   FilePath file;
   file_util::CreateTemporaryFileInDir(dir.path(), &file);
   operation()->FileExists(file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
 
   operation()->GetMetadata(file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_FALSE(mock_dispatcher_->info().is_directory);
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
+  EXPECT_FALSE(info().is_directory);
 }
 
 TEST_F(FileSystemOperationTest, TestReadDirFailure) {
@@ -621,8 +571,7 @@ TEST_F(FileSystemOperationTest, TestReadDirFailure) {
   file_util::EnsureEndsWithSeparator(&nonexisting_dir_path);
   operation()->ReadDirectory(nonexisting_dir_path);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 
   // File exists.
   ScopedTempDir dir;
@@ -632,8 +581,7 @@ TEST_F(FileSystemOperationTest, TestReadDirFailure) {
   operation()->ReadDirectory(file);
   MessageLoop::current()->RunAllPending();
   // TODO(kkanetkar) crbug.com/54309 to change the error code.
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 }
 
 TEST_F(FileSystemOperationTest, TestReadDirSuccess) {
@@ -651,17 +599,16 @@ TEST_F(FileSystemOperationTest, TestReadDirSuccess) {
 
   operation()->ReadDirectory(parent_dir.path());
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationStatusNotSet, mock_dispatcher_->status());
-  EXPECT_EQ(2u, mock_dispatcher_->entries().size());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationStatusNotSet, status());
+  EXPECT_EQ(2u, entries().size());
 
-  for (size_t i = 0; i < mock_dispatcher_->entries().size(); ++i) {
-    if (mock_dispatcher_->entries()[i].is_directory) {
+  for (size_t i = 0; i < entries().size(); ++i) {
+    if (entries()[i].is_directory) {
       EXPECT_EQ(child_dir.BaseName().value(),
-                mock_dispatcher_->entries()[i].name);
+                entries()[i].name);
     } else {
       EXPECT_EQ(child_file.BaseName().value(),
-                mock_dispatcher_->entries()[i].name);
+                entries()[i].name);
     }
   }
 }
@@ -674,8 +621,7 @@ TEST_F(FileSystemOperationTest, TestRemoveFailure) {
 
   operation()->Remove(nonexisting, false /* recursive */);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, status());
 
   // It's an error to try to remove a non-empty directory if recursive flag
   // is false.
@@ -694,8 +640,7 @@ TEST_F(FileSystemOperationTest, TestRemoveFailure) {
   operation()->Remove(parent_dir.path(), false /* recursive */);
   MessageLoop::current()->RunAllPending();
   EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_EMPTY,
-            mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+            status());
 }
 
 TEST_F(FileSystemOperationTest, TestRemoveSuccess) {
@@ -705,9 +650,8 @@ TEST_F(FileSystemOperationTest, TestRemoveSuccess) {
 
   operation()->Remove(empty_dir.path(), false /* recursive */);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_FALSE(file_util::DirectoryExists(empty_dir.path()));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 
   // Removing a non-empty directory with recursive flag == true should be ok.
   //      parent_dir
@@ -724,9 +668,8 @@ TEST_F(FileSystemOperationTest, TestRemoveSuccess) {
 
   operation()->Remove(parent_dir.path(), true /* recursive */);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
+  EXPECT_EQ(kFileOperationSucceeded, status());
   EXPECT_FALSE(file_util::DirectoryExists(parent_dir.path()));
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
 }
 
 // TODO(ericu): Add tests for Write, Cancel.
@@ -745,17 +688,15 @@ TEST_F(FileSystemOperationTest, TestTruncate) {
   // Check that its length is the size of the data written.
   operation()->GetMetadata(file);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_FALSE(mock_dispatcher_->info().is_directory);
-  EXPECT_EQ(data_size, mock_dispatcher_->info().size);
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
+  EXPECT_FALSE(info().is_directory);
+  EXPECT_EQ(data_size, info().size);
 
   // Extend the file by truncating it.
   int length = 17;
   operation()->Truncate(file, length);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
 
   // Check that its length is now 17 and that it's all zeroes after the test
   // data.
@@ -776,8 +717,7 @@ TEST_F(FileSystemOperationTest, TestTruncate) {
   length = 3;
   operation()->Truncate(file, length);
   MessageLoop::current()->RunAllPending();
-  EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status());
-  EXPECT_EQ(request_id_, mock_dispatcher_->request_id());
+  EXPECT_EQ(kFileOperationSucceeded, status());
 
   // Check that its length is now 3 and that it contains only bits of test data.
   EXPECT_TRUE(file_util::GetFileInfo(file, &info));
diff --git a/webkit/fileapi/sandboxed_file_system_operation.cc b/webkit/fileapi/sandboxed_file_system_operation.cc
index dfc2884f677e1..7f9dd38db0a30 100644
--- a/webkit/fileapi/sandboxed_file_system_operation.cc
+++ b/webkit/fileapi/sandboxed_file_system_operation.cc
@@ -38,15 +38,19 @@ void SandboxedFileSystemOperation::OpenFileSystem(
 
 void SandboxedFileSystemOperation::CreateFile(
     const FilePath& path, bool exclusive) {
-  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::CreateFile(path, exclusive);
 }
 
 void SandboxedFileSystemOperation::CreateDirectory(
     const FilePath& path, bool exclusive, bool recursive) {
-  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::CreateDirectory(path, exclusive, recursive);
 }
 
@@ -54,8 +58,10 @@ void SandboxedFileSystemOperation::Copy(
     const FilePath& src_path, const FilePath& dest_path) {
   if (!VerifyFileSystemPathForRead(src_path) ||
       !VerifyFileSystemPathForWrite(dest_path, true /* create */,
-                                    FileSystemQuotaManager::kUnknownSize))
+                                    FileSystemQuotaManager::kUnknownSize)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Copy(src_path, dest_path);
 }
 
@@ -63,39 +69,51 @@ void SandboxedFileSystemOperation::Move(
     const FilePath& src_path, const FilePath& dest_path) {
   if (!VerifyFileSystemPathForRead(src_path) ||
       !VerifyFileSystemPathForWrite(dest_path, true /* create */,
-                                    FileSystemQuotaManager::kUnknownSize))
+                                    FileSystemQuotaManager::kUnknownSize)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Move(src_path, dest_path);
 }
 
 void SandboxedFileSystemOperation::DirectoryExists(const FilePath& path) {
-  if (!VerifyFileSystemPathForRead(path))
+  if (!VerifyFileSystemPathForRead(path)) {
+    delete this;
     return;
+  }
   FileSystemOperation::DirectoryExists(path);
 }
 
 void SandboxedFileSystemOperation::FileExists(const FilePath& path) {
-  if (!VerifyFileSystemPathForRead(path))
+  if (!VerifyFileSystemPathForRead(path)) {
+    delete this;
     return;
+  }
   FileSystemOperation::FileExists(path);
 }
 
 void SandboxedFileSystemOperation::GetMetadata(const FilePath& path) {
-  if (!VerifyFileSystemPathForRead(path))
+  if (!VerifyFileSystemPathForRead(path)) {
+    delete this;
     return;
+  }
   FileSystemOperation::GetMetadata(path);
 }
 
 void SandboxedFileSystemOperation::ReadDirectory(const FilePath& path) {
-  if (!VerifyFileSystemPathForRead(path))
+  if (!VerifyFileSystemPathForRead(path)) {
+    delete this;
     return;
+  }
   FileSystemOperation::ReadDirectory(path);
 }
 
 void SandboxedFileSystemOperation::Remove(
     const FilePath& path, bool recursive) {
-  if (!VerifyFileSystemPathForWrite(path, false /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, false /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Remove(path, recursive);
 }
 
@@ -103,15 +121,19 @@ void SandboxedFileSystemOperation::Write(
     scoped_refptr<URLRequestContext> url_request_context,
     const FilePath& path, const GURL& blob_url, int64 offset) {
   if (!VerifyFileSystemPathForWrite(path, true /* create */,
-                                    FileSystemQuotaManager::kUnknownSize))
+                                    FileSystemQuotaManager::kUnknownSize)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Write(url_request_context, path, blob_url, offset);
 }
 
 void SandboxedFileSystemOperation::Truncate(
     const FilePath& path, int64 length) {
-  if (!VerifyFileSystemPathForWrite(path, false /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, false /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Truncate(path, length);
 }
 
@@ -119,8 +141,10 @@ void SandboxedFileSystemOperation::TouchFile(
     const FilePath& path,
     const base::Time& last_access_time,
     const base::Time& last_modified_time) {
-  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::TouchFile(path, last_access_time, last_modified_time);
 }
 
@@ -128,6 +152,7 @@ void SandboxedFileSystemOperation::DidGetRootPath(
     bool success, const FilePath& path, const std::string& name) {
   DCHECK(success || path.empty());
   dispatcher()->DidOpenFileSystem(name, path);
+  delete this;
 }
 
 bool SandboxedFileSystemOperation::VerifyFileSystemPathForRead(
diff --git a/webkit/fileapi/sandboxed_file_system_operation.h b/webkit/fileapi/sandboxed_file_system_operation.h
index 18d47f7899270..8794a9dbf085f 100644
--- a/webkit/fileapi/sandboxed_file_system_operation.h
+++ b/webkit/fileapi/sandboxed_file_system_operation.h
@@ -71,6 +71,8 @@ class SandboxedFileSystemOperation : public FileSystemOperation {
   // Returns true if the given |path| is a valid FileSystem path.
   // Otherwise it calls dispatcher's DidFail method with
   // PLATFORM_FILE_ERROR_SECURITY and returns false.
+  // (Note: this doesn't delete this when it calls DidFail and returns false;
+  // it's the caller's responsibility.)
   bool VerifyFileSystemPathForRead(const FilePath& path);
 
   // Checks the validity of a given |path| for writing.
@@ -85,6 +87,8 @@ class SandboxedFileSystemOperation : public FileSystemOperation {
   // If |create| flag is true this also checks if the |path| contains
   // any restricted names and chars. If it does, the call fires dispatcher's
   // DidFail with PLATFORM_FILE_ERROR_SECURITY and returns false.
+  // (Note: this doesn't delete this when it calls DidFail and returns false;
+  // it's the caller's responsibility.)
   bool VerifyFileSystemPathForWrite(const FilePath& path,
                                     bool create,
                                     int64 growth);
diff --git a/webkit/tools/test_shell/simple_file_system.cc b/webkit/tools/test_shell/simple_file_system.cc
index 29ea45e323209..94269b97e3b8c 100644
--- a/webkit/tools/test_shell/simple_file_system.cc
+++ b/webkit/tools/test_shell/simple_file_system.cc
@@ -54,17 +54,11 @@ class SimpleFileSystemCallbackDispatcher
   }
 
   ~SimpleFileSystemCallbackDispatcher() {
-    DCHECK(!operation_.get());
-  }
-
-  void set_operation(SandboxedFileSystemOperation* operation) {
-    operation_.reset(operation);
   }
 
   virtual void DidSucceed() {
-    if (file_system_)
-      callbacks_->didSucceed();
-    RemoveOperation();
+    DCHECK(file_system_);
+    callbacks_->didSucceed();
   }
 
   virtual void DidReadMetadata(const base::PlatformFileInfo& info) {
@@ -75,7 +69,6 @@ class SimpleFileSystemCallbackDispatcher
     web_file_info.type = info.is_directory ?
         WebFileInfo::TypeDirectory : WebFileInfo::TypeFile;
     callbacks_->didReadMetadata(web_file_info);
-    RemoveOperation();
   }
 
   virtual void DidReadDirectory(
@@ -93,7 +86,6 @@ class SimpleFileSystemCallbackDispatcher
     WebVector<WebKit::WebFileSystemEntry> web_entries =
         web_entries_vector;
     callbacks_->didReadDirectory(web_entries, has_more);
-    RemoveOperation();
   }
 
   virtual void DidOpenFileSystem(
@@ -104,14 +96,12 @@ class SimpleFileSystemCallbackDispatcher
     else
       callbacks_->didOpenFileSystem(
           UTF8ToUTF16(name), webkit_glue::FilePathToWebString(path));
-    RemoveOperation();
   }
 
   virtual void DidFail(base::PlatformFileError error_code) {
     DCHECK(file_system_);
     callbacks_->didFail(
         webkit_glue::PlatformFileErrorToWebFileError(error_code));
-    RemoveOperation();
   }
 
   virtual void DidWrite(int64, bool) {
@@ -119,17 +109,8 @@ class SimpleFileSystemCallbackDispatcher
   }
 
  private:
-  void RemoveOperation() {
-    // We need to make sure operation_ is null when we delete the operation
-    // (which in turn deletes this dispatcher instance).
-    scoped_ptr<SandboxedFileSystemOperation> operation;
-    operation.swap(operation_);
-    operation.reset();
-  }
-
   WeakPtr<SimpleFileSystem> file_system_;
   WebFileSystemCallbacks* callbacks_;
-  scoped_ptr<SandboxedFileSystemOperation> operation_;
 };
 
 }  // namespace
@@ -262,6 +243,5 @@ SandboxedFileSystemOperation* SimpleFileSystem::GetNewOperation(
   SandboxedFileSystemOperation* operation = new SandboxedFileSystemOperation(
       dispatcher, base::MessageLoopProxy::CreateForCurrentThread(),
       sandboxed_context_.get());
-  dispatcher->set_operation(operation);
   return operation;
 }
diff --git a/webkit/tools/test_shell/simple_file_writer.cc b/webkit/tools/test_shell/simple_file_writer.cc
index bb4758f3f5323..ad6109abd6393 100644
--- a/webkit/tools/test_shell/simple_file_writer.cc
+++ b/webkit/tools/test_shell/simple_file_writer.cc
@@ -28,7 +28,8 @@ class SimpleFileWriter::IOThreadProxy
     : public base::RefCountedThreadSafe<SimpleFileWriter::IOThreadProxy> {
  public:
   explicit IOThreadProxy(const base::WeakPtr<SimpleFileWriter>& simple_writer)
-      : simple_writer_(simple_writer) {
+      : simple_writer_(simple_writer),
+        operation_(NULL) {
     // The IO thread needs to be running for this class to work.
     SimpleResourceLoaderBridge::EnsureIOThread();
     io_thread_ = SimpleResourceLoaderBridge::GetIoThread();
@@ -44,8 +45,8 @@ class SimpleFileWriter::IOThreadProxy
           this, &IOThreadProxy::Truncate, path, offset));
       return;
     }
-    DCHECK(!operation_.get());
-    operation_.reset(GetNewOperation());
+    DCHECK(!operation_);
+    operation_ = GetNewOperation();
     operation_->Truncate(path, offset);
   }
 
@@ -56,8 +57,8 @@ class SimpleFileWriter::IOThreadProxy
       return;
     }
     DCHECK(request_context_);
-    DCHECK(!operation_.get());
-    operation_.reset(GetNewOperation());
+    DCHECK(!operation_);
+    operation_ = GetNewOperation();
     operation_->Write(request_context_, path, blob_url, offset);
   }
 
@@ -67,12 +68,11 @@ class SimpleFileWriter::IOThreadProxy
           this, &IOThreadProxy::Cancel));
       return;
     }
-    if (!operation_.get()) {
+    if (!operation_) {
       DidFail(base::PLATFORM_FILE_ERROR_INVALID_OPERATION);
       return;
     }
-    cancel_operation_.reset(GetNewOperation());
-    operation_->Cancel(cancel_operation_.get());
+    operation_->Cancel(GetNewOperation());
   }
 
  private:
@@ -82,6 +82,10 @@ class SimpleFileWriter::IOThreadProxy
     explicit CallbackDispatcher(IOThreadProxy* proxy) : proxy_(proxy) {
     }
 
+    ~CallbackDispatcher() {
+      proxy_->ClearOperation();
+    }
+
     virtual void DidSucceed() {
       proxy_->DidSucceed();
     }
@@ -119,7 +123,6 @@ class SimpleFileWriter::IOThreadProxy
 
   void DidSucceed() {
     if (!main_thread_->BelongsToCurrentThread()) {
-      operation_.reset();
       main_thread_->PostTask(FROM_HERE, NewRunnableMethod(
           this, &IOThreadProxy::DidSucceed));
       return;
@@ -130,7 +133,6 @@ class SimpleFileWriter::IOThreadProxy
 
   void DidFail(base::PlatformFileError error_code) {
     if (!main_thread_->BelongsToCurrentThread()) {
-      operation_.reset();
       main_thread_->PostTask(FROM_HERE, NewRunnableMethod(
           this, &IOThreadProxy::DidFail, error_code));
       return;
@@ -141,8 +143,6 @@ class SimpleFileWriter::IOThreadProxy
 
   void DidWrite(int64 bytes, bool complete) {
     if (!main_thread_->BelongsToCurrentThread()) {
-      if (complete)
-        operation_.reset();
       main_thread_->PostTask(FROM_HERE, NewRunnableMethod(
           this, &IOThreadProxy::DidWrite, bytes, complete));
       return;
@@ -151,6 +151,11 @@ class SimpleFileWriter::IOThreadProxy
       simple_writer_->DidWrite(bytes, complete);
   }
 
+  void ClearOperation() {
+    DCHECK(io_thread_->BelongsToCurrentThread());
+    operation_ = NULL;
+  }
+
   scoped_refptr<base::MessageLoopProxy> io_thread_;
   scoped_refptr<base::MessageLoopProxy> main_thread_;
 
@@ -158,8 +163,7 @@ class SimpleFileWriter::IOThreadProxy
   base::WeakPtr<SimpleFileWriter> simple_writer_;
 
   // Only used on the io thread.
-  scoped_ptr<FileSystemOperation> operation_;
-  scoped_ptr<FileSystemOperation> cancel_operation_;
+  FileSystemOperation* operation_;
 };
 
 
-- 
GitLab