[fsp] Decouple file_service_provider::Service.

chrome/browser/chromeos/file_system_provider/service.cc

Index: chrome/browser/chromeos/file_system_provider/service.cc
diff --git a/chrome/browser/chromeos/file_system_provider/service.cc b/chrome/browser/chromeos/file_system_provider/service.cc
index b03779cf120c6c898eb02ceb06a1511be49ef151..29b2b86740644cf9e504f6a06d01452e6dad1a0b 100644
--- a/chrome/browser/chromeos/file_system_provider/service.cc
+++ b/chrome/browser/chromeos/file_system_provider/service.cc
@@ -6,13 +6,13 @@
 
 #include "base/files/file_path.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/values.h"
 #include "chrome/browser/chromeos/file_system_provider/observer.h"
 #include "chrome/browser/chromeos/file_system_provider/provided_file_system.h"
+#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/service_factory.h"
 #include "chrome/browser/chromeos/login/user.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
-#include "chrome/common/extensions/api/file_system_provider.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_system.h"
@@ -44,16 +44,6 @@ base::FilePath GetMountPointPath(Profile* profile,
       extension_id + "-" + base::IntToString(file_system_id) + user_suffix);
 }
 
-// Utility function, 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();
-}
-
 }  // namespace
 
 Service::Service(Profile* profile)
@@ -89,12 +79,12 @@ int Service::MountFileSystem(const std::string& extension_id,
     FOR_EACH_OBSERVER(
         Observer,
         observers_,
-        OnProvidedFileSystemMount(ProvidedFileSystem(),
+        OnProvidedFileSystemMount(ProvidedFileSystemInfo(),
                                   base::File::FILE_ERROR_TOO_MANY_OPENED));
     return 0;
   }
 
-  // The file system id is unique per service, so per profile.
+  // The provided file system id is unique per service, so per profile.
   int file_system_id = next_id_;
 
   fileapi::ExternalMountPoints* const mount_points =
@@ -115,7 +105,7 @@ int Service::MountFileSystem(const std::string& extension_id,
     FOR_EACH_OBSERVER(
         Observer,
         observers_,
-        OnProvidedFileSystemMount(ProvidedFileSystem(),
+        OnProvidedFileSystemMount(ProvidedFileSystemInfo(),
                                   base::File::FILE_ERROR_INVALID_OPERATION));
     return 0;
   }
@@ -126,14 +116,23 @@ int Service::MountFileSystem(const std::string& extension_id,
   //   file_system_id = 41
   //   mount_point_name = file_system_id = b33f1337-41-5aa5
   //   mount_point_path = /provided/b33f1337-41-5aa5
-  ProvidedFileSystem file_system(
+  ProvidedFileSystemInfo file_system_info(
       extension_id, file_system_id, file_system_name, mount_point_path);
-  file_systems_[file_system_id] = file_system;
+
+  // Create and add the provided file system to the internal map.
+  extensions::EventRouter* event_router =
+      extensions::ExtensionSystem::Get(profile_)->event_router();
+  DCHECK(event_router);
+
+  ProvidedFileSystem* file_system =
+      new ProvidedFileSystem(event_router, &request_manager_, file_system_info);
+  file_systems_.push_back(file_system);
+  file_system_map_[file_system_id] = file_system;
 
   FOR_EACH_OBSERVER(
       Observer,
       observers_,
-      OnProvidedFileSystemMount(file_system, base::File::FILE_OK));
+      OnProvidedFileSystemMount(file_system_info, base::File::FILE_OK));
 
   next_id_++;
   return file_system_id;
@@ -143,14 +142,17 @@ bool Service::UnmountFileSystem(const std::string& extension_id,
                                 int file_system_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  FileSystemMap::iterator file_system_it = file_systems_.find(file_system_id);
-  if (file_system_it == file_systems_.end() ||
-      file_system_it->second.extension_id() != extension_id) {
-    const ProvidedFileSystem empty_file_system;
-    FOR_EACH_OBSERVER(Observer,
-                      observers_,
-                      OnProvidedFileSystemUnmount(
-                          empty_file_system, base::File::FILE_ERROR_NOT_FOUND));
+  ProvidedFileSystemMap::iterator file_system_it =
+      file_system_map_.find(file_system_id);
+  if (file_system_it == file_system_map_.end() ||
+      file_system_it->second->GetFileSystemInfo().extension_id() !=
+          extension_id) {
+    const ProvidedFileSystemInfo empty_file_system_info;
+    FOR_EACH_OBSERVER(
+        Observer,
+        observers_,
+        OnProvidedFileSystemUnmount(empty_file_system_info,
+                                    base::File::FILE_ERROR_NOT_FOUND));
     return false;
   }
 
@@ -158,13 +160,16 @@ bool Service::UnmountFileSystem(const std::string& extension_id,
       fileapi::ExternalMountPoints::GetSystemInstance();
   DCHECK(mount_points);
 
+  const ProvidedFileSystemInfo file_system_info =

[hashimoto, 2014/03/26 03:03:01]

nit: const ProvidedFileSystemInfo&?

[mtomasz, 2014/03/26 05:45:13]

Done.

[mtomasz, 2014/03/26 05:45:13]

Done.

+      file_system_it->second->GetFileSystemInfo();
+
   const std::string mount_point_name =
-      file_system_it->second.mount_path().BaseName().value();
+      file_system_info.mount_path().BaseName().value();
   if (!mount_points->RevokeFileSystem(mount_point_name)) {
     FOR_EACH_OBSERVER(
         Observer,
         observers_,
-        OnProvidedFileSystemUnmount(file_system_it->second,
+        OnProvidedFileSystemUnmount(file_system_info,
                                     base::File::FILE_ERROR_INVALID_OPERATION));
     return false;
   }
@@ -172,96 +177,74 @@ bool Service::UnmountFileSystem(const std::string& extension_id,
   FOR_EACH_OBSERVER(
       Observer,
       observers_,
-      OnProvidedFileSystemUnmount(file_system_it->second, base::File::FILE_OK));
+      OnProvidedFileSystemUnmount(file_system_info, base::File::FILE_OK));
 
-  file_systems_.erase(file_system_it);
+  file_system_map_.erase(file_system_it);
+  file_systems_.erase(std::find(
+      file_systems_.begin(), file_systems_.end(), file_system_it->second));
   return true;
 }
 
-std::vector<ProvidedFileSystem> Service::GetMountedFileSystems() {
+bool Service::RequestUnmount(const std::string& extension_id,
+                             int file_system_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  std::vector<ProvidedFileSystem> result;
-  for (FileSystemMap::const_iterator it = file_systems_.begin();
-       it != file_systems_.end();
-       ++it) {
-    result.push_back(it->second);
-  }
-  return result;
-}
-
-bool Service::FulfillRequest(const std::string& extension_id,
-                             int file_system_id,
-                             int request_id,
-                             scoped_ptr<base::DictionaryValue> result,
-                             bool has_next) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-
-  FileSystemMap::iterator file_system_it = file_systems_.find(file_system_id);
-  if (file_system_it == file_systems_.end() ||
-      file_system_it->second.extension_id() != extension_id) {
+  ProvidedFileSystemMap::iterator file_system_it =
+      file_system_map_.find(file_system_id);
+  if (file_system_it == file_system_map_.end() ||
+      file_system_it->second->GetFileSystemInfo().extension_id() !=

[hashimoto, 2014/03/26 03:03:01]

Why do you need to check extension ID?
There can be multiple filesystems with the same filesystem ID?

[mtomasz, 2014/03/26 05:45:13]

You're right, this is wrong. I was comparing extension of the caller with the
provider extension. There should be no such check. It surprisingly passed tests,
because the provider is implemented in the same extension as the unmount caller.
I have plans to add tests with separated provider extensions from testing
extensions. 

Done.

+          extension_id) {
     return false;
   }
 
-  return request_manager_.FulfillRequest(
-      file_system_it->second, request_id, result.Pass(), has_next);
+  return file_system_it->second->RequestUnmount(
+      base::Bind(&Service::OnRequestUnmountStatus,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 file_system_it->second->GetFileSystemInfo()));
 }
 
-bool Service::RejectRequest(const std::string& extension_id,
-                            int file_system_id,
-                            int request_id,
-                            base::File::Error error) {
+std::vector<ProvidedFileSystemInfo> Service::GetProvidedFileSystemInfoList() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  FileSystemMap::iterator file_system_it = file_systems_.find(file_system_id);
-  if (file_system_it == file_systems_.end() ||
-      file_system_it->second.extension_id() != extension_id) {
-    return false;
+  std::vector<ProvidedFileSystemInfo> result;
+  for (ProvidedFileSystemMap::const_iterator it = file_system_map_.begin();
+       it != file_system_map_.end();
+       ++it) {
+    result.push_back(it->second->GetFileSystemInfo());
   }
-
-  return request_manager_.RejectRequest(
-      file_system_it->second, request_id, error);
+  return result;
 }
 
-bool Service::RequestUnmount(int file_system_id) {
+ProvidedFileSystemInterface* Service::GetProvidedFileSystem(
+    const std::string& extension_id,
+    int file_system_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  FileSystemMap::iterator file_system_it = file_systems_.find(file_system_id);
-  if (file_system_it == file_systems_.end())
-    return false;
-
-  int request_id =
-      request_manager_.CreateRequest(file_system_it->second,
-                                     SuccessCallback(),
-                                     base::Bind(&Service::OnRequestUnmountError,
-                                                weak_ptr_factory_.GetWeakPtr(),
-                                                file_system_it->second));
-
-  if (!request_id)
-    return false;
-
-  scoped_ptr<base::ListValue> values(
-      CreateRequestValues(file_system_id, request_id));
-
-  extensions::EventRouter* event_router =
-      extensions::ExtensionSystem::Get(profile_)->event_router();
-  DCHECK(event_router);
-
-  event_router->DispatchEventToExtension(
-      file_system_it->second.extension_id(),
-      make_scoped_ptr(new extensions::Event(
-          extensions::api::file_system_provider::OnUnmountRequested::kEventName,
-          values.Pass())));
+  ProvidedFileSystemMap::iterator file_system_it =
+      file_system_map_.find(file_system_id);
+  if (file_system_it == file_system_map_.end() ||
+      file_system_it->second->GetFileSystemInfo().extension_id() !=
+          extension_id) {
+    return NULL;
+  }
 
-  return true;
+  return file_system_it->second;
 }
 
 void Service::Shutdown() { RemoveObserver(&request_manager_); }
 
-void Service::OnRequestUnmountError(const ProvidedFileSystem& file_system,
-                                    base::File::Error error) {
-  FOR_EACH_OBSERVER(
-      Observer, observers_, OnProvidedFileSystemUnmount(file_system, error));
+void Service::OnRequestUnmountStatus(
+    const ProvidedFileSystemInfo& file_system_info,
+    base::File::Error error) {
+  if (error == base::File::FILE_OK) {
+    // Do not notify observers, since the providing app is supposted to call
+    // unmount(), and then the event will be emitted.
+    return;
+  }
+
+  FOR_EACH_OBSERVER(Observer,

[hashimoto, 2014/03/26 03:03:01]

nit: Here what you want is to notify observers about errors which occur before
the request is sent to the provider extension, right?
Then, I think it would be easier to understand by reversing the logic like:
if (error != base::File::FILE_OK) {
  FOR_EACH_OBSERVER(...)
}
WDYT?

[mtomasz, 2014/03/26 05:45:13]

Almost. I want to notify observers about failure in unmounting after provider
extension calls onError, or if ProvidedFileSystem failed to create a request. I
agree with the cleanup. Done.

+                    observers_,
+                    OnProvidedFileSystemUnmount(file_system_info, error));
 }
 
 }  // namespace file_system_provider