diff options
| -rw-r--r-- | base/file_util_proxy.cc | 125 | ||||
| -rw-r--r-- | base/file_util_proxy.h | 5 | ||||
| -rw-r--r-- | base/platform_file.h | 4 | ||||
| -rw-r--r-- | chrome/browser/file_system/file_system_dispatcher_host.cc | 5 | ||||
| -rw-r--r-- | chrome/browser/file_system/file_system_dispatcher_host.h | 3 | ||||
| -rw-r--r-- | chrome/common/file_system/file_system_dispatcher.cc | 4 | ||||
| -rw-r--r-- | chrome/common/file_system/file_system_dispatcher.h | 1 | ||||
| -rw-r--r-- | chrome/common/file_system/webfilesystem_impl.cc | 11 | ||||
| -rw-r--r-- | chrome/common/file_system/webfilesystem_impl.h | 4 | ||||
| -rw-r--r-- | chrome/common/render_messages_internal.h | 5 | ||||
| -rw-r--r-- | chrome/renderer/pepper_plugin_delegate_impl.cc | 2 | ||||
| -rw-r--r-- | webkit/blob/deletable_file_reference.cc | 2 | ||||
| -rw-r--r-- | webkit/fileapi/file_system_operation.cc | 7 | ||||
| -rw-r--r-- | webkit/fileapi/file_system_operation.h | 5 | ||||
| -rw-r--r-- | webkit/fileapi/file_system_operation_unittest.cc | 258 | ||||
| -rw-r--r-- | webkit/glue/webkit_glue.cc | 2 | ||||
| -rw-r--r-- | webkit/tools/test_shell/simple_file_system.cc | 9 | ||||
| -rw-r--r-- | webkit/tools/test_shell/simple_file_system.h | 8 |
18 files changed, 328 insertions, 132 deletions
diff --git a/base/file_util_proxy.cc b/base/file_util_proxy.cc index d71f374..b3b18f8 100644 --- a/base/file_util_proxy.cc +++ b/base/file_util_proxy.cc @@ -10,6 +10,63 @@ // that all of the base:: prefixes would be unnecessary. namespace { +namespace { + +// Performs common checks for move and copy. +// This also removes the destination directory if it's non-empty and all other +// checks are passed (so that the copy/move correctly overwrites the destination). +static base::PlatformFileError PerformCommonCheckAndPreparationForMoveAndCopy( + const FilePath& src_file_path, + const FilePath& dest_file_path) { + // Exits earlier if the source path does not exist. + if (!file_util::PathExists(src_file_path)) + return base::PLATFORM_FILE_ERROR_NOT_FOUND; + + // The parent of the |dest_file_path| does not exist. + if (!file_util::DirectoryExists(dest_file_path.DirName())) + return base::PLATFORM_FILE_ERROR_NOT_FOUND; + + // It is an error to try to copy/move an entry into its child. + if (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 (!file_util::PathExists(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 = file_util::DirectoryExists(src_file_path); + bool dest_is_directory = file_util::DirectoryExists(dest_file_path); + if (src_is_directory && !dest_is_directory) + return base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY; + + // |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_NOT_A_FILE; + + // It is an error to copy/move an entry into the same path. + if (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}. + if (!file_util::Delete(dest_file_path, false /* recursive */)) { + if (!file_util::IsDirectoryEmpty(dest_file_path)) + return base::PLATFORM_FILE_ERROR_NOT_EMPTY; + return base::PLATFORM_FILE_ERROR_FAILED; + } + } + return base::PLATFORM_FILE_OK; +} + +} // anonymous namespace + class MessageLoopRelay : public base::RefCountedThreadSafe<MessageLoopRelay> { public: @@ -205,7 +262,7 @@ class RelayDelete : public RelayWithStatusCallback { } if (!file_util::Delete(file_path_, recursive_)) { if (!recursive_ && !file_util::IsDirectoryEmpty(file_path_)) { - set_error_code(base::PLATFORM_FILE_ERROR_INVALID_OPERATION); + set_error_code(base::PLATFORM_FILE_ERROR_NOT_EMPTY); return; } set_error_code(base::PLATFORM_FILE_ERROR_FAILED); @@ -229,36 +286,13 @@ class RelayCopy : public RelayWithStatusCallback { protected: virtual void RunWork() { - bool dest_path_exists = file_util::PathExists(dest_file_path_); - if (!dest_path_exists && - !file_util::DirectoryExists(dest_file_path_.DirName())) { - set_error_code(base::PLATFORM_FILE_ERROR_NOT_FOUND); + set_error_code(PerformCommonCheckAndPreparationForMoveAndCopy( + src_file_path_, dest_file_path_)); + if (error_code() != base::PLATFORM_FILE_OK) return; - } - // |src_file_path| exists and is a directory. - // |dest_file_path| exists and is a file. - if (file_util::DirectoryExists(src_file_path_) && - dest_path_exists && !file_util::DirectoryExists(dest_file_path_)) { - set_error_code(base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY); - return; - } - if (file_util::ContainsPath(src_file_path_, dest_file_path_)) { - set_error_code(base::PLATFORM_FILE_ERROR_FAILED); - return; - } if (!file_util::CopyDirectory(src_file_path_, dest_file_path_, - true /* recursive */)) { - if (!file_util::PathExists(src_file_path_)) { - set_error_code(base::PLATFORM_FILE_ERROR_NOT_FOUND); - return; - } - if (src_file_path_.value() == dest_file_path_.value()) { - set_error_code(base::PLATFORM_FILE_ERROR_EXISTS); - return; - } - // Something else went wrong. + true /* recursive */)) set_error_code(base::PLATFORM_FILE_ERROR_FAILED); - } } private: @@ -278,36 +312,12 @@ class RelayMove : public RelayWithStatusCallback { protected: virtual void RunWork() { - bool dest_path_exists = file_util::PathExists(dest_file_path_); - if (!dest_path_exists && - !file_util::DirectoryExists(dest_file_path_.DirName())) { - set_error_code(base::PLATFORM_FILE_ERROR_NOT_FOUND); + set_error_code(PerformCommonCheckAndPreparationForMoveAndCopy( + src_file_path_, dest_file_path_)); + if (error_code() != base::PLATFORM_FILE_OK) return; - } - // |src_file_path| exists and is a directory. - // |dest_file_path| exists and is a file. - if (file_util::DirectoryExists(src_file_path_) && - dest_path_exists && - !file_util::DirectoryExists(dest_file_path_)) { - set_error_code(base::PLATFORM_FILE_ERROR_EXISTS); - return; - } - if (file_util::ContainsPath(src_file_path_, dest_file_path_)) { - set_error_code(base::PLATFORM_FILE_ERROR_INVALID_OPERATION); - return; - } - if (!file_util::Move(src_file_path_, dest_file_path_)) { - if (!file_util::PathExists(src_file_path_)) { - set_error_code(base::PLATFORM_FILE_ERROR_NOT_FOUND); - return; - } - if (src_file_path_.value() == dest_file_path_.value()) { - set_error_code(base::PLATFORM_FILE_ERROR_EXISTS); - return; - } - // Something else went wrong. + if (!file_util::Move(src_file_path_, dest_file_path_)) set_error_code(base::PLATFORM_FILE_ERROR_FAILED); - } } private: @@ -703,9 +713,10 @@ bool FileUtilProxy::Close(scoped_refptr<MessageLoopProxy> message_loop_proxy, // static bool FileUtilProxy::Delete(scoped_refptr<MessageLoopProxy> message_loop_proxy, const FilePath& file_path, + bool recursive, StatusCallback* callback) { return Start(FROM_HERE, message_loop_proxy, - new RelayDelete(file_path, false, callback)); + new RelayDelete(file_path, recursive, callback)); } // static diff --git a/base/file_util_proxy.h b/base/file_util_proxy.h index 32b78630..e716ab7 100644 --- a/base/file_util_proxy.h +++ b/base/file_util_proxy.h @@ -89,6 +89,7 @@ class FileUtilProxy { // If destination file doesn't exist or destination's parent // doesn't exists. // If source dir exists but destination path is an existing file. + // If source file exists but destination path is an existing directory. // If source is a parent of destination. // If source doesn't exists. static bool Copy(scoped_refptr<MessageLoopProxy> message_loop_proxy, @@ -105,9 +106,11 @@ class FileUtilProxy { bool recursive, StatusCallback* callback); - // Deletes a file or empty directory. + // Deletes a file or a directory. + // It is an error to delete a non-empty directory with recursive=false. static bool Delete(scoped_refptr<MessageLoopProxy> message_loop_proxy, const FilePath& file_path, + bool recursive, StatusCallback* callback); // Moves a file or a directory from src_file_path to dest_file_path. diff --git a/base/platform_file.h b/base/platform_file.h index 5024717..b350f8c 100644 --- a/base/platform_file.h +++ b/base/platform_file.h @@ -60,7 +60,9 @@ enum PlatformFileError { PLATFORM_FILE_ERROR_NOT_A_DIRECTORY = -9, PLATFORM_FILE_ERROR_INVALID_OPERATION = -10, PLATFORM_FILE_ERROR_SECURITY = -11, - PLATFORM_FILE_ERROR_ABORT = -12 + PLATFORM_FILE_ERROR_ABORT = -12, + PLATFORM_FILE_ERROR_NOT_A_FILE = -13, + PLATFORM_FILE_ERROR_NOT_EMPTY = -14, }; // Used to hold information about a given file. diff --git a/chrome/browser/file_system/file_system_dispatcher_host.cc b/chrome/browser/file_system/file_system_dispatcher_host.cc index 6dd6072..96571ce 100644 --- a/chrome/browser/file_system/file_system_dispatcher_host.cc +++ b/chrome/browser/file_system/file_system_dispatcher_host.cc @@ -161,10 +161,11 @@ void FileSystemDispatcherHost::OnCopy( GetNewOperation(request_id)->Copy(src_path, dest_path); } -void FileSystemDispatcherHost::OnRemove(int request_id, const FilePath& path) { +void FileSystemDispatcherHost::OnRemove( + int request_id, const FilePath& path, bool recursive) { if (!CheckValidFileSystemPath(path, request_id)) return; - GetNewOperation(request_id)->Remove(path); + GetNewOperation(request_id)->Remove(path, recursive); } void FileSystemDispatcherHost::OnReadMetadata( diff --git a/chrome/browser/file_system/file_system_dispatcher_host.h b/chrome/browser/file_system/file_system_dispatcher_host.h index 22a560a..68110be 100644 --- a/chrome/browser/file_system/file_system_dispatcher_host.h +++ b/chrome/browser/file_system/file_system_dispatcher_host.h @@ -21,7 +21,6 @@ namespace base { class Time; } -class ChromeFileSystemOperation; class FileSystemHostContext; class GURL; class HostContentSettingsMap; @@ -50,7 +49,7 @@ class FileSystemDispatcherHost void OnCopy(int request_id, const FilePath& src_path, const FilePath& dest_path); - void OnRemove(int request_id, const FilePath& path); + void OnRemove(int request_id, const FilePath& path, bool recursive); void OnReadMetadata(int request_id, const FilePath& path); void OnCreate(int request_id, const FilePath& path, diff --git a/chrome/common/file_system/file_system_dispatcher.cc b/chrome/common/file_system/file_system_dispatcher.cc index a8761a2..bc52f3a 100644 --- a/chrome/common/file_system/file_system_dispatcher.cc +++ b/chrome/common/file_system/file_system_dispatcher.cc @@ -67,10 +67,11 @@ bool FileSystemDispatcher::Copy( bool FileSystemDispatcher::Remove( const FilePath& path, + bool recursive, fileapi::FileSystemCallbackDispatcher* dispatcher) { int request_id = dispatchers_.Add(dispatcher); return ChildThread::current()->Send( - new ViewHostMsg_FileSystem_Remove(request_id, path)); + new ViewHostMsg_FileSystem_Remove(request_id, path, recursive)); } bool FileSystemDispatcher::ReadMetadata( @@ -219,4 +220,3 @@ void FileSystemDispatcher::DidWrite( if (complete) dispatchers_.Remove(request_id); } - diff --git a/chrome/common/file_system/file_system_dispatcher.h b/chrome/common/file_system/file_system_dispatcher.h index 80443c4..8802c1e 100644 --- a/chrome/common/file_system/file_system_dispatcher.h +++ b/chrome/common/file_system/file_system_dispatcher.h @@ -43,6 +43,7 @@ class FileSystemDispatcher { const FilePath& dest_path, fileapi::FileSystemCallbackDispatcher* dispatcher); bool Remove(const FilePath& path, + bool recursive, fileapi::FileSystemCallbackDispatcher* dispatcher); bool ReadMetadata(const FilePath& path, fileapi::FileSystemCallbackDispatcher* dispatcher); diff --git a/chrome/common/file_system/webfilesystem_impl.cc b/chrome/common/file_system/webfilesystem_impl.cc index 9128cf1..5b180f6 100644 --- a/chrome/common/file_system/webfilesystem_impl.cc +++ b/chrome/common/file_system/webfilesystem_impl.cc @@ -47,6 +47,16 @@ void WebFileSystemImpl::remove(const WebString& path, FileSystemDispatcher* dispatcher = ChildThread::current()->file_system_dispatcher(); dispatcher->Remove(webkit_glue::WebStringToFilePath(path), + false /* recursive */, + new WebFileSystemCallbackDispatcher(callbacks)); +} + +void WebFileSystemImpl::removeRecursively(const WebString& path, + WebFileSystemCallbacks* callbacks) { + FileSystemDispatcher* dispatcher = + ChildThread::current()->file_system_dispatcher(); + dispatcher->Remove(webkit_glue::WebStringToFilePath(path), + true /* recursive */, new WebFileSystemCallbackDispatcher(callbacks)); } @@ -104,4 +114,3 @@ WebKit::WebFileWriter* WebFileSystemImpl::createFileWriter( const WebString& path, WebKit::WebFileWriterClient* client) { return new WebFileWriterImpl(path, client); } - diff --git a/chrome/common/file_system/webfilesystem_impl.h b/chrome/common/file_system/webfilesystem_impl.h index 6892eac..fc051e0 100644 --- a/chrome/common/file_system/webfilesystem_impl.h +++ b/chrome/common/file_system/webfilesystem_impl.h @@ -32,6 +32,10 @@ class WebFileSystemImpl : public WebKit::WebFileSystem { const WebKit::WebString& path, WebKit::WebFileSystemCallbacks*); + virtual void removeRecursively( + const WebKit::WebString& path, + WebKit::WebFileSystemCallbacks*); + virtual void readMetadata( const WebKit::WebString& path, WebKit::WebFileSystemCallbacks*); diff --git a/chrome/common/render_messages_internal.h b/chrome/common/render_messages_internal.h index 38f6332..f5643af 100644 --- a/chrome/common/render_messages_internal.h +++ b/chrome/common/render_messages_internal.h @@ -2846,9 +2846,10 @@ IPC_BEGIN_MESSAGES(ViewHost) FilePath /* dest path */) // WebFileSystem::remove() message. - IPC_MESSAGE_CONTROL2(ViewHostMsg_FileSystem_Remove, + IPC_MESSAGE_CONTROL3(ViewHostMsg_FileSystem_Remove, int /* request_id */, - FilePath /* path */) + FilePath /* path */, + bool /* recursive */) // WebFileSystem::readMetadata() message. IPC_MESSAGE_CONTROL2(ViewHostMsg_FileSystem_ReadMetadata, diff --git a/chrome/renderer/pepper_plugin_delegate_impl.cc b/chrome/renderer/pepper_plugin_delegate_impl.cc index c3b9f9b..8960d29 100644 --- a/chrome/renderer/pepper_plugin_delegate_impl.cc +++ b/chrome/renderer/pepper_plugin_delegate_impl.cc @@ -711,7 +711,7 @@ bool PepperPluginDelegateImpl::Delete( fileapi::FileSystemCallbackDispatcher* dispatcher) { FileSystemDispatcher* file_system_dispatcher = ChildThread::current()->file_system_dispatcher(); - return file_system_dispatcher->Remove(path, dispatcher); + return file_system_dispatcher->Remove(path, false /* recursive */, dispatcher); } bool PepperPluginDelegateImpl::Rename( diff --git a/webkit/blob/deletable_file_reference.cc b/webkit/blob/deletable_file_reference.cc index b005eeb..9f49943 100644 --- a/webkit/blob/deletable_file_reference.cc +++ b/webkit/blob/deletable_file_reference.cc @@ -57,7 +57,7 @@ DeletableFileReference::DeletableFileReference( DeletableFileReference::~DeletableFileReference() { DCHECK(map()->find(path_)->second == this); map()->erase(path_); - base::FileUtilProxy::Delete(file_thread_, path_, NULL); + base::FileUtilProxy::Delete(file_thread_, path_, false /* recursive */, NULL); } } // namespace webkit_blob diff --git a/webkit/fileapi/file_system_operation.cc b/webkit/fileapi/file_system_operation.cc index e636420..29336a8 100644 --- a/webkit/fileapi/file_system_operation.cc +++ b/webkit/fileapi/file_system_operation.cc @@ -118,14 +118,15 @@ void FileSystemOperation::ReadDirectory(const FilePath& path) { &FileSystemOperation::DidReadDirectory)); } -void FileSystemOperation::Remove(const FilePath& path) { +void FileSystemOperation::Remove(const FilePath& path, bool recursive) { #ifndef NDEBUG DCHECK(kOperationNone == pending_operation_); pending_operation_ = kOperationRemove; #endif - base::FileUtilProxy::Delete(proxy_, path, callback_factory_.NewCallback( - &FileSystemOperation::DidFinishFileOperation)); + base::FileUtilProxy::Delete(proxy_, path, recursive, + callback_factory_.NewCallback( + &FileSystemOperation::DidFinishFileOperation)); } void FileSystemOperation::Write( diff --git a/webkit/fileapi/file_system_operation.h b/webkit/fileapi/file_system_operation.h index ab6d56c..ba65b7d 100644 --- a/webkit/fileapi/file_system_operation.h +++ b/webkit/fileapi/file_system_operation.h @@ -45,9 +45,6 @@ class FileSystemOperation { void Copy(const FilePath& src_path, const FilePath& dest_path); - // If |dest_path| exists and is a directory, behavior is unspecified or - // varies for different platforms. TODO(kkanetkar): Fix this as per spec - // when it is addressed in spec. void Move(const FilePath& src_path, const FilePath& dest_path); @@ -59,7 +56,7 @@ class FileSystemOperation { void ReadDirectory(const FilePath& path); - void Remove(const FilePath& path); + void Remove(const FilePath& path, bool recursive); void Write(const FilePath& path, const GURL& blob_url, int64 offset); diff --git a/webkit/fileapi/file_system_operation_unittest.cc b/webkit/fileapi/file_system_operation_unittest.cc index 8a02ade..9b07c03 100644 --- a/webkit/fileapi/file_system_operation_unittest.cc +++ b/webkit/fileapi/file_system_operation_unittest.cc @@ -89,9 +89,7 @@ class FileSystemOperationTest : public testing::Test { } protected: - // Common temp base for all non-existing file/dir path test cases. - // This is in case a dir on test machine exists. It's better to - // create temp and then create non-existing file paths under it. + // Common temp base for nondestructive uses. ScopedTempDir base_; int request_id_; @@ -112,17 +110,14 @@ TEST_F(FileSystemOperationTest, TestMoveFailureSrcDoesntExist) { EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } -// crbug.com/57940 -#if defined(OS_WINDOWS) -#define MAYBE_TestMoveFailureContainsPath FAILS_TestMoveFailureContainsPath -#else -#define MAYBE_TestMoveFailureContainsPath TestMoveFailureContainsPath -#endif - -TEST_F(FileSystemOperationTest, MAYBE_TestMoveFailureContainsPath) { - ScopedTempDir dir_under_base; - dir_under_base.CreateUniqueTempDirUnderPath(base_.path()); - operation()->Move(base_.path(), dir_under_base.path()); +TEST_F(FileSystemOperationTest, TestMoveFailureContainsPath) { + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + FilePath dest_dir_path; + ASSERT_TRUE(file_util::CreateTemporaryDirInDir(src_dir.path(), + FILE_PATH_LITERAL("child_dir"), + &dest_dir_path)); + operation()->Move(src_dir.path(), dest_dir_path); MessageLoop::current()->RunAllPending(); EXPECT_EQ(base::PLATFORM_FILE_ERROR_INVALID_OPERATION, mock_dispatcher_->status()); @@ -131,14 +126,51 @@ TEST_F(FileSystemOperationTest, MAYBE_TestMoveFailureContainsPath) { TEST_F(FileSystemOperationTest, TestMoveFailureSrcDirExistsDestFile) { // Src exists and is dir. Dest is a file. + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + ScopedTempDir dest_dir; ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); FilePath dest_file; file_util::CreateTemporaryFileInDir(dest_dir.path(), &dest_file); - operation()->Move(base_.path(), dest_file); + operation()->Move(src_dir.path(), dest_file); MessageLoop::current()->RunAllPending(); - EXPECT_EQ(base::PLATFORM_FILE_ERROR_EXISTS, mock_dispatcher_->status()); + EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY, + mock_dispatcher_->status()); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); +} + +TEST_F(FileSystemOperationTest, TestMoveFailureSrcFileExistsDestNonEmptyDir) { + // Src exists and is a directory. Dest is a non-empty directory. + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + + ScopedTempDir dest_dir; + ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); + FilePath child_file; + file_util::CreateTemporaryFileInDir(dest_dir.path(), &child_file); + + operation()->Move(src_dir.path(), dest_dir.path()); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_EMPTY, mock_dispatcher_->status()); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); +} + +TEST_F(FileSystemOperationTest, TestMoveFailureSrcFileExistsDestDir) { + // Src exists and is a file. Dest is a directory. + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + FilePath src_file; + file_util::CreateTemporaryFileInDir(src_dir.path(), &src_file); + + ScopedTempDir dest_dir; + ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); + + operation()->Move(src_file, dest_dir.path()); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_FILE, + mock_dispatcher_->status()); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } @@ -148,7 +180,7 @@ TEST_F(FileSystemOperationTest, TestMoveFailureDestParentDoesntExist) { ASSERT_TRUE(src_dir.CreateUniqueTempDir()); FilePath nonexisting_file = base_.path().Append( FILE_PATH_LITERAL("NonexistingDir")).Append( - FILE_PATH_LITERAL("NonexistingFile"));; + FILE_PATH_LITERAL("NonexistingFile")); operation()->Move(src_dir.path(), nonexisting_file); MessageLoop::current()->RunAllPending(); @@ -191,6 +223,57 @@ TEST_F(FileSystemOperationTest, TestMoveSuccessSrcFileAndNew) { EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } +TEST_F(FileSystemOperationTest, TestMoveSuccessSrcDirAndOverwrite) { + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + + ScopedTempDir dest_dir; + ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); + + operation()->Move(src_dir.path(), dest_dir.path()); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); + EXPECT_FALSE(file_util::DirectoryExists(src_dir.path())); + + // Make sure we've overwritten but not moved the source under the |dest_dir|. + EXPECT_TRUE(file_util::DirectoryExists(dest_dir.path())); + EXPECT_FALSE(file_util::DirectoryExists( + dest_dir.path().Append(src_dir.path().BaseName()))); +} + +TEST_F(FileSystemOperationTest, TestMoveSuccessSrcDirAndNew) { + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + + ScopedTempDir dir; + ASSERT_TRUE(dir.CreateUniqueTempDir()); + FilePath dest_dir_path(dir.path().Append(FILE_PATH_LITERAL("NewDirectory"))); + + operation()->Move(src_dir.path(), dest_dir_path); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); + EXPECT_FALSE(file_util::DirectoryExists(src_dir.path())); + EXPECT_TRUE(file_util::DirectoryExists(dest_dir_path)); +} + +TEST_F(FileSystemOperationTest, TestMoveSuccessSrcDirRecursive) { + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + FilePath child_file; + file_util::CreateTemporaryFileInDir(src_dir.path(), &child_file); + + ScopedTempDir dest_dir; + ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); + + operation()->Move(src_dir.path(), dest_dir.path()); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); + EXPECT_TRUE(FileExists(dest_dir.path().Append(child_file.BaseName()))); +} + TEST_F(FileSystemOperationTest, TestCopyFailureSrcDoesntExist) { FilePath src(base_.path().Append(FILE_PATH_LITERAL("a"))); FilePath dest(base_.path().Append(FILE_PATH_LITERAL("b"))); @@ -200,35 +283,70 @@ TEST_F(FileSystemOperationTest, TestCopyFailureSrcDoesntExist) { EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } -// crbug.com/57940 -#if defined(OS_WINDOWS) -#define MAYBE_TestCopyFailureContainsPath FAILS_TestCopyFailureContainsPath -#else -#define MAYBE_TestCopyFailureContainsPath TestCopyFailureContainsPath -#endif - -TEST_F(FileSystemOperationTest, MAYBE_TestCopyFailureContainsPath) { - FilePath file_under_base = base_.path().Append(FILE_PATH_LITERAL("b")); - operation()->Copy(base_.path(), file_under_base); +TEST_F(FileSystemOperationTest, TestCopyFailureContainsPath) { + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + FilePath dest_dir_path; + ASSERT_TRUE(file_util::CreateTemporaryDirInDir(src_dir.path(), + FILE_PATH_LITERAL("child_dir"), + &dest_dir_path)); + operation()->Copy(src_dir.path(), dest_dir_path); MessageLoop::current()->RunAllPending(); - EXPECT_EQ(base::PLATFORM_FILE_ERROR_FAILED, mock_dispatcher_->status()); + EXPECT_EQ(base::PLATFORM_FILE_ERROR_INVALID_OPERATION, + mock_dispatcher_->status()); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } TEST_F(FileSystemOperationTest, TestCopyFailureSrcDirExistsDestFile) { // Src exists and is dir. Dest is a file. - ScopedTempDir dir; - ASSERT_TRUE(dir.CreateUniqueTempDir()); + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + + ScopedTempDir dest_dir; + ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); FilePath dest_file; - file_util::CreateTemporaryFileInDir(dir.path(), &dest_file); + file_util::CreateTemporaryFileInDir(dest_dir.path(), &dest_file); - operation()->Copy(base_.path(), dest_file); + operation()->Copy(src_dir.path(), dest_file); MessageLoop::current()->RunAllPending(); EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY, mock_dispatcher_->status()); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } +TEST_F(FileSystemOperationTest, TestCopyFailureSrcFileExistsDestNonEmptyDir) { + // Src exists and is a directory. Dest is a non-empty directory. + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + + ScopedTempDir dest_dir; + ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); + FilePath child_file; + file_util::CreateTemporaryFileInDir(dest_dir.path(), &child_file); + + operation()->Copy(src_dir.path(), dest_dir.path()); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_EMPTY, mock_dispatcher_->status()); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); +} + +TEST_F(FileSystemOperationTest, TestCopyFailureSrcFileExistsDestDir) { + // Src exists and is a file. Dest is a directory. + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + FilePath src_file; + file_util::CreateTemporaryFileInDir(src_dir.path(), &src_file); + + ScopedTempDir dest_dir; + ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); + + operation()->Copy(src_file, dest_dir.path()); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_A_FILE, + mock_dispatcher_->status()); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); +} + TEST_F(FileSystemOperationTest, TestCopyFailureDestParentDoesntExist) { // Dest. parent path does not exist. ScopedTempDir dir; @@ -281,7 +399,7 @@ TEST_F(FileSystemOperationTest, TestCopySuccessSrcFileAndNew) { EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } -TEST_F(FileSystemOperationTest, TestCopySuccessSrcDir) { +TEST_F(FileSystemOperationTest, TestCopySuccessSrcDirAndOverwrite) { ScopedTempDir src_dir; ASSERT_TRUE(src_dir.CreateUniqueTempDir()); @@ -292,24 +410,42 @@ TEST_F(FileSystemOperationTest, TestCopySuccessSrcDir) { MessageLoop::current()->RunAllPending(); EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); + + // Make sure we've overwritten but not copied the source under the |dest_dir|. + EXPECT_TRUE(file_util::DirectoryExists(dest_dir.path())); + EXPECT_FALSE(file_util::DirectoryExists( + dest_dir.path().Append(src_dir.path().BaseName()))); } -TEST_F(FileSystemOperationTest, TestCopyDestParentExistsSuccess) { +TEST_F(FileSystemOperationTest, TestCopySuccessSrcDirAndNew) { ScopedTempDir src_dir; ASSERT_TRUE(src_dir.CreateUniqueTempDir()); - FilePath src_file; - file_util::CreateTemporaryFileInDir(src_dir.path(), &src_file); + + ScopedTempDir dir; + ASSERT_TRUE(dir.CreateUniqueTempDir()); + FilePath dest_dir(dir.path().Append(FILE_PATH_LITERAL("NewDirectory"))); + + operation()->Copy(src_dir.path(), dest_dir); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); + EXPECT_TRUE(file_util::DirectoryExists(dest_dir)); +} + +TEST_F(FileSystemOperationTest, TestCopySuccessSrcDirRecursive) { + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + FilePath child_file; + file_util::CreateTemporaryFileInDir(src_dir.path(), &child_file); ScopedTempDir dest_dir; ASSERT_TRUE(dest_dir.CreateUniqueTempDir()); - operation()->Copy(src_file, dest_dir.path()); + operation()->Copy(src_dir.path(), dest_dir.path()); MessageLoop::current()->RunAllPending(); EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); - - FilePath src_filename(src_file.BaseName()); - EXPECT_TRUE(FileExists(dest_dir.path().Append(src_filename))); + EXPECT_TRUE(FileExists(dest_dir.path().Append(child_file.BaseName()))); } TEST_F(FileSystemOperationTest, TestCreateFileFailure) { @@ -367,7 +503,6 @@ TEST_F(FileSystemOperationTest, // Dest. parent path does not exist. FilePath nonexisting(base_.path().Append( FILE_PATH_LITERAL("DirDoesntExist"))); - file_util::EnsureEndsWithSeparator(&nonexisting); FilePath nonexisting_file = nonexisting.Append( FILE_PATH_LITERAL("FileDoesntExist")); operation()->CreateDirectory(nonexisting_file, false, false); @@ -378,7 +513,9 @@ TEST_F(FileSystemOperationTest, TEST_F(FileSystemOperationTest, TestCreateDirFailureDirExists) { // Exclusive and dir existing at path. - operation()->CreateDirectory(base_.path(), true, false); + ScopedTempDir src_dir; + ASSERT_TRUE(src_dir.CreateUniqueTempDir()); + operation()->CreateDirectory(src_dir.path(), true, false); MessageLoop::current()->RunAllPending(); EXPECT_EQ(base::PLATFORM_FILE_ERROR_EXISTS, mock_dispatcher_->status()); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); @@ -406,7 +543,8 @@ TEST_F(FileSystemOperationTest, TestCreateDirSuccess) { EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); // Dir doesn't exist. - FilePath nonexisting_dir_path(FILE_PATH_LITERAL("nonexistingdir")); + FilePath nonexisting_dir_path(base_.path().Append( + FILE_PATH_LITERAL("nonexistingdir"))); operation()->CreateDirectory(nonexisting_dir_path, false, false); MessageLoop::current()->RunAllPending(); EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); @@ -416,9 +554,7 @@ TEST_F(FileSystemOperationTest, TestCreateDirSuccess) { TEST_F(FileSystemOperationTest, TestCreateDirSuccessExclusive) { // Dir doesn't exist. - ScopedTempDir dir; - ASSERT_TRUE(dir.CreateUniqueTempDir()); - FilePath nonexisting_dir_path(dir.path().Append( + FilePath nonexisting_dir_path(base_.path().Append( FILE_PATH_LITERAL("nonexistingdir"))); operation()->CreateDirectory(nonexisting_dir_path, true, false); @@ -534,12 +670,13 @@ TEST_F(FileSystemOperationTest, TestRemoveFailure) { FILE_PATH_LITERAL("NonExistingDir"))); file_util::EnsureEndsWithSeparator(&nonexisting); - operation()->Remove(nonexisting); + operation()->Remove(nonexisting, false /* recursive */); MessageLoop::current()->RunAllPending(); EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, mock_dispatcher_->status()); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); - // By spec recursive is always false. Non-empty dir should fail. + // It's an error to try to remove a non-empty directory if recursive flag + // is false. // parent_dir // | | // child_dir child_file @@ -552,9 +689,9 @@ TEST_F(FileSystemOperationTest, TestRemoveFailure) { ASSERT_TRUE(file_util::CreateTemporaryDirInDir( parent_dir.path(), FILE_PATH_LITERAL("child_dir"), &child_dir)); - operation()->Remove(parent_dir.path()); + operation()->Remove(parent_dir.path(), false /* recursive */); MessageLoop::current()->RunAllPending(); - EXPECT_EQ(base::PLATFORM_FILE_ERROR_INVALID_OPERATION, + EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_EMPTY, mock_dispatcher_->status()); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } @@ -564,11 +701,30 @@ TEST_F(FileSystemOperationTest, TestRemoveSuccess) { ASSERT_TRUE(empty_dir.CreateUniqueTempDir()); EXPECT_TRUE(file_util::DirectoryExists(empty_dir.path())); - operation()->Remove(empty_dir.path()); + operation()->Remove(empty_dir.path(), false /* recursive */); MessageLoop::current()->RunAllPending(); EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); EXPECT_FALSE(file_util::DirectoryExists(empty_dir.path())); EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); + + // Removing a non-empty directory with recursive flag == true should be ok. + // parent_dir + // | | + // child_dir child_file + // Verify deleting parent_dir. + ScopedTempDir parent_dir; + ASSERT_TRUE(parent_dir.CreateUniqueTempDir()); + FilePath child_file; + file_util::CreateTemporaryFileInDir(parent_dir.path(), &child_file); + FilePath child_dir; + ASSERT_TRUE(file_util::CreateTemporaryDirInDir( + parent_dir.path(), FILE_PATH_LITERAL("child_dir"), &child_dir)); + + operation()->Remove(parent_dir.path(), true /* recursive */); + MessageLoop::current()->RunAllPending(); + EXPECT_EQ(kFileOperationSucceeded, mock_dispatcher_->status()); + EXPECT_FALSE(file_util::DirectoryExists(parent_dir.path())); + EXPECT_EQ(request_id_, mock_dispatcher_->request_id()); } TEST_F(FileSystemOperationTest, TestTruncate) { diff --git a/webkit/glue/webkit_glue.cc b/webkit/glue/webkit_glue.cc index 661b87b..f7f60aa 100644 --- a/webkit/glue/webkit_glue.cc +++ b/webkit/glue/webkit_glue.cc @@ -321,6 +321,8 @@ WebKit::WebFileError PlatformFileErrorToWebFileError( case base::PLATFORM_FILE_ERROR_INVALID_OPERATION: case base::PLATFORM_FILE_ERROR_EXISTS: case base::PLATFORM_FILE_ERROR_NOT_A_DIRECTORY: + case base::PLATFORM_FILE_ERROR_NOT_A_FILE: + case base::PLATFORM_FILE_ERROR_NOT_EMPTY: return WebKit::WebFileErrorInvalidModification; case base::PLATFORM_FILE_ERROR_ACCESS_DENIED: return WebKit::WebFileErrorNoModificationAllowed; diff --git a/webkit/tools/test_shell/simple_file_system.cc b/webkit/tools/test_shell/simple_file_system.cc index 8e059ea..411d987 100644 --- a/webkit/tools/test_shell/simple_file_system.cc +++ b/webkit/tools/test_shell/simple_file_system.cc @@ -133,7 +133,14 @@ void SimpleFileSystem::remove( const WebString& path, WebFileSystemCallbacks* callbacks) { FilePath filepath(webkit_glue::WebStringToFilePath(path)); - GetNewOperation(callbacks)->Remove(filepath); + GetNewOperation(callbacks)->Remove(filepath, false /* recursive */); +} + +void SimpleFileSystem::removeRecursively( + const WebString& path, WebFileSystemCallbacks* callbacks) { + FilePath filepath(webkit_glue::WebStringToFilePath(path)); + + GetNewOperation(callbacks)->Remove(filepath, true /* recursive */); } void SimpleFileSystem::readMetadata( diff --git a/webkit/tools/test_shell/simple_file_system.h b/webkit/tools/test_shell/simple_file_system.h index db13d9c..031ed8b 100644 --- a/webkit/tools/test_shell/simple_file_system.h +++ b/webkit/tools/test_shell/simple_file_system.h @@ -1,6 +1,6 @@ -// Copyright (c) 2010 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. +// Copyright (c) 2010 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. #ifndef WEBKIT_TOOLS_TEST_SHELL_SIMPLE_FILE_SYSTEM_H_ #define WEBKIT_TOOLS_TEST_SHELL_SIMPLE_FILE_SYSTEM_H_ @@ -28,6 +28,8 @@ class SimpleFileSystem : public WebKit::WebFileSystem { WebKit::WebFileSystemCallbacks* callbacks); virtual void remove(const WebKit::WebString& path, WebKit::WebFileSystemCallbacks* callbacks); + virtual void removeRecursively(const WebKit::WebString& path, + WebKit::WebFileSystemCallbacks* callbacks); virtual void readMetadata(const WebKit::WebString& path, WebKit::WebFileSystemCallbacks* callbacks); virtual void createFile(const WebKit::WebString& path, |