Cleanup: Removing FileSystemPathManager

webkit/fileapi/file_system_context.h

Index: webkit/fileapi/file_system_context.h
diff --git a/webkit/fileapi/file_system_context.h b/webkit/fileapi/file_system_context.h
index a797cebcfb00495d2763f476b40488e02ab50c10..0864f7f0a2601dddb595e85fd4b9fd2d1f6da5a3 100644
--- a/webkit/fileapi/file_system_context.h
+++ b/webkit/fileapi/file_system_context.h
@@ -23,7 +23,11 @@ class QuotaManagerProxy;
 
 namespace fileapi {
 
+class ExternalFileSystemMountPointProvider;
 class FileSystemContext;
+class FileSystemFileUtil;
+class FileSystemMountPointProvider;
+class FileSystemOptions;
 class FileSystemPathManager;
 class FileSystemQuotaUtil;
 class SandboxMountPointProvider;
@@ -36,15 +40,14 @@ class FileSystemContext
     : public base::RefCountedThreadSafe<FileSystemContext,
                                         DefaultContextDeleter> {
  public:
+  // The ownership of |options| is transferred from the caller.

[ericu, 2011/12/20 18:12:59]

It's a very small class.  Perhaps we should just pass it by value?

[kinuko, 2011/12/21 13:00:50]

The point is I used virtual methods for allowing browser code to implement them.
I could also turn this class into a simple struct but I wanted to avoid using
raw boolean values.

[ericu, 2011/12/21 22:22:48]

I meant that the class, with its virtual methods, could be passed by value to
avoid the mental overhead of lifetime management of a dynamically-allocated
object.  But if you really prefer it this way, I don't feel very strongly about
it.

[kinuko, 2011/12/22 05:28:39]

Sorry I might be missing something but I cannot imagine what type we should
receive for the passed value.  (The BrowserFileSystemOption is defined in
content but FileSystemContext doesn't know the subclass(es) so that it cannot
tell how much storage it needs for the value.)

[kinuko, 2011/12/27 17:33:27]

Ok I stopped using virtual methods and converted them to use simple pass-by
values.

   FileSystemContext(
-      scoped_refptr<base::MessageLoopProxy> file_message_loop,
-      scoped_refptr<base::MessageLoopProxy> io_message_loop,
-      scoped_refptr<quota::SpecialStoragePolicy> special_storage_policy,
+      base::MessageLoopProxy* file_message_loop,
+      base::MessageLoopProxy* io_message_loop,
+      quota::SpecialStoragePolicy* special_storage_policy,

[ericu, 2011/12/20 18:12:59]

Why are these no longer scoped_refptrs?  Should they not have been all along?

[kinuko, 2011/12/21 13:00:50]

They are all cast'ed to scoped_refptr in the initializers so they should be just
fine.  (There's no strong reason to turn them into raw pointers for now though.
Subtle benefits from this change are the code looks simpler and we can more
easily pass NULL pointers)

[ericu, 2011/12/21 22:22:48]

Making the code look simpler than it is may cause confusion, and these values
won't often be NULL.  When they are, it's probably worth the explicit
scoped_refptr<base::MessageLoopProxy>() to make it clear.  I'd prefer not to
make this change.

[kinuko, 2011/12/22 05:28:39]

Another subtle benefit is we can save refcount churn at arg passing (though it's
not the only way to do so).  Recently I saw similar changes elsewhere so I
thought this change'd be preferable.  For example SpecialStoragePolicy is almost
always passed as a raw pointer)  I can revert this (personally I have no strong
opinion on this) but I feel changing this to pointer might make the code more
consistent with other parts.

[kinuko, 2011/12/27 17:33:27]

I reverted these changes for now.  We can change them later if we really want
to.

       quota::QuotaManagerProxy* quota_manager_proxy,
       const FilePath& profile_path,
-      bool is_incognito,
-      bool allow_file_access_from_files,
-      FileSystemPathManager* path_manager);
+      FileSystemOptions* options);
   ~FileSystemContext();
 
   // This method can be called on any thread.
@@ -52,7 +55,6 @@ class FileSystemContext
   bool DeleteDataForOriginAndTypeOnFileThread(const GURL& origin_url,
                                               FileSystemType type);
 
-  FileSystemPathManager* path_manager() const { return path_manager_.get(); }
   quota::QuotaManagerProxy* quota_manager_proxy() const {
     return quota_manager_proxy_.get();
   }
@@ -62,17 +64,39 @@ class FileSystemContext
   // it is not a quota-managed storage.
   FileSystemQuotaUtil* GetQuotaUtil(FileSystemType type) const;
 
+  // Returns the appropriate FileUtil instance for the given |type|.
+  // This may return NULL if it is given an invalid or unsupported filesystem
+  // type.
+  FileSystemFileUtil* GetFileUtil(FileSystemType type) const;
+
+  // Returns the mount point provider instance for the given |type|.
+  // This may return NULL if it is given an invalid or unsupported filesystem
+  // type.
+  FileSystemMountPointProvider* GetMountPointProvider(
+      FileSystemType type) const;
+
+  // Returns a FileSystemMountPointProvider instance for sandboxed filesystem
+  // types (e.g. TEMPORARY or PERSISTENT).  This is equivalent to calling
+  // GetMountPointProvider(kFileSystemType{Temporary, Persistent}).
+  SandboxMountPointProvider* sandbox_provider() const;
+
+  // Returns a FileSystemMountPointProvider instance for external filesystem
+  // type, which is used only by chromeos for now.  This is equivalent to
+  // calling GetMountPointProvider(kFileSystemTypeExternal).
+  ExternalFileSystemMountPointProvider* external_provider() const;
+
  private:
   friend struct DefaultContextDeleter;
   void DeleteOnCorrectThread() const;
-  SandboxMountPointProvider* sandbox_provider() const;
 
   scoped_refptr<base::MessageLoopProxy> file_message_loop_;
   scoped_refptr<base::MessageLoopProxy> io_message_loop_;
 
   scoped_refptr<quota::QuotaManagerProxy> quota_manager_proxy_;
 
-  scoped_ptr<FileSystemPathManager> path_manager_;
+  // Mount point providers.
+  scoped_ptr<SandboxMountPointProvider> sandbox_provider_;
+  scoped_ptr<ExternalFileSystemMountPointProvider> external_provider_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(FileSystemContext);
 };