Simplify directory path accounting.

webkit/fileapi/file_system_file_util.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "webkit/fileapi/file_system_file_util.h"
 
 #include <stack>
 #include <vector>
 
 #include "base/file_util_proxy.h"
@@ skipping 118 matching lines @@
     // http://code.google.com/p/chromium-os/issues/detail?id=15948
     // This currently just prevents a file from showing up at all
     // if it's a link, hence preventing arbitary 'read' exploits.
     if (!file_util::IsLink(file_path.Append(entry.name)))
       entries->push_back(entry);
   }
   return base::PLATFORM_FILE_OK;
 }
 
 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;
 
   bool path_exists = file_util::PathExists(file_path);
   if (exclusive && path_exists)
     return base::PLATFORM_FILE_ERROR_EXISTS;
 
   // If file exists at the path.
   if (path_exists && !file_util::DirectoryExists(file_path))
@@ skipping 90 matching lines @@
 PlatformFileError
 FileSystemFileUtil::PerformCommonCheckAndPreparationForMoveAndCopy(
     FileSystemOperationContext* context,
     const FilePath& src_file_path,
     const FilePath& dest_file_path) {
   bool same_file_system =
      (context->src_origin_url() == context->dest_origin_url()) &&
      (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.
   if (same_file_system && src_file_path.IsParent(dest_file_path))
     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;
 
   // |src_file_path| exists and is a file.
   // |dest_file_path| exists and is a directory.
   if (!src_is_directory && dest_is_directory)
     return base::PLATFORM_FILE_ERROR_INVALID_OPERATION;
 
   // It is an error to copy/move an entry into the same path.
   if (same_file_system && (src_file_path.value() == dest_file_path.value()))
     return base::PLATFORM_FILE_ERROR_EXISTS;
 
   if (dest_is_directory) {
     // It is an error to copy/move an entry to a non-empty directory.
     // Otherwise the copy/move attempt must overwrite the destination, but
     // 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;
     }
   }
   return base::PLATFORM_FILE_OK;
 }
 
 PlatformFileError FileSystemFileUtil::CopyOrMoveFile(
       FileSystemOperationContext* unused,
       const FilePath& src_file_path,
@@ skipping 16 matching lines @@
       const FilePath& dest_file_path) {
   return CopyOrMoveFile(context, src_file_path, dest_file_path, true);
 }
 
 PlatformFileError FileSystemFileUtil::CopyOrMoveDirectory(
       FileSystemOperationContext* context,
       const FilePath& src_file_path,
       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;
   }
 
   scoped_ptr<AbstractFileEnumerator> file_enum(
       CreateFileEnumerator(context, src_file_path));
   FilePath src_file_path_each;
   while (!(src_file_path_each = file_enum->Next()).empty()) {
     FilePath dest_file_path_each(dest_file_path);
     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;
     }
   }
 
   if (!copy) {
     PlatformFileError error = Delete(context, src_file_path, true);
     if (error != base::PLATFORM_FILE_OK)
       return error;
   }
@@ skipping 65 matching lines @@
   scoped_ptr<AbstractFileEnumerator> file_enum(
       CreateFileEnumerator(context, file_path));
   FilePath file_path_each;
 
   std::stack<FilePath> directories;
   while (!(file_path_each = file_enum->Next()).empty()) {
     if (file_enum->IsDirectory()) {
       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)
         return error;
     }
   }
 
   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)
       return error;
     directories.pop();
   }
   return DeleteSingleDirectory(context, file_path);
 }
 
 bool FileSystemFileUtil::PathExists(
@@ skipping 46 matching lines @@
 FileSystemFileUtil::CreateFileEnumerator(
     FileSystemOperationContext* unused,
     const FilePath& root_path) {
   return new FileSystemFileEnumerator(
       root_path, true, static_cast<file_util::FileEnumerator::FILE_TYPE>(
       file_util::FileEnumerator::FILES |
       file_util::FileEnumerator::DIRECTORIES));
 }
 
 }  // namespace fileapi