[fsp] Decouple file_service_provider::Service.

chrome/browser/chromeos/file_system_provider/service.h

Index: chrome/browser/chromeos/file_system_provider/service.h
diff --git a/chrome/browser/chromeos/file_system_provider/service.h b/chrome/browser/chromeos/file_system_provider/service.h
index 735e05fb9bf14f79a37f67a34639838da72c7a7e..d9c48214c89f603c1442511c7652ad5122055637 100644
--- a/chrome/browser/chromeos/file_system_provider/service.h
+++ b/chrome/browser/chromeos/file_system_provider/service.h
@@ -15,6 +15,7 @@
 #include "base/observer_list.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/file_system_provider/observer.h"
+#include "chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"

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

nit: Is this include needed?

[mtomasz, 2014/04/11 19:54:12]

Done.

 #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"
@@ -24,14 +25,27 @@
 namespace chromeos {
 namespace file_system_provider {
 
+class ProvidedFileSystemInfo;
+class ProvidedFileSystemInterface;
 class ServiceFactory;
 
-// Manages and registers the fileSystemProvider service.
+// Manages and registers the file system provider service. Maintains provided
+// file systems.
+//
+// Internally creates ProvidedFileSystem instances which require a browser
+// running. For unit tests, create with a custom factory (in most cases
+// FakeProvidedFileSystemFactory will do) via CreateForTesting().
 class Service : public KeyedService {
  public:
   explicit Service(Profile* profile);
   virtual ~Service();
 
+  // Creates a service instance for tests. Takes ownership on the
+  // |file_system_factory| instance.
+  static Service* CreateForTesting(
+      Profile* profile,
+      ProvidedFileSystemFactoryInterface* file_system_factory);
+
   // Mounts a file system provided by an extension with the |extension_id|.
   // For success, it returns a numeric file system id, which is an
   // auto-incremented non-zero value. For failures, it returns zero.
@@ -42,30 +56,21 @@ class Service : public KeyedService {
   // |extension_id|. For success returns true, otherwise false.
   bool UnmountFileSystem(const std::string& extension_id, int file_system_id);
 
-  // Returns a list of currently mounted file systems. All items are copied.
-  std::vector<ProvidedFileSystem> GetMountedFileSystems();
-
-  // 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(const std::string& extension_id,
-                      int file_system_id,
-                      int request_id,
-                      scoped_ptr<base::DictionaryValue> result,
-                      bool has_next);
-
-  // Handles error response for the |request_id|. If handling the error fails,
-  // returns false. Always disposes the request.
-  bool RejectRequest(const std::string& extension_id,
-                     int file_system_id,
-                     int request_id,
-                     base::File::Error error);
-
-  // Requests unmounting of a file system with the passed |file_system_id|.
-  // Returns true is unmounting has been requested. False, if the request is
-  // invalid (eg. already unmounted).
+  // 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.
   bool RequestUnmount(int file_system_id);
 
+  // Returns a list of information of all currently provided file systems. All
+  // items are copied.
+  std::vector<ProvidedFileSystemInfo> GetProvidedFileSystemInfoList();
+
+  // Returns a provided file system with |file_system_id|, handled by
+  // the extension with |extension_id|. If not found, then returns NULL.
+  ProvidedFileSystemInterface* GetProvidedFileSystem(
+      const std::string& extension_id,
+      int file_system_id);
+
   // Adds and removes observers.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -76,18 +81,28 @@ class Service : public KeyedService {
   // BrowserContextKeyedService overrides.
   virtual void Shutdown() OVERRIDE;
 
+  // Getter for the request manager. Used by the extension API to forward
+  // replies. Valid as long as the service.
+  RequestManager* request_manager() { return &request_manager_; }
+
  private:
-  typedef std::map<int, ProvidedFileSystem> FileSystemMap;
+  typedef std::map<int, ProvidedFileSystemInterface*> ProvidedFileSystemMap;
+
+  // Creates a service with a custom factory. Takes ownership on the
+  // |file_system_factory| instance.
+  Service(Profile* profile,

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

Why don't you simply replace the 1-argument version ctor with this?

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

We would have to pass the factory in many places, eg. in the VolumeManager, and
in any browser test (not unit test). I think that since the factory is default,
it is nice, if it is created inside Service, not outside. WDYT?

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

Do you think this merit is worth for the cost required to maintain two versions
of constructors?

Also, while this trick makes writing browser_tests handy (I guess we can ignore
production code because it's only one place), it makes unit_tests verbose.

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

I think that having multiple constructors is useful if it makes it easier to
instantiate the class later. Why not taking an advantage of that?

If we remove this second constructor, then unit tests *will not* be less
verbose, because if I understand correctly, we are talking about removing the
Service(Profile) constructor not Service(Profile, FactoryClass/FactoryClosure).
Do I understand correctly?

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

I don't think we should pay the cost for maintaining an additional version of
ctor used only by tests, just to save some key strokes when adding new tests.
If this ctor is public, you don't have to type "::CreateForTest".

+          ProvidedFileSystemFactoryInterface* file_system_factory);

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

You should use scoped_ptr if this method takes ownership of the pointer.

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

Good idea.

 
-  // Called when the providing extension calls the success callback for the
-  // onUnmountRequested event.
-  void OnRequestUnmountError(const ProvidedFileSystem& file_system,
-                             base::File::Error error);
+  // Called when the providing extension accepts or refuses a unmount request.
+  // If |error| is equal to FILE_OK, then the request is accepted.
+  void OnRequestUnmountStatus(const ProvidedFileSystemInfo& file_system_info,
+                              base::File::Error error);
 
   RequestManager request_manager_;
   Profile* profile_;
+  scoped_ptr<ProvidedFileSystemFactoryInterface> file_system_factory_;
   ObserverList<Observer> observers_;
-  FileSystemMap file_systems_;
+  ProvidedFileSystemMap file_system_map_;  // Owns pointers.
   int next_id_;
   base::WeakPtrFactory<Service> weak_ptr_factory_;