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 82028)
+++ 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"
@@ -26,11 +27,12 @@
   const char* web_root_path;
 } FixedExposedPaths;
 
+const char kChromeUIScheme[] = "chrome";
+
 // Top level file system elements exposed in FileAPI in ChromeOS:
 FixedExposedPaths fixed_exposed_paths[] = {
     {"/home/chronos/user/", "Downloads"},
     {"/",                   "media"},
-    {"/",                   "tmp"},
 };
 
 CrosMountPointProvider::CrosMountPointProvider(
@@ -55,18 +57,35 @@
   return web_security_origin.databaseIdentifier().utf8();
 }
 
+bool CrosMountPointProvider::GetRootForVirtualPath(
+    const FilePath& virtual_path, FilePath* root_path) {
+  std::vector<FilePath::StringType> components;
+  virtual_path.GetComponents(&components);
+  if (components.size() < 1) {
+    return false;
+  }
+
+  base::AutoLock locker(lock_);
+  // Check if this root mount point 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(

[ericu, 2011/04/19 20:30:49]

Add a comment explaining that this is only called by openFileSystem, and that it
doesn't actually get a root path, but a filesystem root URL instead.

Actually, this is a bit of a mess, and it's my fault.  GetFileSystemRootPath
doesn't do the same thing as GetFileSystemRootPathOnFileThread, and that's just
wrong.  How about we change the name of this method to
ValidateFileSystemRootAndGetURL everywhere?  That'll clear up the confusing
names, and we can leave the stuff that actually gets the root path named as it
is.  Does that sound reasonable?

We could also change the stuff that gets the root path to
ValidateFileSystemRootAndGetPathOnFileThread, as the sandbox provider actually
does more than the extension provider, but I'm not sure it's necessary.

[zel, 2011/04/19 23:14:00]

Done.

     const GURL& origin_url,
     fileapi::FileSystemType type,
     bool create,
     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);
+  FilePath root_path;
+  root_path = FilePath(fileapi::kExternalDir);
   callback_ptr->Run(!root_path.empty(), root_path, name);

[ericu, 2011/04/19 20:30:49]

This code looks odd.  Why check if it's empty?
Also, you're not returning a full filesystem URL, which you should be.

[zel, 2011/04/19 23:14:00]

What do you mean URL? This one is accepting FilePath here:

typedef Callback3<bool /* success */,
                    const FilePath& /* root_path */,
                    const std::string& /* name */>::Type GetRootPathCallback;

[ericu, 2011/04/19 23:36:19]

You're right--I got myself confused.  Nevermind.

But perhaps we should return an empty path, instead of /External, since it
should never be used anyway, and /External looks like it could mean something?

[zel, 2011/04/20 02:02:07]

I think there are tests on my side that expect /external/ here. I will remove
this is really nothing depends on it.

 }
 
@@ -78,20 +97,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.
@@ -104,14 +114,31 @@
                                              const FilePath& virtual_path) {
   if (type != fileapi::kFileSystemTypeExternal)
     return false;
+
+  // Permit access to mount points from internal WebUI.
+  if (origin_url.SchemeIs(kChromeUIScheme))
+    return true;
+
   std::string extension_id = origin_url.host();
   // Check first to make sure this extension has fileBrowserHander permissions.
   if (!special_storage_policy_->IsFileHandler(extension_id))
     return false;
+
   return file_access_permissions_->HasAccessPermission(extension_id,
                                                        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));