Stop returning the true root path of each filesystem from openFileSystem....

webkit/fileapi/file_system_util.cc

Index: webkit/fileapi/file_system_util.cc
===================================================================
--- webkit/fileapi/file_system_util.cc	(revision 77587)
+++ webkit/fileapi/file_system_util.cc	(working copy)
@@ -7,6 +7,7 @@
 #include "build/build_config.h"
 
 #include "base/file_path.h"
+#include "base/logging.h"
 #include "base/sys_string_conversions.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/escape.h"
@@ -19,27 +20,48 @@
 
 bool CrackFileSystemURL(const GURL& url, GURL* origin_url, FileSystemType* type,
                         FilePath* file_path) {
-  *origin_url = GURL();
-  *type = kFileSystemTypeUnknown;
-  *file_path = FilePath();
+  GURL origin;
+  FileSystemType file_system_type;
 
   if (url.scheme() != "filesystem")
     return false;
 
-  GURL bare_url(url.path());
-  *origin_url = bare_url.GetOrigin();
+  std::string temp = url.path();

[kinuko, 2011/03/14 11:03:57]

Can't we create a new FilePath (e.g. by FilePath().AppendASCII(url.path()) and
call NormalizeWindowsPathSeparators() and ReferencesParent()?

[ericu, 2011/03/15 02:43:11]

url.path() is something like "file:///temporary/\testdir; it's not yet clean
enough to use FilePath methods, which can't handle mixed separators.

[kinuko, 2011/03/16 19:33:38]

I see, it's a bit sad though.  How about using FilePath for the parent reference
check then?  I just want to get rid of duplicated code, even it may be not super
efficient (I believe it is not significant).

[ericu, 2011/03/16 23:47:31]

Using FilePath wouldn't shrink the code much.  First we'd have to cut off the
part that the FilePath could handle, since "file:///temporary/\testdir" isn't
really valid for it.  This whole body of code is going away soon, so I'd really
prefer not to spend too much time optimizing it.  I certainly agree that it's
sad, though.

+  // On Windows, this will have backslashes for now.  TODO(ericu) remove this
+  // code when that ceases to be true, which should be soon.
+  size_t pos = temp.find('\\');
+  for (; pos != std::string::npos; pos = temp.find('\\', pos + 1)) {
+    temp[pos] = '/';
+  }
+  // TODO(ericu): This should probably be done elsewhere after the stackable
+  // layers are properly in.  We're supposed to reject any paths that contain
+  // '..' segments, but the GURL constructor is helpfully resolving them for us.
+  // Make sure there aren't any before we call it.
+  pos = temp.find("..");
+  for (; pos != std::string::npos; pos = temp.find("..", pos + 1)) {
+    if ((pos == 0 || temp[pos - 1] == '/') &&
+        (pos == temp.length() - 2 || temp[pos + 2] == '/'))
+      return false;
+  }
 
+  GURL bare_url(temp);

[kinuko, 2011/03/14 11:03:57]

It might be great to have a brief comment to explain how url and bare_url
differ. (Simple example urls for each would be enough)

[ericu, 2011/03/15 02:43:11]

Good point.  Done.

   // The input URL was malformed, bail out early.
-  if (origin_url->is_empty() || bare_url.path().empty())
+  if (bare_url.path().empty())
     return false;
 
+  origin = bare_url.GetOrigin();
+
+  // The input URL was malformed, bail out early.
+  if (origin.is_empty())
+    return false;
+
   std::string path = UnescapeURLComponent(bare_url.path(),
       UnescapeRule::SPACES | UnescapeRule::URL_SPECIAL_CHARS);
   if (path.compare(0, strlen(kPersistentDir), kPersistentDir) == 0) {
-    *type = kFileSystemTypePersistent;
+    file_system_type = kFileSystemTypePersistent;
     path = path.substr(strlen(kPersistentDir));
   } else if (path.compare(0, strlen(kTemporaryDir), kTemporaryDir) == 0) {
-    *type = kFileSystemTypeTemporary;
+    file_system_type = kFileSystemTypeTemporary;
     path = path.substr(strlen(kTemporaryDir));
   } else {
     return false;
@@ -50,13 +72,36 @@
     path.erase(0, 1);
 
 #if defined(OS_WIN)
-  const FilePath::StringType& sys_path = base::SysUTF8ToWide(path);
+  const FilePath::StringType sys_path = base::SysUTF8ToWide(path);
 #elif defined(OS_POSIX)
-  const FilePath::StringType& sys_path = path;
+  const FilePath::StringType sys_path = path;
 #endif
+  if (origin_url)
+    *origin_url = origin;
+  if (type)
+    *type = file_system_type;
+  if (file_path)
+    *file_path = FilePath(sys_path);
 
-  *file_path = FilePath(sys_path);
   return true;
 }
 
+GURL GetFileSystemRootURI(
+    const GURL& origin_url, fileapi::FileSystemType type) {
+  std::string path("filesystem:");
+  path += origin_url.spec();
+  switch (type) {
+  case kFileSystemTypeTemporary:
+    path += (kTemporaryDir + 1);  // We don't want the leading slash.
+    break;
+  case kFileSystemTypePersistent:
+    path += (kPersistentDir + 1);  // We don't want the leading slash.
+    break;
+  default:
+    NOTREACHED();
+    return GURL();
+  }
+  return GURL(path);
+}
+
 }  // namespace fileapi