FileSystem: Modify ObfucatedFileUtil to delete contents of the plugin private filesystem

storage/browser/fileapi/obfuscated_file_util.cc

Index: storage/browser/fileapi/obfuscated_file_util.cc
diff --git a/storage/browser/fileapi/obfuscated_file_util.cc b/storage/browser/fileapi/obfuscated_file_util.cc
index 8c02dd11bf4a18e2fbcbd7ab2bdc0edbd6b6a1ca..b2abc373f1bc6f78187ea071afe28098c3cdbbd5 100644
--- a/storage/browser/fileapi/obfuscated_file_util.cc
+++ b/storage/browser/fileapi/obfuscated_file_util.cc
@@ -15,6 +15,7 @@
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
@@ -906,6 +907,39 @@ bool ObfuscatedFileUtil::DeleteDirectoryForOriginAndType(
   return true;
 }
 
+bool ObfuscatedFileUtil::DeleteOriginDirectoryForPluginPrivateFileSystem(

[kinuko, 2014/09/24 15:51:37]

So far we hadn't introduced the term 'plugin' in this module
(ObfuscatedFileUtil), do we want to make this class 'PluginPrivate' aware?  To
make this module free from 'plugin' terminology, and also to make it clear that
this method works similar to DeleteDirectoryForOriginAndType(), it might make
better sense to rename this to DeleteDirectoryForOrigin.

[nhiroki, 2014/09/25 10:34:16]

You're right. We don't have to add a specialized function for the plugin private
filesystem. I made DeleteDirectoryForOriginAndType() available for the empty
type_string case and let PluginPrivateFSBackend call it.

(At first I thought a separated function makes things simpler :)

+    const GURL& origin) {
+  // Delete the plugin private filesystems from the directory database.
+  const std::string key_prefix = storage::GetIdentifierFromOrigin(origin);
+  for (DirectoryMap::iterator iter = directories_.lower_bound(key_prefix);
+       iter != directories_.end();) {
+    if (!StartsWithASCII(iter->first, key_prefix, true))
+      break;
+    SandboxDirectoryDatabase* database = iter->second;
+    directories_.erase(iter++);

[tzik, 2014/09/24 18:48:12]

As we chatted locally. (Sorry, I forget to leave that as a written text.)
This deletes http://example.com:1234 for http://example.com/12 deletion.
Please add extra check to ensure iter->first is the database to delete.
And for that, adding a separator between origin and type_string will be easiest
way.

[nhiroki, 2014/09/25 10:34:16]

"http://example.com/12" -> "http://example.com:12" (just in case)
Thank you for pointing it out! I added a whitespace (' ') as a key separator
which is never used for origins and readable when debugging. Wdyt?

+    delete database;
+  }
+
+  base::File::Error error = base::File::FILE_OK;
+
+  // |origin_path| points to "/path/to/File System/Plugins/<origin>".
+  base::FilePath origin_path = GetDirectoryForOrigin(origin, false, &error);
+  if (!origin_path.empty()) {
+    if (!SandboxDirectoryDatabase::DestroyDatabase(origin_path, env_override_))
+      return false;
+    if (!base::DeleteFile(origin_path, true /* recursive */))
+      return false;
+  }

[kinuko, 2014/09/24 15:51:37]

I wonder if supporting type_string.empty() case in DestroyDirectoryDatabase()
(by moving 
moving line 913-921 into DestroyDirectoryDatabase()) and just calling it instead
of line 913-932 might make the code a bit simpler, less dup?  (Either we add
this new method or not)  Wdyt?

[nhiroki, 2014/09/25 10:34:16]

(Removed entirely this function)

+
+  // Delete the origin directory from the origin database.
+  InitOriginDatabase(origin, false);
+  if (origin_database_) {
+    origin_database_->RemovePathForOrigin(
+        storage::GetIdentifierFromOrigin(origin));
+  }
+  return true;
+}
+
 ObfuscatedFileUtil::AbstractOriginEnumerator*
 ObfuscatedFileUtil::CreateOriginEnumerator() {
   std::vector<SandboxOriginDatabase::OriginRecord> origins;