[fsp] Decouple file_service_provider::Service.

chrome/browser/chromeos/file_system_provider/provided_file_system.cc

Index: chrome/browser/chromeos/file_system_provider/provided_file_system.cc
diff --git a/chrome/browser/chromeos/file_system_provider/provided_file_system.cc b/chrome/browser/chromeos/file_system_provider/provided_file_system.cc
index 8d669afd31a467a82409229054b62a7029c3d8c1..8f0cee098b856c42bc1616af3e9b1723b383890d 100644
--- a/chrome/browser/chromeos/file_system_provider/provided_file_system.cc
+++ b/chrome/browser/chromeos/file_system_provider/provided_file_system.cc
@@ -4,21 +4,94 @@
 
 #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