[fsp] Refactor handling operations.

This CL reorganizes code before adding the read directory implementation.

First of all, operations are now extracted to separate classes, to avoid huge
file_system_provider.cc. Such operation classes are now arguments for the
RequestManager which replaces the former pair of [success, error] callbacks.

Such operation classes are now easy to test, because of ability to cut out
the EventRouter by using Operation::SetDispatchEventImplForTests.

Finally, a RequestValue class has been introduced, which replaces former
base::DictionaryValue. The former solution was not good, since we already
parse the base::Value in api function implementations (using generated from
IDL parsers). So, passing a raw base::Value and later parsing is again is an
overkill.

The RequestValue class is a trivial wrapper for all kinds of IDL values, which
can be passed through the RequestManager to Operation classes.

TEST=unit_tests, browser_tests: *FileSystemProvider*
BUG=248427

Review URL: https://codereview.chromium.org/257493004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266516 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc

@@ -9,12 +9,14 @@
 #include "base/values.h"
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
 #include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "chrome/browser/chromeos/file_system_provider/request_value.h"
 #include "chrome/browser/chromeos/file_system_provider/service.h"
 #include "chrome/common/extensions/api/file_system_provider.h"
 #include "chrome/common/extensions/api/file_system_provider_internal.h"
 
 using chromeos::file_system_provider::ProvidedFileSystemInterface;
 using chromeos::file_system_provider::RequestManager;
+using chromeos::file_system_provider::RequestValue;
 using chromeos::file_system_provider::Service;
 
 namespace extensions {
@@ -151,7 +153,7 @@ bool FileSystemProviderUnmountFunction::RunImpl() {
 
 bool FileSystemProviderInternalUnmountRequestedSuccessFunction::RunImpl() {
   using api::file_system_provider_internal::UnmountRequestedSuccess::Params;
-  const scoped_ptr<Params> params(Params::Create(*args_));
+  scoped_ptr<Params> params(Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
 
   Service* service = Service::Get(GetProfile());
@@ -170,9 +172,11 @@ bool FileSystemProviderInternalUnmountRequestedSuccessFunction::RunImpl() {
   RequestManager* request_manager = file_system->GetRequestManager();
   DCHECK(request_manager);
 
-  if (!request_manager->FulfillRequest(params->request_id,
-                                       scoped_ptr<base::DictionaryValue>(),
-                                       false /* has_more */)) {
+  const int request_id = params->request_id;
+  if (!request_manager->FulfillRequest(
+          request_id,
+          RequestValue::CreateForUnmountSuccess(params.Pass()),
+          false /* has_more */)) {
     // TODO(mtomasz): Pass more detailed errors, rather than just a bool.
     base::ListValue* result = new base::ListValue();
     result->Append(

chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc

@@ -13,15 +13,15 @@ namespace file_system_provider {
 
 FakeProvidedFileSystem::FakeProvidedFileSystem(
     const ProvidedFileSystemInfo& file_system_info)
-    : file_system_info_(file_system_info) {}
+    : file_system_info_(file_system_info) {
+}
 
 FakeProvidedFileSystem::~FakeProvidedFileSystem() {}
 
-bool FakeProvidedFileSystem::RequestUnmount(
+void FakeProvidedFileSystem::RequestUnmount(
     const fileapi::AsyncFileUtil::StatusCallback& callback) {
   base::MessageLoopProxy::current()->PostTask(
       FROM_HERE, base::Bind(callback, base::File::FILE_OK));
-  return true;
 }
 
 const ProvidedFileSystemInfo& FakeProvidedFileSystem::GetFileSystemInfo()

chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h

@@ -28,7 +28,7 @@ class FakeProvidedFileSystem : public ProvidedFileSystemInterface {
   virtual ~FakeProvidedFileSystem();
 
   // ProvidedFileSystemInterface overrides.
-  virtual bool RequestUnmount(
+  virtual void RequestUnmount(
       const fileapi::AsyncFileUtil::StatusCallback& callback) OVERRIDE;
   virtual const ProvidedFileSystemInfo& GetFileSystemInfo() const OVERRIDE;
   virtual RequestManager* GetRequestManager() OVERRIDE;
@@ -41,6 +41,7 @@ class FakeProvidedFileSystem : public ProvidedFileSystemInterface {
 
  private:
   ProvidedFileSystemInfo file_system_info_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeProvidedFileSystem);
 };
 

chrome/browser/chromeos/file_system_provider/operations/operation.cc

+// Copyright 2014 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/chromeos/file_system_provider/operations/operation.h"
+
+#include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
+#include "extensions/browser/event_router.h"
+
+namespace chromeos {
+namespace file_system_provider {
+namespace operations {
+namespace {
+
+// Default implementation for dispatching an event. Can be replaced for unit
+// tests by Operation::SetDispatchEventImplForTest().
+bool DispatchEventImpl(extensions::EventRouter* event_router,
+                       const std::string& extension_id,
+                       scoped_ptr<extensions::Event> event) {
+  if (!event_router->ExtensionHasEventListener(extension_id, event->event_name))
+    return false;
+
+  event_router->DispatchEventToExtension(extension_id, event.Pass());
+  return true;
+}
+
+}  // namespace
+
+Operation::Operation(extensions::EventRouter* event_router,
+                     const ProvidedFileSystemInfo& file_system_info)
+    : file_system_info_(file_system_info),
+      dispatch_event_impl_(base::Bind(&DispatchEventImpl,
+                                      event_router,
+                                      file_system_info_.extension_id())) {
+}
+
+Operation::~Operation() {
+}
+
+void Operation::SetDispatchEventImplForTesting(
+    const DispatchEventImplCallback& callback) {
+  dispatch_event_impl_ = callback;
+}
+
+bool Operation::SendEvent(int request_id,
+                          const std::string& event_name,
+                          scoped_ptr<base::ListValue> event_args) {
+  event_args->Insert(
+      0, new base::FundamentalValue(file_system_info_.file_system_id()));
+  event_args->Insert(1, new base::FundamentalValue(request_id));
+
+  return dispatch_event_impl_.Run(
+      make_scoped_ptr(new extensions::Event(event_name, event_args.Pass())));
+}
+
+}  // namespace operations
+}  // namespace file_system_provider
+}  // namespace chromeos

chrome/browser/chromeos/file_system_provider/operations/operation.h

+// Copyright 2014 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_CHROMEOS_FILE_SYSTEM_PROVIDER_OPERATIONS_OPERATION_H_
+#define CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_OPERATIONS_OPERATION_H_
+
+#include <string>
+
+#include "base/files/file.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
+#include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "webkit/browser/fileapi/async_file_util.h"
+
+namespace base {
+class ListValue;
+}  // namespace base
+
+namespace extensions {
+struct Event;
+class EventRouter;
+}  // namespace extensions
+
+namespace chromeos {
+namespace file_system_provider {
+namespace operations {
+
+// Base class for operation bridges between fileapi and providing extensions.
+class Operation : public RequestManager::HandlerInterface {
+ public:
+  typedef base::Callback<bool(scoped_ptr<extensions::Event> event)>
+      DispatchEventImplCallback;
+
+  Operation(extensions::EventRouter* event_router,
+            const ProvidedFileSystemInfo& file_system_info);
+  virtual ~Operation();
+
+  // RequestManager::HandlerInterface overrides.
+  virtual bool Execute(int request_id) OVERRIDE = 0;
+  virtual void OnSuccess(int request_id,
+                         scoped_ptr<RequestValue> result,
+                         bool has_next) OVERRIDE = 0;
+  virtual void OnError(int request_id, base::File::Error error) OVERRIDE = 0;
+
+  // Sets custom dispatchign event implementation for tests.
+  void SetDispatchEventImplForTesting(
+      const DispatchEventImplCallback& callback);
+
+ protected:
+  // Sends an event to the providing extension. Automatically adds the file
+  // system id and the request id fields. Returns false, if the providing
+  // extension does not handle the |event_name| event.
+  bool SendEvent(int request_id,
+                 const std::string& event_name,
+                 scoped_ptr<base::ListValue> event_args);
+
+  ProvidedFileSystemInfo file_system_info_;
+
+ private:
+  DispatchEventImplCallback dispatch_event_impl_;
+  DISALLOW_COPY_AND_ASSIGN(Operation);
+};
+
+}  // namespace operations
+}  // namespace file_system_provider
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_OPERATIONS_OPERATION_H_

chrome/browser/chromeos/file_system_provider/operations/unmount.cc

+// Copyright 2014 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/chromeos/file_system_provider/operations/unmount.h"
+
+#include "base/values.h"
+#include "chrome/common/extensions/api/file_system_provider.h"
+
+namespace chromeos {
+namespace file_system_provider {
+namespace operations {
+
+Unmount::Unmount(extensions::EventRouter* event_router,
+                 const ProvidedFileSystemInfo& file_system_info,
+                 const fileapi::AsyncFileUtil::StatusCallback& callback)
+    : Operation(event_router, file_system_info), callback_(callback) {
+}
+
+Unmount::~Unmount() {
+}
+
+bool Unmount::Execute(int request_id) {
+  scoped_ptr<base::ListValue> values(new base::ListValue);
+  return SendEvent(
+      request_id,
+      extensions::api::file_system_provider::OnUnmountRequested::kEventName,
+      values.Pass());
+}
+
+void Unmount::OnSuccess(int /* request_id */,
+                        scoped_ptr<RequestValue> /* result */,
+                        bool /* has_next */) {
+  callback_.Run(base::File::FILE_OK);
+}
+
+void Unmount::OnError(int /* request_id */, base::File::Error error) {
+  callback_.Run(error);
+}
+
+}  // namespace operations
+}  // namespace file_system_provider
+}  // namespace chromeos

chrome/browser/chromeos/file_system_provider/operations/unmount.h

+// Copyright 2014 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_CHROMEOS_FILE_SYSTEM_PROVIDER_OPERATIONS_UNMOUNT_H_
+#define CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_OPERATIONS_UNMOUNT_H_
+
+#include "base/files/file.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/chromeos/file_system_provider/operations/operation.h"
+#include "webkit/browser/fileapi/async_file_util.h"
+
+namespace base {
+class DictionaryValue;
+}  // namespace base
+
+namespace extensions {
+class EventRouter;
+}  // namespace extensions
+
+namespace chromeos {
+namespace file_system_provider {
+
+class ProvidedFileSystemInfo;
+
+namespace operations {
+
+// Bridge between fileBrowserPrivate's unmount operation and providing
+// extension's unmount request. Created per request.
+class Unmount : public Operation {
+ public:
+  Unmount(extensions::EventRouter* event_router,
+          const ProvidedFileSystemInfo& file_system_info,
+          const fileapi::AsyncFileUtil::StatusCallback& callback);
+  virtual ~Unmount();
+
+  // Operation overrides.
+  virtual bool Execute(int request_id) OVERRIDE;
+  virtual void OnSuccess(int request_id,
+                         scoped_ptr<RequestValue> result,
+                         bool has_next) OVERRIDE;
+  virtual void OnError(int request_id, base::File::Error error) OVERRIDE;
+
+ private:
+  const fileapi::AsyncFileUtil::StatusCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(Unmount);
+};
+
+}  // namespace operations
+}  // namespace file_system_provider
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_OPERATIONS_UNMOUNT_H_

chrome/browser/chromeos/file_system_provider/provided_file_system.cc

@@ -5,7 +5,7 @@
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system.h"
 
 #include "base/files/file.h"
-#include "base/values.h"
+#include "chrome/browser/chromeos/file_system_provider/operations/unmount.h"
 #include "chrome/browser/chromeos/file_system_provider/request_manager.h"
 #include "chrome/common/extensions/api/file_system_provider.h"
 #include "extensions/browser/event_router.h"
@@ -14,52 +14,24 @@ namespace chromeos {
 namespace file_system_provider {
 namespace {
 
-// Creates values to be passed to request events. These values can be extended
-// by additional fields.
-scoped_ptr<base::ListValue> CreateRequestValues(int file_system_id,
-                                                int request_id) {
-  scoped_ptr<base::ListValue> values(new base::ListValue());
-  values->AppendInteger(file_system_id);
-  values->AppendInteger(request_id);
-  return values.Pass();
-}
-
-// Forwards the success callback to the status callback. Ignores arguments,
-// since unmount request does not provide arguments.
-void OnRequestUnmountSuccess(
-    const fileapi::AsyncFileUtil::StatusCallback& callback,
-    scoped_ptr<base::DictionaryValue> /* result */,
-    bool /* has_next */) {
-  callback.Run(base::File::FILE_OK);
-}
-
 }  // namespace
 
 ProvidedFileSystem::ProvidedFileSystem(
     extensions::EventRouter* event_router,
     const ProvidedFileSystemInfo& file_system_info)
-    : event_router_(event_router), file_system_info_(file_system_info) {}
+    : event_router_(event_router), file_system_info_(file_system_info) {
+}
 
 ProvidedFileSystem::~ProvidedFileSystem() {}
 
-bool ProvidedFileSystem::RequestUnmount(
+void ProvidedFileSystem::RequestUnmount(
     const fileapi::AsyncFileUtil::StatusCallback& callback) {
-  int request_id = request_manager_.CreateRequest(
-      base::Bind(&OnRequestUnmountSuccess, callback), callback);
-
-  if (!request_id)
-    return false;
-
-  scoped_ptr<base::ListValue> values(
-      CreateRequestValues(file_system_info_.file_system_id(), request_id));
-
-  event_router_->DispatchEventToExtension(
-      file_system_info_.extension_id(),
-      make_scoped_ptr(new extensions::Event(
-          extensions::api::file_system_provider::OnUnmountRequested::kEventName,
-          values.Pass())));
-
-  return true;
+  if (!request_manager_.CreateRequest(
+          make_scoped_ptr<RequestManager::HandlerInterface>(
+              new operations::Unmount(
+                  event_router_, file_system_info_, callback)))) {
+    callback.Run(base::File::FILE_ERROR_SECURITY);
+  }
 }
 
 const ProvidedFileSystemInfo& ProvidedFileSystem::GetFileSystemInfo() const {

chrome/browser/chromeos/file_system_provider/provided_file_system.h

@@ -8,6 +8,11 @@
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
 #include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "webkit/browser/fileapi/async_file_util.h"
+
+namespace base {
+class FilePath;
+}  // namespace base
 
 namespace extensions {
 class EventRouter;
@@ -25,7 +30,7 @@ class ProvidedFileSystem : public ProvidedFileSystemInterface {
   virtual ~ProvidedFileSystem();
 
   // ProvidedFileSystemInterface overrides.
-  virtual bool RequestUnmount(
+  virtual void RequestUnmount(
       const fileapi::AsyncFileUtil::StatusCallback& callback) OVERRIDE;
   virtual const ProvidedFileSystemInfo& GetFileSystemInfo() const OVERRIDE;
   virtual RequestManager* GetRequestManager() OVERRIDE;

chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h

@@ -9,6 +9,10 @@
 
 class EventRouter;
 
+namespace base {
+class FilePath;
+}  // namespace base
+
 namespace chromeos {
 namespace file_system_provider {
 
@@ -23,9 +27,8 @@ class ProvidedFileSystemInterface {
   virtual ~ProvidedFileSystemInterface() {}
 
   // Requests unmounting of the file system. The callback is called when the
-  // request is accepted or rejected, with an error code. Returns false if the
-  // request could not been created, true otherwise.
-  virtual bool RequestUnmount(
+  // request is accepted or rejected, with an error code.
+  virtual void RequestUnmount(
       const fileapi::AsyncFileUtil::StatusCallback& callback) = 0;
 
   // Returns a provided file system info for this file system.

chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
 #include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "chrome/common/extensions/api/file_system_provider.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "extensions/browser/event_router.h"
@@ -84,6 +85,11 @@ class FileSystemProviderProvidedFileSystemTest : public testing::Test {
   virtual void SetUp() OVERRIDE {
     profile_.reset(new TestingProfile);
     event_router_.reset(new FakeEventRouter(profile_.get()));
+    event_router_->AddEventListener(
+        extensions::api::file_system_provider::OnUnmountRequested::kEventName,
+        NULL,
+        kExtensionId);
+
     base::FilePath mount_path =
         util::GetMountPath(profile_.get(), kExtensionId, kFileSystemId);
     file_system_info_.reset(new ProvidedFileSystemInfo(
@@ -102,9 +108,8 @@ class FileSystemProviderProvidedFileSystemTest : public testing::Test {
 TEST_F(FileSystemProviderProvidedFileSystemTest, RequestUnmount_Success) {
   EventLogger logger;
 
-  bool result = provided_file_system_->RequestUnmount(
+  provided_file_system_->RequestUnmount(
       base::Bind(&EventLogger::OnStatusCallback, logger.GetWeakPtr()));
-  ASSERT_TRUE(result);
   base::RunLoop().RunUntilIdle();
 
   // Verify that the event has been sent to the providing extension.
@@ -129,7 +134,7 @@ TEST_F(FileSystemProviderProvidedFileSystemTest, RequestUnmount_Success) {
   // Simulate sending a success response from the providing extension.
   RequestManager* request_manager = provided_file_system_->GetRequestManager();
   ASSERT_TRUE(request_manager);
-  scoped_ptr<base::DictionaryValue> response(new base::DictionaryValue());
+  scoped_ptr<RequestValue> response;
   bool reply_result = request_manager->FulfillRequest(
       request_id, response.Pass(), false /* has_next */);
   EXPECT_TRUE(reply_result);
@@ -142,9 +147,8 @@ TEST_F(FileSystemProviderProvidedFileSystemTest, RequestUnmount_Success) {
 TEST_F(FileSystemProviderProvidedFileSystemTest, RequestUnmount_Error) {
   EventLogger logger;
 
-  bool result = provided_file_system_->RequestUnmount(
+  provided_file_system_->RequestUnmount(
       base::Bind(&EventLogger::OnStatusCallback, logger.GetWeakPtr()));
-  ASSERT_TRUE(result);
   base::RunLoop().RunUntilIdle();
 
   // Verify that the event has been sent to the providing extension.

chrome/browser/chromeos/file_system_provider/request_manager.cc

@@ -6,7 +6,6 @@
 
 #include "base/files/file.h"
 #include "base/stl_util.h"
-#include "base/values.h"
 
 namespace chromeos {
 namespace file_system_provider {
@@ -37,8 +36,7 @@ RequestManager::~RequestManager() {
   STLDeleteValues(&requests_);
 }
 
-int RequestManager::CreateRequest(const SuccessCallback& success_callback,
-                                  const ErrorCallback& error_callback) {
+int RequestManager::CreateRequest(scoped_ptr<HandlerInterface> handler) {
   // The request id is unique per request manager, so per service, thereof
   // per profile.
   int request_id = next_id_++;
@@ -48,8 +46,7 @@ int RequestManager::CreateRequest(const SuccessCallback& success_callback,
     return 0;
 
   Request* request = new Request;
-  request->success_callback = success_callback;
-  request->error_callback = error_callback;
+  request->handler = handler.Pass();
   request->timeout_timer.Start(FROM_HERE,
                                timeout_,
                                base::Bind(&RequestManager::OnRequestTimeout,
@@ -57,19 +54,28 @@ int RequestManager::CreateRequest(const SuccessCallback& success_callback,
                                           request_id));
   requests_[request_id] = request;
 
+  // Execute the request implementation. In case of an execution failure,
+  // unregister and return 0. This may often happen, eg. if the providing
+  // extension is not listening for the request event being sent.
+  // In such case, there is no reason we should abort as soon as possible.
+  if (!request->handler->Execute(request_id)) {
+    delete request;
+    requests_.erase(request_id);
+    return 0;
+  }
+
   return request_id;
 }
 
 bool RequestManager::FulfillRequest(int request_id,
-                                    scoped_ptr<base::DictionaryValue> response,
+                                    scoped_ptr<RequestValue> response,
                                     bool has_next) {
   RequestMap::iterator request_it = requests_.find(request_id);
 
   if (request_it == requests_.end())
     return false;
 
-  if (!request_it->second->success_callback.is_null())
-    request_it->second->success_callback.Run(response.Pass(), has_next);
+  request_it->second->handler->OnSuccess(request_id, response.Pass(), has_next);
   if (!has_next) {
     delete request_it->second;
     requests_.erase(request_it);
@@ -86,8 +92,7 @@ bool RequestManager::RejectRequest(int request_id, base::File::Error error) {
   if (request_it == requests_.end())
     return false;
 
-  if (!request_it->second->error_callback.is_null())
-    request_it->second->error_callback.Run(error);
+  request_it->second->handler->OnError(request_id, error);
   delete request_it->second;
   requests_.erase(request_it);
 

chrome/browser/chromeos/file_system_provider/request_manager.h

@@ -15,35 +15,48 @@
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system_info.h"
-
-namespace base {
-class DictionaryValue;
-}  // namespace base
+#include "chrome/browser/chromeos/file_system_provider/request_value.h"
 
 namespace chromeos {
 namespace file_system_provider {
 
-typedef base::Callback<void(scoped_ptr<base::DictionaryValue> result,
-                            bool has_next)> SuccessCallback;
-typedef base::Callback<void(base::File::Error)> ErrorCallback;
-
 // Manages requests between the service, async utils and the providing
 // extensions.
 class RequestManager {
  public:
+  class HandlerInterface {
+   public:
+    virtual ~HandlerInterface() {}
+
+    // Called when the request is created. Executes the request implementation.
+    // Returns false in case of a execution failure.
+    virtual bool Execute(int request_id) = 0;
+
+    // Success callback invoked by the providing extension in response to
+    // Execute(). It may be called more than once, until |has_next| is set to
+    // false.
+    virtual void OnSuccess(int request_id,
+                           scoped_ptr<RequestValue> result,
+                           bool has_next) = 0;
+
+    // Error callback invoked by the providing extension in response to
+    // Execute(). It can be called at most once. It can be also called if the
+    // request is aborted due to a timeout.
+    virtual void OnError(int request_id, base::File::Error error) = 0;
+  };
+
   RequestManager();
   virtual ~RequestManager();
 
   // Creates a request and returns its request id (greater than 0). Returns 0 in
-  // case of an error (eg. too many requests). The passed callbacks can be NULL.
-  int CreateRequest(const SuccessCallback& success_callback,
-                    const ErrorCallback& error_callback);
+  // case of an error (eg. too many requests).
+  int CreateRequest(scoped_ptr<HandlerInterface> handler);
 
   // Handles successful response for the |request_id|. If |has_next| is false,
   // then the request is disposed, after handling the |response|. On error,
   // returns false, and the request is disposed.
   bool FulfillRequest(int request_id,
-                      scoped_ptr<base::DictionaryValue> response,
+                      scoped_ptr<RequestValue> response,
                       bool has_next);
 
   // Handles error response for the |request_id|. If handling the error fails,
@@ -62,11 +75,8 @@ class RequestManager {
     // Timer for discarding the request during a timeout.
     base::OneShotTimer<RequestManager> timeout_timer;
 
-    // Callback to be called on success.
-    SuccessCallback success_callback;
-
-    // Callback to be called on error.
-    ErrorCallback error_callback;
+    // Handler tied to this request.
+    scoped_ptr<HandlerInterface> handler;
 
    private:
     DISALLOW_COPY_AND_ASSIGN(Request);

chrome/browser/chromeos/file_system_provider/request_value.cc

+// Copyright 2014 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/chromeos/file_system_provider/request_value.h"
+
+namespace chromeos {
+namespace file_system_provider {
+
+RequestValue::RequestValue() {
+}
+
+RequestValue::~RequestValue() {
+}
+
+scoped_ptr<RequestValue> RequestValue::CreateForUnmountSuccess(
+    scoped_ptr<extensions::api::file_system_provider_internal::
+                   UnmountRequestedSuccess::Params> params) {
+  scoped_ptr<RequestValue> result(new RequestValue);
+  result->unmount_success_params_ = params.Pass();
+  return result.Pass();
+}
+
+scoped_ptr<RequestValue> RequestValue::CreateForTesting(
+    const std::string& params) {
+  scoped_ptr<RequestValue> result(new RequestValue);
+  result->testing_params_.reset(new std::string(params));
+  return result.Pass();
+}
+
+}  // namespace file_system_provider
+}  // namespace chromeos

chrome/browser/chromeos/file_system_provider/request_value.h

+// Copyright 2014 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_CHROMEOS_FILE_SYSTEM_PROVIDER_REQUEST_VALUE_H_
+#define CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_REQUEST_VALUE_H_
+
+#include <string>
+
+#include "base/memory/scoped_ptr.h"
+#include "chrome/common/extensions/api/file_system_provider_internal.h"
+
+namespace chromeos {
+namespace file_system_provider {
+
+// Holds a parsed value returned by a providing extension. Each accessor can
+// return NULL in case the requested value type is not available. It is used
+// to pass values of success callbacks.
+class RequestValue {
+ public:
+  // Creates an empty value. Use static methods to create a value holding a
+  // proper content.
+  RequestValue();
+
+  virtual ~RequestValue();
+
+  static scoped_ptr<RequestValue> CreateForUnmountSuccess(
+      scoped_ptr<extensions::api::file_system_provider_internal::
+                     UnmountRequestedSuccess::Params> params);
+
+  static scoped_ptr<RequestValue> CreateForTesting(const std::string& params);
+
+  const std::string* testing_params() const { return testing_params_.get(); }
+
+  const extensions::api::file_system_provider_internal::
+      UnmountRequestedSuccess::Params*
+      unmount_success_params() const {
+    return unmount_success_params_.get();
+  }
+
+ private:
+  scoped_ptr<extensions::api::file_system_provider_internal::
+                 UnmountRequestedSuccess::Params> unmount_success_params_;
+  scoped_ptr<std::string> testing_params_;
+
+  DISALLOW_COPY_AND_ASSIGN(RequestValue);
+};
+
+}  // namespace file_system_provider
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_FILE_SYSTEM_PROVIDER_REQUEST_VALUE_H_

chrome/browser/chromeos/file_system_provider/service.cc

@@ -47,6 +47,8 @@ Service::Service(Profile* profile,
 }
 
 Service::~Service() {
+  extension_registry_->RemoveObserver(this);
+
   ProvidedFileSystemMap::iterator it = file_system_map_.begin();
   while (it != file_system_map_.end()) {
     const int file_system_id = it->first;
@@ -204,10 +206,11 @@ bool Service::RequestUnmount(int file_system_id) {
   if (file_system_it == file_system_map_.end())
     return false;
 
-  return file_system_it->second->RequestUnmount(
+  file_system_it->second->RequestUnmount(
       base::Bind(&Service::OnRequestUnmountStatus,
                  weak_ptr_factory_.GetWeakPtr(),
                  file_system_it->second->GetFileSystemInfo()));
+  return true;
 }
 
 std::vector<ProvidedFileSystemInfo> Service::GetProvidedFileSystemInfoList() {

chrome/chrome_browser_chromeos.gypi

@@ -379,6 +379,10 @@
         'browser/chromeos/file_system_provider/mount_path_util.cc',
         'browser/chromeos/file_system_provider/mount_path_util.h',
         'browser/chromeos/file_system_provider/observer.h',
+        'browser/chromeos/file_system_provider/operations/operation.cc',
+        'browser/chromeos/file_system_provider/operations/operation.h',
+        'browser/chromeos/file_system_provider/operations/unmount.cc',
+        'browser/chromeos/file_system_provider/operations/unmount.h',
         'browser/chromeos/file_system_provider/provided_file_system.cc',
         'browser/chromeos/file_system_provider/provided_file_system.h',
         'browser/chromeos/file_system_provider/provided_file_system_info.cc',
@@ -386,6 +390,8 @@
         'browser/chromeos/file_system_provider/provided_file_system_interface.h',
         'browser/chromeos/file_system_provider/request_manager.cc',
         'browser/chromeos/file_system_provider/request_manager.h',
+        'browser/chromeos/file_system_provider/request_value.cc',
+        'browser/chromeos/file_system_provider/request_value.h',
         'browser/chromeos/file_system_provider/service.cc',
         'browser/chromeos/file_system_provider/service.h',
         'browser/chromeos/file_system_provider/service_factory.cc',