Fixed file/directory url resolution for external mount point provider.

webkit/chromeos/fileapi/cros_mount_point_provider.cc

Index: webkit/chromeos/fileapi/cros_mount_point_provider.cc
===================================================================
--- webkit/chromeos/fileapi/cros_mount_point_provider.cc	(revision 81876)
+++ webkit/chromeos/fileapi/cros_mount_point_provider.cc	(working copy)
@@ -10,6 +10,7 @@
 #include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
 #include "base/stringprintf.h"
+#include "base/synchronization/lock.h"
 #include "base/utf_string_conversions.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebCString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFileSystem.h"
@@ -30,7 +31,6 @@
 FixedExposedPaths fixed_exposed_paths[] = {
     {"/home/chronos/user/", "Downloads"},
     {"/",                   "media"},
-    {"/",                   "tmp"},
 };
 
 CrosMountPointProvider::CrosMountPointProvider(
@@ -55,18 +55,48 @@
   return web_security_origin.databaseIdentifier().utf8();
 }
 
+bool CrosMountPointProvider::GetRootForVirtualPath(
+    const FilePath& virtual_path, FilePath* root_path) {
+  // On the other hand, the root path of a file/dir from the real file system

[ericu, 2011/04/18 20:35:56]

I don't understand this comment.
Also, the start "on the other hand" implies there's some background I'm missing.

[zel, 2011/04/18 20:51:12]

sorry, this got removed from the context of other comments. Done

+  // needs to be mapped based on its first virtual directory segment that
+  // comes just after as in /external/<first-dir-segment>/file.foo
+  std::vector<FilePath::StringType> components;
+  virtual_path.GetComponents(&components);
+  if (components.size() < 1) {
+    return false;
+  }
+
+  base::AutoLock locker(lock_);
+  // Check if this root directory is exposed by this provider.
+  MountPointMap::iterator iter = mount_point_map_.find(components[0]);
+  if (iter == mount_point_map_.end()) {
+    return false;
+  }
+  *root_path = iter->second;
+  return true;
+}
+
 void CrosMountPointProvider::GetFileSystemRootPath(
     const GURL& origin_url,
     fileapi::FileSystemType type,
     bool create,
+    bool is_file_system,

[ericu, 2011/04/18 20:35:56]

is_file_system seems poorly named.  If this flag is intended to mean "this path
isn't actually in the filesystem at all", then it seems like you shouldn't
really be calling this method at all.  Is the whole point of this change just to
add a hack so that you can test some other class?  Is there another way to do
that?

[zel, 2011/04/18 20:51:12]

I agree about the name, any suggestion is welcomed.

It's actually not a test hack, it's little bit more complicated than that. The
external mount point provider exposes first segment of virtual path that does
not have a corresponding directory entry in the real file system - /external/.
When we want to get root path to construct DOMFileSystem, then /external/ is
acceptable, but when I need to map a file/dir URL to the real file path then I
need to strip /external/ out of the virtual path. This flag differentiates
between these two cases.

+    const FilePath& virtual_path,
     fileapi::FileSystemPathManager::GetRootPathCallback* callback_ptr) {
   DCHECK(type == fileapi::kFileSystemTypeExternal);
-
   std::string name(GetOriginIdentifierFromURL(origin_url));
   name += ':';
   name += fileapi::kExternalName;
-
-  FilePath root_path = FilePath(fileapi::kExternalDir);
+  // File system root path is a non-existent path from the file system...

[ericu, 2011/04/18 20:35:56]

Why are we looking up a nonexistent path?

[zel, 2011/04/18 20:51:12]

see above

+  FilePath root_path;
+  if (is_file_system) {
+    root_path = FilePath(fileapi::kExternalDir);

[ericu, 2011/04/18 20:35:56]

This is kind of weird.  '/external/' is in the namespace of FileSystem URLs, and
now you're using it in a FilePath that's intended to be in the namespace of real
paths.  Why do this?

[zel, 2011/04/18 20:51:12]

see my first comment on this one as well

+  } else {
+    if (!GetRootForVirtualPath(virtual_path, &root_path)) {
+      callback_ptr->Run(false, FilePath(), std::string());
+      return;
+    }
+  }
   callback_ptr->Run(!root_path.empty(), root_path, name);
 }
 
@@ -78,20 +108,11 @@
     const FilePath& virtual_path,
     bool create) {
   DCHECK(type == fileapi::kFileSystemTypeExternal);
-
-  std::vector<FilePath::StringType> components;
-  virtual_path.GetComponents(&components);
-  if (components.size() < 1) {
+  FilePath root_path;
+  if (!GetRootForVirtualPath(virtual_path, &root_path))
     return FilePath();
-  }
 
-  // Check if this root directory is exposed by this provider.
-  MountPointMap::iterator iter = mount_point_map_.find(components[0]);
-  if (iter == mount_point_map_.end()) {
-    return FilePath();
-  }
-
-  return iter->second;
+  return root_path;
 }
 
 // TODO(zelidrag): Share this code with SandboxMountPointProvider impl.
@@ -112,6 +133,17 @@
                                                        virtual_path);
 }
 
+void CrosMountPointProvider::AddMountPoint(FilePath mount_point) {
+  base::AutoLock locker(lock_);
+  mount_point_map_.insert(std::pair<std::string, FilePath>(
+      mount_point.BaseName().value(), mount_point.DirName()));
+}
+
+void CrosMountPointProvider::RemoveMountPoint(FilePath mount_point) {
+  base::AutoLock locker(lock_);
+  mount_point_map_.erase(mount_point.BaseName().value());
+}
+
 void CrosMountPointProvider::GrantFullAccessToExtension(
     const std::string& extension_id) {
   DCHECK(special_storage_policy_->IsFileHandler(extension_id));