[fsp] Decouple file_service_provider::Service.

chrome/browser/chromeos/file_system_provider/provided_file_system.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/provided_file_system.h"
 
+#include "base/files/file.h"
+#include "base/values.h"
+#include "chrome/browser/chromeos/file_system_provider/request_manager.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/api/file_system_provider.h"
+#include "extensions/browser/event_router.h"
+#include "extensions/browser/extension_system.h"
+
 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() {}
 
-ProvidedFileSystem::ProvidedFileSystem(const std::string& extension_id,
-                                       int file_system_id,
-                                       const std::string& file_system_name,
-                                       const base::FilePath& mount_path)
-    : extension_id_(extension_id),
-      file_system_id_(file_system_id),
-      file_system_name_(file_system_name),
-      mount_path_(mount_path) {}
+ProvidedFileSystem::ProvidedFileSystem(
+    extensions::EventRouter* event_router,
+    RequestManager* request_manager,
+    const ProvidedFileSystemInfo& file_system_info)
+    : event_router_(event_router),
+      request_manager_(request_manager),
+      file_system_info_(file_system_info) {}
 
 ProvidedFileSystem::~ProvidedFileSystem() {}
 
+bool ProvidedFileSystem::RequestUnmount(
+    const fileapi::AsyncFileUtil::StatusCallback& callback) {
+  int request_id = request_manager_->CreateRequest(
+      file_system_info_.extension_id(),
+      file_system_info_.file_system_id(),
+      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;
+}
+
+const ProvidedFileSystemInfo& ProvidedFileSystem::GetFileSystemInfo() const {
+  return file_system_info_;
+}
+
+ProvidedFileSystemFactory::ProvidedFileSystemFactory() {}
+
+ProvidedFileSystemFactory::~ProvidedFileSystemFactory() {}
+
+ProvidedFileSystemInterface* ProvidedFileSystemFactory::Create(
+    Profile* profile,

[hashimoto, 2014/04/11 06:01:50]

Why doesn't this method take event_router instead of profile?

[mtomasz, 2014/04/11 07:11:56]

I tried it, but it would be difficult to test. Provided_file_system_unittest.cc
creates an instance with a fake event router.

[hashimoto, 2014/04/14 10:43:42]

Sorry, I still don't understand why changing this method's argument can cause
problems in provided_file_system_unittest.cc
Can you elaborate on this?

[mtomasz, 2014/04/15 02:46:40]

Yep, I must have been tired writing that comment :p.

The problem is that extensions::ExtensionSystem::Get(profile)->event_router()
returns NULL for a testing profile. So if we pass TestingProfile in unit tests,
ProvidedFileSystem will fail because the event router is NULL. Also,
ProvidedFileSystem doesn't use Profile, so there seem to be no need to add this
extra dependency. WDYT?

[hashimoto, 2014/04/15 07:22:45]

Sorry, I still don't understand why changing this method results in breaking
provided_file_system_unittest.cc.
AFAIK this method is only used by Service, but provided_file_system_unittest.cc
does not instantiate a Service.
Replacing |profile| here with an EventRouter does not mean adding an extra
dependency.
It's narrowing dependency from the entire Profile to EventRouter only.

[mtomasz, 2014/04/15 09:42:12]

Per offline talk. I misread your first comment. Sorry! Of course we can change
the Profile with an EventRouter here.

+    RequestManager* request_manager,
+    const ProvidedFileSystemInfo& file_system_info) {
+  DCHECK(profile);
+  DCHECK(request_manager);
+  extensions::EventRouter* event_router =
+      extensions::ExtensionSystem::Get(profile)->event_router();
+  DCHECK(event_router);
+  return new ProvidedFileSystem(
+      event_router, request_manager, file_system_info);
+}
+
 }  // namespace file_system_provider
 }  // namespace chromeos