Don't keep file system directories around if they contain unrelated files.

webkit/fileapi/obfuscated_file_util.cc

Index: webkit/fileapi/obfuscated_file_util.cc
diff --git a/webkit/fileapi/obfuscated_file_util.cc b/webkit/fileapi/obfuscated_file_util.cc
index 4b3ac347b4602fed578feecbf4013cf9111db030..da8afab26fbb09c88ca72b5bae5399269c9e3bb2 100644
--- a/webkit/fileapi/obfuscated_file_util.cc
+++ b/webkit/fileapi/obfuscated_file_util.cc
@@ -908,45 +908,66 @@ FilePath ObfuscatedFileUtil::GetDirectoryForOriginAndType(
     return FilePath();
   }
   FilePath path = origin_dir.Append(type_string);
+  base::PlatformFileError error = base::PLATFORM_FILE_OK;
   if (!file_util::DirectoryExists(path) &&
       (!create || !file_util::CreateDirectory(path))) {
-    if (error_code) {
-      *error_code = create ?
+    error = create ?
           base::PLATFORM_FILE_ERROR_FAILED :
           base::PLATFORM_FILE_ERROR_NOT_FOUND;
-    }
-    return FilePath();
   }
 
   if (error_code)
-    *error_code = base::PLATFORM_FILE_OK;
+    *error_code = error;

[kinuko, 2012/07/09 11:47:50]

I'm afraid some of the callsites of this method rely on the fact that the
returned path is empty if the error code is not OK.

[Bernhard Bauer, 2012/07/09 15:20:28]

Good point. Would you prefer updating all callsites to look at the error code
instead, or adding a convenience method that returns an empty path if the error
code is not OK?

   return path;
 }
 
 bool ObfuscatedFileUtil::DeleteDirectoryForOriginAndType(
     const GURL& origin, FileSystemType type) {
-  FilePath origin_type_path = GetDirectoryForOriginAndType(origin, type, false);
-  if (!file_util::PathExists(origin_type_path))
+  base::PlatformFileError error = base::PLATFORM_FILE_OK;
+  FilePath origin_type_path = GetDirectoryForOriginAndType(origin, type, false,
+                                                           &error);
+  if (origin_type_path.empty())
     return true;
 
-  // TODO(dmikurube): Consider the return value of DestroyDirectoryDatabase.
-  // We ignore its error now since 1) it doesn't matter the final result, and
-  // 2) it always returns false in Windows because of LevelDB's implementation.
-  // Information about failure would be useful for debugging.
-  DestroyDirectoryDatabase(origin, type);
-  if (!file_util::Delete(origin_type_path, true /* recursive */))
-    return false;
+  if (error != base::PLATFORM_FILE_ERROR_NOT_FOUND) {
+    // TODO(dmikurube): Consider the return value of DestroyDirectoryDatabase.
+    // We ignore its error now since 1) it doesn't matter the final result, and
+    // 2) it always returns false in Windows because of LevelDB's
+    // implementation.
+    // Information about failure would be useful for debugging.
+    DestroyDirectoryDatabase(origin, type);
+    if (!file_util::Delete(origin_type_path, true /* recursive */))
+      return false;
+  }
 
   FilePath origin_path = origin_type_path.DirName();
   DCHECK_EQ(origin_path.value(),
             GetDirectoryForOrigin(origin, false, NULL).value());
 
   // Delete the origin directory if the deleted one was the last remaining
-  // type for the origin.
-  if (file_util::Delete(origin_path, false /* recursive */)) {
+  // type for the origin, i.e. if the *other* type doesn't exist.
+  FileSystemType other_type = kFileSystemTypeUnknown;
+  switch (type) {
+    case kFileSystemTypeTemporary:
+      other_type = kFileSystemTypePersistent;
+      break;
+    case kFileSystemTypePersistent:
+      other_type = kFileSystemTypeTemporary;
+      break;
+    // These types shouldn't be used.
+    case kFileSystemTypeUnknown:
+    case kFileSystemTypeIsolated:
+    case kFileSystemTypeExternal:
+    case kFileSystemTypeTest:
+      NOTREACHED();
+  }
+  if (!file_util::DirectoryExists(
+          origin_path.Append(GetDirectoryNameForType(other_type)))) {

[kinuko, 2012/07/09 07:23:33]

Could we instead check if the type directory is the only directory in the origin
directory?

[Bernhard Bauer, 2012/07/09 09:03:29]

Hmm, I think there can also be garbage directories (for example, ZIP files of
directories on HFS volumes created by Mac OS contain a __MACOSX directory when
extracted on other file systems).

[kinuko, 2012/07/09 11:47:50]

Ok...

     InitOriginDatabase(false);
     if (origin_database_.get())
       origin_database_->RemovePathForOrigin(GetOriginIdentifierFromURL(origin));
+    if (!file_util::Delete(origin_path, true /* recursive */))
+      return false;
   }
 
   // At this point we are sure we had successfully deleted the origin/type