FileAPI code should not rely on or assume specific MountPointProvider types

To move some non-regular MountPointProviders out of webkit/fileapi,
we should not:
- Require FileAPI code change for a new MountPointProvider to
  get/set provider-specific operation context value (e.g.  MediaPathFilter)
- Require a new MountPointProvider type to be listed as a 'frined' of
  FileAPI code

To avoid having these dependencies, this change:
- Makes FileSystemOperationContext inherit from SupportsUserData so that
  MountPointProviders can get/set arbitrary user values with string keys.
- Makes some private methods public (with proper comments) and remove
  the friend list of MountPointProviders

This patch is a part of following change plan (internal only doc):
https://docs.google.com/a/google.com/document/d/1XEtX0OO_RIA_c0KuKvd9yqK5l3XtsTulwXs3c-mAQ1o/view

BUG=175936
TEST=content_unittests:*File*
TEST=browser_tests:*MediaGalleries*

Review URL: https://codereview.chromium.org/14341004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@195193 0039d316-1c4b-4281-b951-d872f2087c98

webkit/fileapi/file_system_operation_context.cc

@@ -16,11 +16,13 @@ FileSystemOperationContext::FileSystemOperationContext(
       task_runner_(file_system_context_->task_runners()->file_task_runner()),
       allowed_bytes_growth_(0) {}
 
-FileSystemOperationContext::~FileSystemOperationContext() {}
+FileSystemOperationContext::FileSystemOperationContext(
+    FileSystemContext* context,
+    base::SequencedTaskRunner* task_runner)
+    : file_system_context_(context),
+      task_runner_(task_runner),
+      allowed_bytes_growth_(0) {}
 
-void FileSystemOperationContext::set_task_runner(
-    base::SequencedTaskRunner* task_runner) {
-  task_runner_ = task_runner;
-}
+FileSystemOperationContext::~FileSystemOperationContext() {}
 
 }  // namespace fileapi

webkit/fileapi/file_system_operation_context.h

@@ -5,7 +5,7 @@
 #ifndef WEBKIT_FILEAPI_FILE_SYSTEM_OPERATION_CONTEXT_H_
 #define WEBKIT_FILEAPI_FILE_SYSTEM_OPERATION_CONTEXT_H_
 
-#include "webkit/fileapi/media/mtp_device_file_system_config.h"
+#include "base/supports_user_data.h"
 #include "webkit/fileapi/task_runner_bound_observer_list.h"
 #include "webkit/storage/webkit_storage_export.h"
 
@@ -16,7 +16,6 @@ class SequencedTaskRunner;
 namespace fileapi {
 
 class FileSystemContext;
-class MediaPathFilter;
 
 // A context class which is carried around by FileSystemOperation and
 // its delegated tasks. It is valid to reuse one context instance across
@@ -24,10 +23,16 @@ class MediaPathFilter;
 // the same context (e.g. use the same task runner, share the quota etc).
 // Note that the remaining quota bytes (allowed_bytes_growth) may be
 // updated during the execution of write operations.
-class WEBKIT_STORAGE_EXPORT_PRIVATE FileSystemOperationContext {
+class WEBKIT_STORAGE_EXPORT_PRIVATE FileSystemOperationContext
+    : public base::SupportsUserData {
  public:
   explicit FileSystemOperationContext(FileSystemContext* context);
-  ~FileSystemOperationContext();
+
+  // Specifies |task_runner| which the operation is performed on.
+  FileSystemOperationContext(FileSystemContext* context,
+                             base::SequencedTaskRunner* task_runner);
+
+  virtual ~FileSystemOperationContext();
 
   FileSystemContext* file_system_context() const {
     return file_system_context_;
@@ -41,26 +46,37 @@ class WEBKIT_STORAGE_EXPORT_PRIVATE FileSystemOperationContext {
   // Returns the current remaining quota.
   int64 allowed_bytes_growth() const { return allowed_bytes_growth_; }
 
-#if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-  // Reads |mtp_device_delegate_url_| on |task_runner_|.
-  const std::string& mtp_device_delegate_url() const {
-    return mtp_device_delegate_url_;
-  }
-#endif
-
   // Returns TaskRunner which the operation is performed on.
   base::SequencedTaskRunner* task_runner() const {
     return task_runner_.get();
   }
 
-  MediaPathFilter* media_path_filter() { return media_path_filter_; }
   ChangeObserverList* change_observers() { return &change_observers_; }
   AccessObserverList* access_observers() { return &access_observers_; }
   UpdateObserverList* update_observers() { return &update_observers_; }
 
+  // Gets and sets value-type (or not-owned) variable as UserData.
+  template <typename T> T GetUserValue(const char* key) const {
+    ValueAdapter<T>* v = static_cast<ValueAdapter<T>*>(GetUserData(key));
+    return v ? v->value() : T();
+  }
+  template <typename T> void SetUserValue(const char* key, const T& value) {
+    SetUserData(key, new ValueAdapter<T>(value));
+  }
+
  private:
-  // Only MountPointProviders can access these setters.
-  friend class CrosMountPointProvider;
+  // An adapter for setting a value-type (or not owned) data as UserData.
+  template <typename T> class ValueAdapter
+      : public base::SupportsUserData::Data {
+   public:
+    ValueAdapter(const T& value) : value_(value) {}
+    const T& value() const { return value_; }
+   private:
+    T value_;
+    DISALLOW_COPY_AND_ASSIGN(ValueAdapter);
+  };
+
+  // Only regular (and test) MountPointProviders can access following setters.
   friend class IsolatedMountPointProvider;
   friend class SandboxMountPointProvider;
   friend class TestMountPointProvider;
@@ -72,13 +88,6 @@ class WEBKIT_STORAGE_EXPORT_PRIVATE FileSystemOperationContext {
   friend class LocalFileSystemTestOriginHelper;
   friend class ObfuscatedFileUtilTest;
 
-  // Overrides TaskRunner which the operation is performed on.
-  // file_system_context_->task_runners()->file_task_runner() is used otherwise.
-  void set_task_runner(base::SequencedTaskRunner* task_runner);
-
-  void set_media_path_filter(MediaPathFilter* media_path_filter) {
-    media_path_filter_ = media_path_filter;
-  }
   void set_change_observers(const ChangeObserverList& list) {
     change_observers_ = list;
   }
@@ -88,29 +97,16 @@ class WEBKIT_STORAGE_EXPORT_PRIVATE FileSystemOperationContext {
   void set_update_observers(const UpdateObserverList& list) {
     update_observers_ = list;
   }
-#if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-  // Initializes |mtp_device_delegate_url_| on the IO thread.
-  void set_mtp_device_delegate_url(const std::string& delegate_url) {
-    mtp_device_delegate_url_ = delegate_url;
-  }
-#endif
 
   FileSystemContext* file_system_context_;
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   int64 allowed_bytes_growth_;
-  MediaPathFilter* media_path_filter_;
 
   AccessObserverList access_observers_;
   ChangeObserverList change_observers_;
   UpdateObserverList update_observers_;
 
-#if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-  // URL for the media transfer protocol (MTP) device delegate.
-  // Initialized on IO thread and used on |task_runner_|.
-  std::string mtp_device_delegate_url_;
-#endif
-
   DISALLOW_COPY_AND_ASSIGN(FileSystemOperationContext);
 };
 

webkit/fileapi/isolated_mount_point_provider.cc

@@ -36,6 +36,11 @@
 
 namespace fileapi {
 
+const char IsolatedMountPointProvider::kMediaPathFilterKey[] =
+    "MediaPathFilterKey";
+const char IsolatedMountPointProvider::kMTPDeviceDelegateURLKey[] =
+    "MTPDeviceDelegateKey";
+
 IsolatedMountPointProvider::IsolatedMountPointProvider(
     const base::FilePath& profile_path)
     : profile_path_(profile_path),
@@ -181,18 +186,25 @@ FileSystemOperation* IsolatedMountPointProvider::CreateFileSystemOperation(
     const FileSystemURL& url,
     FileSystemContext* context,
     base::PlatformFileError* error_code) const {
-  scoped_ptr<FileSystemOperationContext> operation_context(
-      new FileSystemOperationContext(context));
-  if (url.type() == kFileSystemTypeNativeMedia ||
-      url.type() == kFileSystemTypeDeviceMedia) {
-    operation_context->set_media_path_filter(media_path_filter_.get());
-    operation_context->set_task_runner(
-        context->task_runners()->media_task_runner());
+  if (url.type() != kFileSystemTypeNativeMedia &&
+      url.type() != kFileSystemTypeDeviceMedia) {
+    return new LocalFileSystemOperation(
+        context, make_scoped_ptr(new FileSystemOperationContext(context)));
   }
 
+  // For media filesystems.
+  scoped_ptr<FileSystemOperationContext> operation_context(
+      new FileSystemOperationContext(
+          context, context->task_runners()->media_task_runner()));
+
+  operation_context->SetUserValue(kMediaPathFilterKey,
+                                  media_path_filter_.get());
+
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-  if (url.type() == kFileSystemTypeDeviceMedia)
-    operation_context->set_mtp_device_delegate_url(url.filesystem_id());
+  if (url.type() == kFileSystemTypeDeviceMedia) {
+    operation_context->SetUserValue(kMTPDeviceDelegateURLKey,
+                                    url.filesystem_id());
+  }
 #endif
 
   return new LocalFileSystemOperation(context, operation_context.Pass());

webkit/fileapi/isolated_mount_point_provider.h

@@ -21,6 +21,9 @@ class DeviceMediaAsyncFileUtil;
 
 class IsolatedMountPointProvider : public FileSystemMountPointProvider {
  public:
+  static const char kMediaPathFilterKey[];
+  static const char kMTPDeviceDelegateURLKey[];
+
   explicit IsolatedMountPointProvider(const base::FilePath& profile_path);
   virtual ~IsolatedMountPointProvider();
 

webkit/fileapi/local_file_system_operation.cc

@@ -32,7 +32,23 @@ using webkit_blob::ShareableFileReference;
 
 namespace fileapi {
 
+LocalFileSystemOperation::LocalFileSystemOperation(
+    FileSystemContext* file_system_context,
+    scoped_ptr<FileSystemOperationContext> operation_context)
+    : file_system_context_(file_system_context),
+      operation_context_(operation_context.Pass()),
+      async_file_util_(NULL),
+      peer_handle_(base::kNullProcessHandle),
+      pending_operation_(kOperationNone),
+      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+  DCHECK(operation_context_.get());
+  operation_context_->DetachUserDataThread();
+}
+
 LocalFileSystemOperation::~LocalFileSystemOperation() {
+  if (!operation_context())
+    return;
+  operation_context()->DetachUserDataThread();
   if (write_target_url_.is_valid()) {
     operation_context()->update_observers()->Notify(
         &FileUpdateObserver::OnEndUpdate, MakeTuple(write_target_url_));
@@ -430,7 +446,7 @@ LocalFileSystemOperation* LocalFileSystemOperation::CreateNestedOperation() {
   LocalFileSystemOperation* operation = new LocalFileSystemOperation(
       file_system_context(),
       make_scoped_ptr(new FileSystemOperationContext(file_system_context())));
-  operation->parent_operation_ = this;
+  operation->parent_operation_ = weak_factory_.GetWeakPtr();
   return operation;
 }
 
@@ -535,19 +551,6 @@ void LocalFileSystemOperation::MoveFileLocal(
       base::Bind(callback, base::PLATFORM_FILE_ERROR_FAILED));
 }
 
-LocalFileSystemOperation::LocalFileSystemOperation(
-    FileSystemContext* file_system_context,
-    scoped_ptr<FileSystemOperationContext> operation_context)
-    : file_system_context_(file_system_context),
-      operation_context_(operation_context.Pass()),
-      async_file_util_(NULL),
-      parent_operation_(NULL),
-      peer_handle_(base::kNullProcessHandle),
-      pending_operation_(kOperationNone),
-      weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
-  DCHECK(operation_context_.get());
-}
-
 void LocalFileSystemOperation::GetUsageAndQuotaThenRunTask(
     const FileSystemURL& url,
     const base::Closure& task,

webkit/fileapi/local_file_system_operation.h

@@ -34,6 +34,14 @@ class RecursiveOperationDelegate;
 class WEBKIT_STORAGE_EXPORT LocalFileSystemOperation
     : public NON_EXPORTED_BASE(FileSystemOperation) {
  public:
+  // NOTE: This constructor should not be called outside MountPointProviders;
+  // instead please consider using
+  // file_system_context->CreateFileSystemOperation() to instantiate
+  // an appropriate FileSystemOperation.
+  LocalFileSystemOperation(
+      FileSystemContext* file_system_context,
+      scoped_ptr<FileSystemOperationContext> operation_context);
+
   virtual ~LocalFileSystemOperation();
 
   // FileSystemOperation overrides.
@@ -158,34 +166,6 @@ class WEBKIT_STORAGE_EXPORT LocalFileSystemOperation
   void SyncGetPlatformPath(const FileSystemURL& url,
                            base::FilePath* platform_path);
 
- private:
-  enum OperationMode {
-    OPERATION_MODE_READ,
-    OPERATION_MODE_WRITE,
-  };
-
-  // Only MountPointProviders or testing class can create a
-  // new operation directly.
-  friend class FileSystemTestHelper;
-  friend class IsolatedMountPointProvider;
-  friend class SandboxMountPointProvider;
-  friend class TestMountPointProvider;
-  friend class chromeos::CrosMountPointProvider;
-
-  friend class LocalFileSystemOperationTest;
-  friend class LocalFileSystemOperationWriteTest;
-  friend class FileWriterDelegateTest;
-  friend class FileSystemQuotaTest;
-  friend class LocalFileSystemTestOriginHelper;
-
-  friend class RecursiveOperationDelegate;
-  friend class CrossOperationDelegate;
-  friend class sync_file_system::SyncableFileSystemOperation;
-
-  LocalFileSystemOperation(
-      FileSystemContext* file_system_context,
-      scoped_ptr<FileSystemOperationContext> operation_context);
-
   FileSystemContext* file_system_context() const {
     return file_system_context_;
   }
@@ -196,6 +176,14 @@ class WEBKIT_STORAGE_EXPORT LocalFileSystemOperation
     return operation_context_.get();
   }
 
+ private:
+  enum OperationMode {
+    OPERATION_MODE_READ,
+    OPERATION_MODE_WRITE,
+  };
+
+  friend class sync_file_system::SyncableFileSystemOperation;
+
   // Queries the quota and usage and then runs the given |task|.
   // If an error occurs during the quota query it runs |error_callback| instead.
   void GetUsageAndQuotaThenRunTask(
@@ -316,7 +304,7 @@ class WEBKIT_STORAGE_EXPORT LocalFileSystemOperation
   // this holds non-null value and points to the parent operation.
   // TODO(kinuko): Cleanup this when we finish cleaning up the
   // FileSystemOperation lifetime issue.
-  LocalFileSystemOperation* parent_operation_;
+  base::WeakPtr<LocalFileSystemOperation> parent_operation_;
 
   // These are all used only by Write().
   friend class FileWriterDelegate;

webkit/fileapi/media/device_media_async_file_util.cc

@@ -12,6 +12,7 @@
 #include "webkit/fileapi/file_system_task_runners.h"
 #include "webkit/fileapi/file_system_url.h"
 #include "webkit/fileapi/isolated_context.h"
+#include "webkit/fileapi/isolated_mount_point_provider.h"
 #include "webkit/fileapi/media/filtering_file_enumerator.h"
 #include "webkit/fileapi/media/media_path_filter.h"
 #include "webkit/fileapi/media/mtp_device_async_delegate.h"
@@ -35,7 +36,8 @@ MTPDeviceAsyncDelegate* GetMTPDeviceDelegate(
     FileSystemOperationContext* context) {
   DCHECK(IsOnIOThread(context));
   return MTPDeviceMapService::GetInstance()->GetMTPDeviceAsyncDelegate(
-      context->mtp_device_delegate_url());
+      context->GetUserValue<std::string>(
+          IsolatedMountPointProvider::kMTPDeviceDelegateURLKey));
 }
 
 // Called on a blocking pool thread to create a snapshot file to hold the
@@ -251,8 +253,7 @@ bool DeviceMediaAsyncFileUtil::CreateSnapshotFile(
     return true;
   }
   base::FilePath* snapshot_file_path = new base::FilePath;
-  return context->file_system_context()->task_runners()->media_task_runner()->
-      PostTaskAndReply(
+  return context->task_runner()->PostTaskAndReply(
           FROM_HERE,
           base::Bind(&CreateSnapshotFileOnBlockingPool,
                      url.path(),

webkit/fileapi/media/native_media_file_util.cc

@@ -5,8 +5,9 @@
 #include "webkit/fileapi/media/native_media_file_util.h"
 
 #include "webkit/fileapi/file_system_operation_context.h"
-#include "webkit/fileapi/media/media_path_filter.h"
+#include "webkit/fileapi/isolated_mount_point_provider.h"
 #include "webkit/fileapi/media/filtering_file_enumerator.h"
+#include "webkit/fileapi/media/media_path_filter.h"
 #include "webkit/fileapi/native_file_util.h"
 
 using base::PlatformFile;
@@ -15,6 +16,15 @@ using base::PlatformFileInfo;
 
 namespace fileapi {
 
+namespace {
+
+MediaPathFilter* GetMediaPathFilter(FileSystemOperationContext* context) {
+  return context->GetUserValue<MediaPathFilter*>(
+          IsolatedMountPointProvider::kMediaPathFilterKey);
+}
+
+}  // namespace
+
 NativeMediaFileUtil::NativeMediaFileUtil() {
 }
 
@@ -45,7 +55,7 @@ NativeMediaFileUtil::CreateFileEnumerator(
   DCHECK(context);
   return make_scoped_ptr(new FilteringFileEnumerator(
       IsolatedFileUtil::CreateFileEnumerator(context, root_url),
-      context->media_path_filter()))
+      GetMediaPathFilter(context)))
       .PassAs<FileSystemFileUtil::AbstractFileEnumerator>();
 }
 
@@ -109,7 +119,7 @@ PlatformFileError NativeMediaFileUtil::CopyOrMoveFile(
     return error;
   if (error == base::PLATFORM_FILE_OK && file_info.is_directory)
     return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
-  if (!context->media_path_filter()->Match(dest_file_path))
+  if (!GetMediaPathFilter(context)->Match(dest_file_path))
     return base::PLATFORM_FILE_ERROR_SECURITY;
 
   return NativeFileUtil::CopyOrMoveFile(src_file_path, dest_file_path, copy);
@@ -143,7 +153,7 @@ PlatformFileError NativeMediaFileUtil::DeleteFile(
     return error;
   if (file_info.is_directory)
     return base::PLATFORM_FILE_ERROR_NOT_A_FILE;
-  if (!context->media_path_filter()->Match(file_path))
+  if (!GetMediaPathFilter(context)->Match(file_path))
     return base::PLATFORM_FILE_ERROR_NOT_FOUND;
   return NativeFileUtil::DeleteFile(file_path);
 }
@@ -154,7 +164,7 @@ PlatformFileError NativeMediaFileUtil::GetFileInfo(
     PlatformFileInfo* file_info,
     base::FilePath* platform_path) {
   DCHECK(context);
-  DCHECK(context->media_path_filter());
+  DCHECK(GetMediaPathFilter(context));
   DCHECK(file_info);
   DCHECK(platform_path);
 
@@ -164,7 +174,7 @@ PlatformFileError NativeMediaFileUtil::GetFileInfo(
     return error;
 
   if (file_info->is_directory ||
-      context->media_path_filter()->Match(*platform_path)) {
+      GetMediaPathFilter(context)->Match(*platform_path)) {
     return base::PLATFORM_FILE_OK;
   }
   return base::PLATFORM_FILE_ERROR_NOT_FOUND;
@@ -179,7 +189,7 @@ PlatformFileError NativeMediaFileUtil::GetFilteredLocalFilePath(
       IsolatedFileUtil::GetLocalFilePath(context, file_system_url, &file_path);
   if (error != base::PLATFORM_FILE_OK)
     return error;
-  if (!context->media_path_filter()->Match(file_path))
+  if (!GetMediaPathFilter(context)->Match(file_path))
     return base::PLATFORM_FILE_ERROR_SECURITY;
 
   *local_file_path = file_path;
@@ -205,7 +215,7 @@ NativeMediaFileUtil::GetFilteredLocalFilePathForExistingFileOrDirectory(
     return base::PLATFORM_FILE_ERROR_FAILED;
 
   if (!file_info.is_directory &&
-      !context->media_path_filter()->Match(file_path)) {
+      !GetMediaPathFilter(context)->Match(file_path)) {
     return failure_error;
   }