Simplify directory path accounting.

webkit/fileapi/file_system_file_util.cc

Index: webkit/fileapi/file_system_file_util.cc
diff --git a/webkit/fileapi/file_system_file_util.cc b/webkit/fileapi/file_system_file_util.cc
index afb6453b55e4c87275f5e857f4d7200da9cff76d..25f0c0c9a6e8a4c7c09195fa73a701f8c984b145 100644
--- a/webkit/fileapi/file_system_file_util.cc
+++ b/webkit/fileapi/file_system_file_util.cc
@@ -136,13 +136,10 @@ PlatformFileError FileSystemFileUtil::ReadDirectory(
 }
 
 PlatformFileError FileSystemFileUtil::CreateDirectory(
-    FileSystemOperationContext* fs_context,
+    FileSystemOperationContext* unused,
     const FilePath& file_path,
     bool exclusive,
     bool recursive) {
-  if (fs_context->do_not_write_actually())
-    return base::PLATFORM_FILE_OK;
-
   // If parent dir of file doesn't exist.
   if (!recursive && !file_util::PathExists(file_path.DirName()))
     return base::PLATFORM_FILE_ERROR_NOT_FOUND;
@@ -253,21 +250,30 @@ FileSystemFileUtil::PerformCommonCheckAndPreparationForMoveAndCopy(
      (context->src_type() == context->dest_type());
   FileSystemFileUtil* dest_util = context->dest_file_system_file_util();
   DCHECK(dest_util);
-  if (same_file_system)
+  FileSystemOperationContext local_dest_context(
+      context->file_system_context(), dest_util);
+  FileSystemOperationContext* dest_context;
+  if (same_file_system) {
+    dest_context = context;
     DCHECK(context->src_file_system_file_util() ==
          context->dest_file_system_file_util());
-  // All the single-path virtual FSFU methods expect the context information
-  // to be in the src_* variables, not the dest_* variables, so we have to
-  // make a new context if we want to call them on the dest_file_path.
-  scoped_ptr<FileSystemOperationContext> dest_context(
-      context->CreateInheritedContextForDest());
+  } else {
+    // All the single-path virtual FSFU methods expect the context information
+    // to be in the src_* variables, not the dest_* variables, so we have to
+    // make a new context if we want to call them on the dest_file_path.
+    dest_context = &local_dest_context;
+    dest_context->set_src_type(context->dest_type());
+    dest_context->set_src_origin_url(context->dest_origin_url());
+    dest_context->set_src_virtual_path(context->dest_virtual_path());
+    dest_context->set_allowed_bytes_growth(context->allowed_bytes_growth());
+  }

[Dai Mikurube (NOT FULLTIME), 2011/08/12 05:17:54]

I guess passing a correct context is better even if not used currently. (e.g.
virtual_path in the dest_context for same_file_system==true is not correct.)

 
   // Exits earlier if the source path does not exist.
   if (!PathExists(context, src_file_path))
     return base::PLATFORM_FILE_ERROR_NOT_FOUND;
 
   // The parent of the |dest_file_path| does not exist.
-  if (!ParentExists(dest_context.get(), dest_util, dest_file_path))
+  if (!ParentExists(dest_context, dest_util, dest_file_path))
     return base::PLATFORM_FILE_ERROR_NOT_FOUND;
 
   // It is an error to try to copy/move an entry into its child.
@@ -275,14 +281,14 @@ FileSystemFileUtil::PerformCommonCheckAndPreparationForMoveAndCopy(
     return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
 
   // Now it is ok to return if the |dest_file_path| does not exist.
-  if (!dest_util->PathExists(dest_context.get(), dest_file_path))
+  if (!dest_util->PathExists(dest_context, dest_file_path))
     return base::PLATFORM_FILE_OK;
 
   // |src_file_path| exists and is a directory.
   // |dest_file_path| exists and is a file.
   bool src_is_directory = DirectoryExists(context, src_file_path);
   bool dest_is_directory =
-      dest_util->DirectoryExists(dest_context.get(), dest_file_path);
+      dest_util->DirectoryExists(dest_context, dest_file_path);
   if (src_is_directory && !dest_is_directory)
     return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
 
@@ -301,11 +307,10 @@ FileSystemFileUtil::PerformCommonCheckAndPreparationForMoveAndCopy(
     // the file_util's Copy or Move method doesn't perform overwrite
     // on all platforms, so we delete the destination directory here.
     // TODO(kinuko): may be better to change the file_util::{Copy,Move}.
-    PlatformFileError error = dest_util->Delete(
-        dest_context.get(), dest_file_path, false /* recursive */);
-    context->ImportAllowedBytesGrowth(*dest_context);
-    if (base::PLATFORM_FILE_OK != error) {
-      if (!dest_util->IsDirectoryEmpty(dest_context.get(), dest_file_path))
+    if (base::PLATFORM_FILE_OK !=
+        dest_util->Delete(dest_context, dest_file_path,
+                          false /* recursive */)) {
+      if (!dest_util->IsDirectoryEmpty(dest_context, dest_file_path))
         return base::PLATFORM_FILE_ERROR_NOT_EMPTY;
       return base::PLATFORM_FILE_ERROR_FAILED;
     }
@@ -342,21 +347,27 @@ PlatformFileError FileSystemFileUtil::CopyOrMoveDirectory(
       const FilePath& dest_file_path,
       bool copy) {
   FileSystemFileUtil* dest_util = context->dest_file_system_file_util();
-  scoped_ptr<FileSystemOperationContext> dest_context(
-      context->CreateInheritedContextForDest());
+  FileSystemOperationContext dest_context(
+      context->file_system_context(), dest_util);
+  // All the single-path virtual FSFU methods expect the context information to
+  // be in the src_* variables, not the dest_* variables, so we have to make a
+  // new context if we want to call them on the dest_file_path.
+  dest_context.set_src_type(context->dest_type());
+  dest_context.set_src_origin_url(context->dest_origin_url());
+  dest_context.set_src_virtual_path(context->dest_virtual_path());
+  dest_context.set_allowed_bytes_growth(context->allowed_bytes_growth());
 
   // Re-check PerformCommonCheckAndPreparationForMoveAndCopy() by DCHECK.
   DCHECK(DirectoryExists(context, src_file_path));
-  DCHECK(ParentExists(dest_context.get(), dest_util, dest_file_path));
-  DCHECK(!dest_util->PathExists(dest_context.get(), dest_file_path));
+  DCHECK(ParentExists(&dest_context, dest_util, dest_file_path));
+  DCHECK(!dest_util->PathExists(&dest_context, dest_file_path));
   if ((context->src_origin_url() == context->dest_origin_url()) &&
       (context->src_type() == context->dest_type()))
     DCHECK(!src_file_path.IsParent(dest_file_path));
 
-  if (!dest_util->DirectoryExists(dest_context.get(), dest_file_path)) {
-    PlatformFileError error = dest_util->CreateDirectory(dest_context.get(),
+  if (!dest_util->DirectoryExists(&dest_context, dest_file_path)) {
+    PlatformFileError error = dest_util->CreateDirectory(&dest_context,
         dest_file_path, false, false);
-    context->ImportAllowedBytesGrowth(*dest_context);
     if (error != base::PLATFORM_FILE_OK)
       return error;
   }
@@ -369,21 +380,13 @@ PlatformFileError FileSystemFileUtil::CopyOrMoveDirectory(
     src_file_path.AppendRelativePath(src_file_path_each, &dest_file_path_each);
 
     if (file_enum->IsDirectory()) {
-      scoped_ptr<FileSystemOperationContext> new_directory_context(
-          dest_context->CreateInheritedContextWithNewVirtualPaths(
-              dest_file_path_each, FilePath()));
-      PlatformFileError error = dest_util->CreateDirectory(
-          new_directory_context.get(), dest_file_path_each, false, false);
-      context->ImportAllowedBytesGrowth(*new_directory_context);
+      PlatformFileError error = dest_util->CreateDirectory(&dest_context,
+          dest_file_path_each, false, false);

[Dai Mikurube (NOT FULLTIME), 2011/08/12 05:17:54]

ditto.  The function names (CreateInherited..., Import...) might be to be fixed,
but a correct context is better.  This dest_context's virtual_path doesn't work
for CreateDirectory though virtual_path is not used currently.

[ericu, 2011/08/15 23:47:10]

I agree with you about putting CreateInheritedContextForDest back; the code's
already in two places in this file, so we should consolidate it into one.  I'm
less sure about CreateInheritedContextWithNewVirtualPaths.  It looks like we're
doing a lot of bookkeeping and allocations just to keep the FSOC members, which
aren't used, in sync with the parameters, which carry the same information here.
 Perhaps there's a way to get rid of those FSOC members entirely?

It looks like they're only ever used to pass paths from a method to a later
method [called after looking up quota] in FileSystemOperation.  If that's the
case, we should really just come up with another way to pass that parameter. 
Either the quota callback should become a closure that carries the information,
or perhaps we should just add it as a data member in FileSystemOperation. 
There's precedent; members such as file_writer_delegate_ and exclusive_ are kept
around only for single operations, and thus have no business being in the FSOC. 
Or of course we could just crack the filesystem URL again to get the path out,
but that also seems wasteful.

[Dai Mikurube (NOT FULLTIME), 2011/08/16 05:11:00]

Ah, make sense.  I agree with taking *_virtual_path away from FSOC and moving
them into FSO like exclusive_.  They're used only for Delayed... in FSO now.

       if (error != base::PLATFORM_FILE_OK)
         return error;
     } else {
-      scoped_ptr<FileSystemOperationContext> copy_context(
-          context->CreateInheritedContextWithNewVirtualPaths(
-              src_file_path_each, dest_file_path_each));
       PlatformFileError error = CopyOrMoveFileHelper(
-          copy_context.get(), src_file_path_each, dest_file_path_each, copy);
-      context->ImportAllowedBytesGrowth(*copy_context);
+          context, src_file_path_each, dest_file_path_each, copy);
       if (error != base::PLATFORM_FILE_OK)
         return error;
     }
@@ -469,12 +472,7 @@ PlatformFileError FileSystemFileUtil::DeleteDirectoryRecursive(
       directories.push(file_path_each);
     } else {
       // DeleteFile here is the virtual overridden member function.
-      scoped_ptr<FileSystemOperationContext> inherited_context(
-          context->CreateInheritedContextWithNewVirtualPaths(
-              file_path_each, FilePath()));
-      PlatformFileError error =
-          DeleteFile(inherited_context.get(), file_path_each);
-      context->ImportAllowedBytesGrowth(*inherited_context);
+      PlatformFileError error = DeleteFile(context, file_path_each);
       if (error == base::PLATFORM_FILE_ERROR_NOT_FOUND)
         return base::PLATFORM_FILE_ERROR_FAILED;
       else if (error != base::PLATFORM_FILE_OK)
@@ -483,12 +481,7 @@ PlatformFileError FileSystemFileUtil::DeleteDirectoryRecursive(
   }
 
   while (!directories.empty()) {
-    scoped_ptr<FileSystemOperationContext> inherited_context(
-        context->CreateInheritedContextWithNewVirtualPaths(
-            directories.top(), FilePath()));
-    PlatformFileError error =
-        DeleteSingleDirectory(inherited_context.get(), directories.top());
-    context->ImportAllowedBytesGrowth(*inherited_context);
+    PlatformFileError error = DeleteSingleDirectory(context, directories.top());
     if (error == base::PLATFORM_FILE_ERROR_NOT_FOUND)
       return base::PLATFORM_FILE_ERROR_FAILED;
     else if (error != base::PLATFORM_FILE_OK)