diff options
23 files changed, 207 insertions, 471 deletions
diff --git a/chrome/browser/chromeos/drive/async_file_util.cc b/chrome/browser/chromeos/drive/async_file_util.cc index e385033..a2b0e88 100644 --- a/chrome/browser/chromeos/drive/async_file_util.cc +++ b/chrome/browser/chromeos/drive/async_file_util.cc @@ -50,7 +50,8 @@ void RunCreateOrOpenFileCallback( const base::FilePath& file_path, const AsyncFileUtil::CreateOrOpenCallback& callback, base::PlatformFileError error, - base::PlatformFile file) { + base::PlatformFile file, + const base::Closure& close_callback_on_ui_thread) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); // It is necessary to make a closure, which runs on file closing here. @@ -58,10 +59,9 @@ void RunCreateOrOpenFileCallback( // (crbug.com/259184). callback.Run( error, base::PassPlatformFile(&file), - base::Bind(&PostFileSystemCallback, - file_system_getter, - base::Bind(&fileapi_internal::CloseFile, file_path), - base::Closure())); + base::Bind(&google_apis::RunTaskOnThread, + BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI), + close_callback_on_ui_thread)); } // Runs CreateOrOpenFile when the error happens. diff --git a/chrome/browser/chromeos/drive/dummy_file_system.h b/chrome/browser/chromeos/drive/dummy_file_system.h index e9b95b5..92e546f 100644 --- a/chrome/browser/chromeos/drive/dummy_file_system.h +++ b/chrome/browser/chromeos/drive/dummy_file_system.h @@ -28,8 +28,6 @@ class DummyFileSystem : public FileSystemInterface { virtual void OpenFile(const base::FilePath& file_path, OpenMode open_mode, const OpenFileCallback& callback) OVERRIDE {} - virtual void CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback) OVERRIDE {} virtual void Copy(const base::FilePath& src_file_path, const base::FilePath& dest_file_path, const FileOperationCallback& callback) OVERRIDE {} @@ -85,7 +83,7 @@ class DummyFileSystem : public FileSystemInterface { const GetFilesystemMetadataCallback& callback) OVERRIDE {} virtual void MarkCacheFileAsMounted( const base::FilePath& drive_file_path, - const OpenFileCallback& callback) OVERRIDE {} + const MarkMountedCallback& callback) OVERRIDE {} virtual void MarkCacheFileAsUnmounted( const base::FilePath& cache_file_path, const FileOperationCallback& callback) OVERRIDE {} diff --git a/chrome/browser/chromeos/drive/fake_file_system.cc b/chrome/browser/chromeos/drive/fake_file_system.cc index 7f49b9c..234aadb 100644 --- a/chrome/browser/chromeos/drive/fake_file_system.cc +++ b/chrome/browser/chromeos/drive/fake_file_system.cc @@ -74,11 +74,6 @@ void FakeFileSystem::OpenFile(const base::FilePath& file_path, DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); } -void FakeFileSystem::CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); -} - void FakeFileSystem::Copy(const base::FilePath& src_file_path, const base::FilePath& dest_file_path, const FileOperationCallback& callback) { @@ -210,7 +205,7 @@ void FakeFileSystem::GetMetadata( void FakeFileSystem::MarkCacheFileAsMounted( const base::FilePath& drive_file_path, - const OpenFileCallback& callback) { + const MarkMountedCallback& callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); } diff --git a/chrome/browser/chromeos/drive/fake_file_system.h b/chrome/browser/chromeos/drive/fake_file_system.h index 501cf14..c1320ad 100644 --- a/chrome/browser/chromeos/drive/fake_file_system.h +++ b/chrome/browser/chromeos/drive/fake_file_system.h @@ -62,8 +62,6 @@ class FakeFileSystem : public FileSystemInterface { virtual void OpenFile(const base::FilePath& file_path, OpenMode open_mode, const OpenFileCallback& callback) OVERRIDE; - virtual void CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback) OVERRIDE; virtual void Copy(const base::FilePath& src_file_path, const base::FilePath& dest_file_path, const FileOperationCallback& callback) OVERRIDE; @@ -117,7 +115,7 @@ class FakeFileSystem : public FileSystemInterface { const GetFilesystemMetadataCallback& callback) OVERRIDE; virtual void MarkCacheFileAsMounted( const base::FilePath& drive_file_path, - const OpenFileCallback& callback) OVERRIDE; + const MarkMountedCallback& callback) OVERRIDE; virtual void MarkCacheFileAsUnmounted( const base::FilePath& cache_file_path, const FileOperationCallback& callback) OVERRIDE; diff --git a/chrome/browser/chromeos/drive/file_system.cc b/chrome/browser/chromeos/drive/file_system.cc index 4909ccd..b3b22b5 100644 --- a/chrome/browser/chromeos/drive/file_system.cc +++ b/chrome/browser/chromeos/drive/file_system.cc @@ -16,7 +16,6 @@ #include "chrome/browser/chromeos/drive/change_list_processor.h" #include "chrome/browser/chromeos/drive/drive.pb.h" #include "chrome/browser/chromeos/drive/file_cache.h" -#include "chrome/browser/chromeos/drive/file_system/close_file_operation.h" #include "chrome/browser/chromeos/drive/file_system/copy_operation.h" #include "chrome/browser/chromeos/drive/file_system/create_directory_operation.h" #include "chrome/browser/chromeos/drive/file_system/create_file_operation.h" @@ -105,11 +104,6 @@ void FileSystem::Initialize() { SetupChangeListLoader(); file_system::OperationObserver* observer = this; - close_file_operation_.reset( - new file_system::CloseFileOperation(blocking_task_runner_.get(), - observer, - resource_metadata_, - &open_files_)); copy_operation_.reset( new file_system::CopyOperation(blocking_task_runner_.get(), observer, @@ -134,8 +128,7 @@ void FileSystem::Initialize() { scheduler_, resource_metadata_, cache_, - temporary_file_directory_, - &open_files_)); + temporary_file_directory_)); remove_operation_.reset( new file_system::RemoveOperation(blocking_task_runner_.get(), observer, @@ -755,7 +748,7 @@ void FileSystem::GetMetadata( void FileSystem::MarkCacheFileAsMounted( const base::FilePath& drive_file_path, - const OpenFileCallback& callback) { + const MarkMountedCallback& callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(!callback.is_null()); @@ -766,7 +759,7 @@ void FileSystem::MarkCacheFileAsMounted( } void FileSystem::MarkCacheFileAsMountedAfterGetResourceEntry( - const OpenFileCallback& callback, + const MarkMountedCallback& callback, FileError error, scoped_ptr<ResourceEntry> entry) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); @@ -846,14 +839,6 @@ void FileSystem::OpenFile(const base::FilePath& file_path, open_file_operation_->OpenFile(file_path, open_mode, callback); } -void FileSystem::CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(!callback.is_null()); - - close_file_operation_->CloseFile(file_path, callback); -} - void FileSystem::CheckLocalModificationAndRun( scoped_ptr<ResourceEntry> entry, const GetResourceEntryCallback& callback) { diff --git a/chrome/browser/chromeos/drive/file_system.h b/chrome/browser/chromeos/drive/file_system.h index 2b469cc..351f637 100644 --- a/chrome/browser/chromeos/drive/file_system.h +++ b/chrome/browser/chromeos/drive/file_system.h @@ -43,7 +43,6 @@ class SyncClient; } // namespace internal namespace file_system { -class CloseFileOperation; class CopyOperation; class CreateDirectoryOperation; class CreateFileOperation; @@ -95,8 +94,6 @@ class FileSystem : public FileSystemInterface, virtual void OpenFile(const base::FilePath& file_path, OpenMode open_mode, const OpenFileCallback& callback) OVERRIDE; - virtual void CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback) OVERRIDE; virtual void Copy(const base::FilePath& src_file_path, const base::FilePath& dest_file_path, const FileOperationCallback& callback) OVERRIDE; @@ -143,7 +140,7 @@ class FileSystem : public FileSystemInterface, const GetFilesystemMetadataCallback& callback) OVERRIDE; virtual void MarkCacheFileAsMounted( const base::FilePath& drive_file_path, - const OpenFileCallback& callback) OVERRIDE; + const MarkMountedCallback& callback) OVERRIDE; virtual void MarkCacheFileAsUnmounted( const base::FilePath& cache_file_path, const FileOperationCallback& callback) OVERRIDE; @@ -291,7 +288,7 @@ class FileSystem : public FileSystemInterface, // Part of MarkCacheFileAsMounted. Called after GetResourceEntryByPath is // completed. |callback| must not be null. void MarkCacheFileAsMountedAfterGetResourceEntry( - const OpenFileCallback& callback, + const MarkMountedCallback& callback, FileError error, scoped_ptr<ResourceEntry> entry); @@ -313,10 +310,6 @@ class FileSystem : public FileSystemInterface, // True if hosted documents should be hidden. bool hide_hosted_docs_; - // Map from opened file paths to the number how many the file is opened. - // The value should be incremented by OpenFile, and decremented by CloseFile. - std::map<base::FilePath, int> open_files_; - scoped_ptr<PrefChangeRegistrar> pref_registrar_; scoped_ptr<internal::SyncClient> sync_client_; @@ -331,7 +324,6 @@ class FileSystem : public FileSystemInterface, base::FilePath temporary_file_directory_; // Implementation of each file system operation. - scoped_ptr<file_system::CloseFileOperation> close_file_operation_; scoped_ptr<file_system::CopyOperation> copy_operation_; scoped_ptr<file_system::CreateDirectoryOperation> create_directory_operation_; scoped_ptr<file_system::CreateFileOperation> create_file_operation_; diff --git a/chrome/browser/chromeos/drive/file_system/close_file_operation.cc b/chrome/browser/chromeos/drive/file_system/close_file_operation.cc deleted file mode 100644 index dfd2a55..0000000 --- a/chrome/browser/chromeos/drive/file_system/close_file_operation.cc +++ /dev/null @@ -1,83 +0,0 @@ -// Copyright 2013 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 "chrome/browser/chromeos/drive/file_system/close_file_operation.h" - -#include "base/logging.h" -#include "base/message_loop/message_loop_proxy.h" -#include "base/sequenced_task_runner.h" -#include "chrome/browser/chromeos/drive/drive.pb.h" -#include "chrome/browser/chromeos/drive/file_errors.h" -#include "chrome/browser/chromeos/drive/file_system/operation_observer.h" -#include "chrome/browser/chromeos/drive/resource_metadata.h" -#include "content/public/browser/browser_thread.h" - -using content::BrowserThread; - -namespace drive { -namespace file_system { - -CloseFileOperation::CloseFileOperation( - base::SequencedTaskRunner* blocking_task_runner, - OperationObserver* observer, - internal::ResourceMetadata* metadata, - std::map<base::FilePath, int>* open_files) - : blocking_task_runner_(blocking_task_runner), - observer_(observer), - metadata_(metadata), - open_files_(open_files), - weak_ptr_factory_(this) { -} - -CloseFileOperation::~CloseFileOperation() { -} - -void CloseFileOperation::CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(!callback.is_null()); - - if (open_files_->find(file_path) == open_files_->end()) { - // The file is not being opened. - base::MessageLoopProxy::current()->PostTask( - FROM_HERE, base::Bind(callback, FILE_ERROR_NOT_FOUND)); - return; - } - - ResourceEntry* entry = new ResourceEntry; - base::PostTaskAndReplyWithResult( - blocking_task_runner_.get(), - FROM_HERE, - base::Bind(&internal::ResourceMetadata::GetResourceEntryByPath, - base::Unretained(metadata_), file_path, entry), - base::Bind(&CloseFileOperation::CloseFileAfterGetResourceEntry, - weak_ptr_factory_.GetWeakPtr(), - file_path, callback, base::Owned(entry))); -} - -void CloseFileOperation::CloseFileAfterGetResourceEntry( - const base::FilePath& file_path, - const FileOperationCallback& callback, - const ResourceEntry* entry, - FileError error) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(!callback.is_null()); - DCHECK(entry); - - if (error == FILE_ERROR_OK && entry->file_info().is_directory()) - error = FILE_ERROR_NOT_FOUND; - - DCHECK_GT((*open_files_)[file_path], 0); - if (--(*open_files_)[file_path] == 0) { - // All clients closes this file, so notify to upload the file. - open_files_->erase(file_path); - observer_->OnCacheFileUploadNeededByOperation(entry->resource_id()); - } - - // Then invokes the user-supplied callback function. - callback.Run(error); -} - -} // namespace file_system -} // namespace drive diff --git a/chrome/browser/chromeos/drive/file_system/close_file_operation.h b/chrome/browser/chromeos/drive/file_system/close_file_operation.h deleted file mode 100644 index 005c9cf..0000000 --- a/chrome/browser/chromeos/drive/file_system/close_file_operation.h +++ /dev/null @@ -1,70 +0,0 @@ -// Copyright 2013 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 CHROME_BROWSER_CHROMEOS_DRIVE_FILE_SYSTEM_CLOSE_FILE_OPERATION_H_ -#define CHROME_BROWSER_CHROMEOS_DRIVE_FILE_SYSTEM_CLOSE_FILE_OPERATION_H_ - -#include <map> - -#include "base/basictypes.h" -#include "base/memory/ref_counted.h" -#include "base/memory/weak_ptr.h" -#include "chrome/browser/chromeos/drive/file_errors.h" - -namespace base { -class FilePath; -class SequencedTaskRunner; -} // namespace base - -namespace drive { - -class ResourceEntry; - -namespace internal { -class ResourceMetadata; -} // namespace internal - -namespace file_system { - -class OperationObserver; - -class CloseFileOperation { - public: - CloseFileOperation(base::SequencedTaskRunner* blocking_task_runner, - OperationObserver* observer, - internal::ResourceMetadata* metadata, - std::map<base::FilePath, int>* open_files); - ~CloseFileOperation(); - - // Closes the currently opened file |file_path|. - // |callback| must not be null. - void CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback); - - private: - // Part of CloseFile(). Called after ResourceMetadata::GetResourceEntry(). - void CloseFileAfterGetResourceEntry(const base::FilePath& file_path, - const FileOperationCallback& callback, - const ResourceEntry* entry, - FileError error); - - scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_; - OperationObserver* observer_; - internal::ResourceMetadata* metadata_; - - // The map from paths for opened file to the number how many the file is - // opened. The instance is owned by FileSystem and shared with - // OpenFileOperation. - std::map<base::FilePath, int>* open_files_; - - // Note: This should remain the last member so it'll be destroyed and - // invalidate its weak pointers before any other members are destroyed. - base::WeakPtrFactory<CloseFileOperation> weak_ptr_factory_; - DISALLOW_COPY_AND_ASSIGN(CloseFileOperation); -}; - -} // namespace file_system -} // namespace drive - -#endif // CHROME_BROWSER_CHROMEOS_DRIVE_FILE_SYSTEM_CLOSE_FILE_OPERATION_H_ diff --git a/chrome/browser/chromeos/drive/file_system/close_file_operation_unittest.cc b/chrome/browser/chromeos/drive/file_system/close_file_operation_unittest.cc deleted file mode 100644 index 2fe4d88..0000000 --- a/chrome/browser/chromeos/drive/file_system/close_file_operation_unittest.cc +++ /dev/null @@ -1,105 +0,0 @@ -// Copyright 2013 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 "chrome/browser/chromeos/drive/file_system/close_file_operation.h" - -#include <set> - -#include "base/files/file_path.h" -#include "base/memory/scoped_ptr.h" -#include "chrome/browser/chromeos/drive/drive.pb.h" -#include "chrome/browser/chromeos/drive/file_errors.h" -#include "chrome/browser/chromeos/drive/file_system/operation_test_base.h" -#include "chrome/browser/google_apis/test_util.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace drive { -namespace file_system { - -class CloseFileOperationTest : public OperationTestBase { - protected: - virtual void SetUp() { - OperationTestBase::SetUp(); - - operation_.reset(new CloseFileOperation( - blocking_task_runner(), observer(), metadata(), &open_files_)); - } - - std::map<base::FilePath, int> open_files_; - scoped_ptr<CloseFileOperation> operation_; -}; - -TEST_F(CloseFileOperationTest, CloseFile) { - base::FilePath file_in_root(FILE_PATH_LITERAL("drive/root/File 1.txt")); - ResourceEntry src_entry; - ASSERT_EQ(FILE_ERROR_OK, GetLocalResourceEntry(file_in_root, &src_entry)); - - open_files_[file_in_root] = 1; - FileError error = FILE_ERROR_FAILED; - operation_->CloseFile( - file_in_root, - google_apis::test_util::CreateCopyResultCallback(&error)); - test_util::RunBlockingPoolTask(); - - EXPECT_EQ(FILE_ERROR_OK, error); - EXPECT_TRUE(open_files_.empty()); - EXPECT_EQ( - 1U, - observer()->upload_needed_resource_ids().count(src_entry.resource_id())); -} - -TEST_F(CloseFileOperationTest, NotOpenedFile) { - base::FilePath file_in_root(FILE_PATH_LITERAL("drive/root/File 1.txt")); - ResourceEntry src_entry; - ASSERT_EQ(FILE_ERROR_OK, GetLocalResourceEntry(file_in_root, &src_entry)); - - FileError error = FILE_ERROR_FAILED; - operation_->CloseFile( - file_in_root, - google_apis::test_util::CreateCopyResultCallback(&error)); - test_util::RunBlockingPoolTask(); - - // Even if the file is actually exists, NOT_FOUND should be returned if the - // file is not opened. - EXPECT_EQ(FILE_ERROR_NOT_FOUND, error); -} - -TEST_F(CloseFileOperationTest, CloseFileTwice) { - base::FilePath file_in_root(FILE_PATH_LITERAL("drive/root/File 1.txt")); - ResourceEntry src_entry; - ASSERT_EQ(FILE_ERROR_OK, GetLocalResourceEntry(file_in_root, &src_entry)); - - // Set the number of opening clients is two. - open_files_[file_in_root] = 2; - - // The first CloseFile. - FileError error = FILE_ERROR_FAILED; - operation_->CloseFile( - file_in_root, - google_apis::test_util::CreateCopyResultCallback(&error)); - test_util::RunBlockingPoolTask(); - - EXPECT_EQ(FILE_ERROR_OK, error); - EXPECT_EQ(1, open_files_[file_in_root]); - - // There still remains a client opening the file, so it shouldn't be - // uploaded yet. - EXPECT_TRUE(observer()->upload_needed_resource_ids().empty()); - - // The second CloseFile. - operation_->CloseFile( - file_in_root, - google_apis::test_util::CreateCopyResultCallback(&error)); - test_util::RunBlockingPoolTask(); - - EXPECT_EQ(FILE_ERROR_OK, error); - EXPECT_TRUE(open_files_.empty()); - // Here, all the clients close the file, so it should be uploaded then. - EXPECT_EQ( - 1U, - observer()->upload_needed_resource_ids().count(src_entry.resource_id())); -} - -} // namespace file_system -} // namespace drive diff --git a/chrome/browser/chromeos/drive/file_system/open_file_operation.cc b/chrome/browser/chromeos/drive/file_system/open_file_operation.cc index e5a1bfa..4a49830 100644 --- a/chrome/browser/chromeos/drive/file_system/open_file_operation.cc +++ b/chrome/browser/chromeos/drive/file_system/open_file_operation.cc @@ -14,6 +14,7 @@ #include "chrome/browser/chromeos/drive/file_errors.h" #include "chrome/browser/chromeos/drive/file_system/create_file_operation.h" #include "chrome/browser/chromeos/drive/file_system/download_operation.h" +#include "chrome/browser/chromeos/drive/file_system/operation_observer.h" #include "chrome/browser/chromeos/drive/file_system_interface.h" #include "content/public/browser/browser_thread.h" @@ -42,18 +43,16 @@ OpenFileOperation::OpenFileOperation( JobScheduler* scheduler, internal::ResourceMetadata* metadata, internal::FileCache* cache, - const base::FilePath& temporary_file_directory, - std::map<base::FilePath, int>* open_files) + const base::FilePath& temporary_file_directory) : blocking_task_runner_(blocking_task_runner), + observer_(observer), cache_(cache), create_file_operation_(new CreateFileOperation( blocking_task_runner, observer, scheduler, metadata, cache)), download_operation_(new DownloadOperation( blocking_task_runner, observer, scheduler, metadata, cache, temporary_file_directory)), - open_files_(open_files), weak_ptr_factory_(this) { - DCHECK(open_files); } OpenFileOperation::~OpenFileOperation() { @@ -97,7 +96,7 @@ void OpenFileOperation::OpenFileAfterCreateFile( DCHECK(!callback.is_null()); if (error != FILE_ERROR_OK) { - callback.Run(error, base::FilePath()); + callback.Run(error, base::FilePath(), base::Closure()); return; } @@ -108,11 +107,10 @@ void OpenFileOperation::OpenFileAfterCreateFile( google_apis::GetContentCallback(), base::Bind( &OpenFileOperation::OpenFileAfterFileDownloaded, - weak_ptr_factory_.GetWeakPtr(), file_path, callback)); + weak_ptr_factory_.GetWeakPtr(), callback)); } void OpenFileOperation::OpenFileAfterFileDownloaded( - const base::FilePath& file_path, const OpenFileCallback& callback, FileError error, const base::FilePath& local_file_path, @@ -129,7 +127,7 @@ void OpenFileOperation::OpenFileAfterFileDownloaded( } if (error != FILE_ERROR_OK) { - callback.Run(error, base::FilePath()); + callback.Run(error, base::FilePath(), base::Closure()); return; } @@ -146,22 +144,39 @@ void OpenFileOperation::OpenFileAfterFileDownloaded( new_local_file_path), base::Bind(&OpenFileOperation::OpenFileAfterUpdateLocalState, weak_ptr_factory_.GetWeakPtr(), - file_path, + entry->resource_id(), callback, base::Owned(new_local_file_path))); } void OpenFileOperation::OpenFileAfterUpdateLocalState( - const base::FilePath& file_path, + const std::string& resource_id, const OpenFileCallback& callback, const base::FilePath* local_file_path, FileError error) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(!callback.is_null()); - if (error == FILE_ERROR_OK) - ++(*open_files_)[file_path]; - callback.Run(error, *local_file_path); + if (error != FILE_ERROR_OK) { + callback.Run(error, base::FilePath(), base::Closure()); + return; + } + + ++open_files_[resource_id]; + callback.Run(error, *local_file_path, + base::Bind(&OpenFileOperation::CloseFile, + weak_ptr_factory_.GetWeakPtr(), resource_id)); +} + +void OpenFileOperation::CloseFile(const std::string& resource_id) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK_GT(open_files_[resource_id], 0); + + if (--open_files_[resource_id] == 0) { + // All clients closes this file, so notify to upload the file. + open_files_.erase(resource_id); + observer_->OnCacheFileUploadNeededByOperation(resource_id); + } } } // namespace file_system diff --git a/chrome/browser/chromeos/drive/file_system/open_file_operation.h b/chrome/browser/chromeos/drive/file_system/open_file_operation.h index 8aea8ad..f656d2f 100644 --- a/chrome/browser/chromeos/drive/file_system/open_file_operation.h +++ b/chrome/browser/chromeos/drive/file_system/open_file_operation.h @@ -42,8 +42,7 @@ class OpenFileOperation { JobScheduler* scheduler, internal::ResourceMetadata* metadata, internal::FileCache* cache, - const base::FilePath& temporary_file_directory, - std::map<base::FilePath, int>* open_files); + const base::FilePath& temporary_file_directory); ~OpenFileOperation(); // Opens the file at |file_path|. @@ -63,28 +62,30 @@ class OpenFileOperation { FileError error); // Part of OpenFile(). Called after file downloading is completed. - void OpenFileAfterFileDownloaded(const base::FilePath& file_path, - const OpenFileCallback& callback, + void OpenFileAfterFileDownloaded(const OpenFileCallback& callback, FileError error, const base::FilePath& local_file_path, scoped_ptr<ResourceEntry> entry); // Part of OpenFile(). Called after the updating of the local state. - void OpenFileAfterUpdateLocalState(const base::FilePath& file_path, + void OpenFileAfterUpdateLocalState(const std::string& resource_id, const OpenFileCallback& callback, const base::FilePath* local_file_path, FileError error); + // Closes the file with |resource_id|. + void CloseFile(const std::string& resource_id); + scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_; + OperationObserver* observer_; internal::FileCache* cache_; scoped_ptr<CreateFileOperation> create_file_operation_; scoped_ptr<DownloadOperation> download_operation_; - // The map from paths for opened file to the number how many the file is - // opened. The instance is owned by FileSystem and shared with - // CloseFileOperation. - std::map<base::FilePath, int>* open_files_; + // The map from resource id for an opened file to the number how many times + // the file is opened. + std::map<std::string, int> open_files_; // Note: This should remain the last member so it'll be destroyed and // invalidate its weak pointers before any other members are destroyed. diff --git a/chrome/browser/chromeos/drive/file_system/open_file_operation_unittest.cc b/chrome/browser/chromeos/drive/file_system/open_file_operation_unittest.cc index 4e22efe..3628166 100644 --- a/chrome/browser/chromeos/drive/file_system/open_file_operation_unittest.cc +++ b/chrome/browser/chromeos/drive/file_system/open_file_operation_unittest.cc @@ -25,10 +25,9 @@ class OpenFileOperationTest : public OperationTestBase { operation_.reset(new OpenFileOperation( blocking_task_runner(), observer(), scheduler(), metadata(), cache(), - temp_dir(), &open_files_)); + temp_dir())); } - std::map<base::FilePath, int> open_files_; scoped_ptr<OpenFileOperation> operation_; }; @@ -41,10 +40,12 @@ TEST_F(OpenFileOperationTest, OpenExistingFile) { FileError error = FILE_ERROR_FAILED; base::FilePath file_path; + base::Closure close_callback; operation_->OpenFile( file_in_root, OPEN_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback)); test_util::RunBlockingPoolTask(); EXPECT_EQ(FILE_ERROR_OK, error); @@ -53,8 +54,11 @@ TEST_F(OpenFileOperationTest, OpenExistingFile) { ASSERT_TRUE(file_util::GetFileSize(file_path, &local_file_size)); EXPECT_EQ(file_size, local_file_size); - // The file_path should be added into the set. - EXPECT_EQ(1, open_files_[file_in_root]); + ASSERT_FALSE(close_callback.is_null()); + close_callback.Run(); + EXPECT_EQ( + 1U, + observer()->upload_needed_resource_ids().count(src_entry.resource_id())); } TEST_F(OpenFileOperationTest, OpenNonExistingFile) { @@ -63,15 +67,15 @@ TEST_F(OpenFileOperationTest, OpenNonExistingFile) { FileError error = FILE_ERROR_FAILED; base::FilePath file_path; + base::Closure close_callback; operation_->OpenFile( file_in_root, OPEN_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback)); test_util::RunBlockingPoolTask(); EXPECT_EQ(FILE_ERROR_NOT_FOUND, error); - - // The file shouldn't be in the set of opened files. - EXPECT_EQ(0U, open_files_.count(file_in_root)); + EXPECT_TRUE(close_callback.is_null()); } TEST_F(OpenFileOperationTest, CreateExistingFile) { @@ -82,16 +86,16 @@ TEST_F(OpenFileOperationTest, CreateExistingFile) { FileError error = FILE_ERROR_FAILED; base::FilePath file_path; + base::Closure close_callback; operation_->OpenFile( file_in_root, CREATE_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback)); test_util::RunBlockingPoolTask(); EXPECT_EQ(FILE_ERROR_EXISTS, error); - - // The file shouldn't be in the set of opened files. - EXPECT_EQ(0U, open_files_.count(file_in_root)); + EXPECT_TRUE(close_callback.is_null()); } TEST_F(OpenFileOperationTest, CreateNonExistingFile) { @@ -100,10 +104,12 @@ TEST_F(OpenFileOperationTest, CreateNonExistingFile) { FileError error = FILE_ERROR_FAILED; base::FilePath file_path; + base::Closure close_callback; operation_->OpenFile( file_in_root, CREATE_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback)); test_util::RunBlockingPoolTask(); EXPECT_EQ(FILE_ERROR_OK, error); @@ -112,8 +118,11 @@ TEST_F(OpenFileOperationTest, CreateNonExistingFile) { ASSERT_TRUE(file_util::GetFileSize(file_path, &local_file_size)); EXPECT_EQ(0, local_file_size); // Should be an empty file. - // The file_path should be added into the set. - EXPECT_EQ(1, open_files_[file_in_root]); + ASSERT_FALSE(close_callback.is_null()); + close_callback.Run(); + // Here we don't know about the resource id, so just make sure + // OnCacheFileUploadNeededByOperation is called actually. + EXPECT_EQ(1U, observer()->upload_needed_resource_ids().size()); } TEST_F(OpenFileOperationTest, OpenOrCreateExistingFile) { @@ -125,10 +134,12 @@ TEST_F(OpenFileOperationTest, OpenOrCreateExistingFile) { FileError error = FILE_ERROR_FAILED; base::FilePath file_path; + base::Closure close_callback; operation_->OpenFile( file_in_root, OPEN_OR_CREATE_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback)); test_util::RunBlockingPoolTask(); EXPECT_EQ(FILE_ERROR_OK, error); @@ -137,8 +148,11 @@ TEST_F(OpenFileOperationTest, OpenOrCreateExistingFile) { ASSERT_TRUE(file_util::GetFileSize(file_path, &local_file_size)); EXPECT_EQ(file_size, local_file_size); - // The file_path should be added into the set. - EXPECT_EQ(1, open_files_[file_in_root]); + ASSERT_FALSE(close_callback.is_null()); + close_callback.Run(); + EXPECT_EQ( + 1U, + observer()->upload_needed_resource_ids().count(src_entry.resource_id())); } TEST_F(OpenFileOperationTest, OpenOrCreateNonExistingFile) { @@ -147,10 +161,12 @@ TEST_F(OpenFileOperationTest, OpenOrCreateNonExistingFile) { FileError error = FILE_ERROR_FAILED; base::FilePath file_path; + base::Closure close_callback; operation_->OpenFile( file_in_root, OPEN_OR_CREATE_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback)); test_util::RunBlockingPoolTask(); EXPECT_EQ(FILE_ERROR_OK, error); @@ -159,8 +175,11 @@ TEST_F(OpenFileOperationTest, OpenOrCreateNonExistingFile) { ASSERT_TRUE(file_util::GetFileSize(file_path, &local_file_size)); EXPECT_EQ(0, local_file_size); // Should be an empty file. - // The file_path should be added into the set. - EXPECT_EQ(1, open_files_[file_in_root]); + ASSERT_FALSE(close_callback.is_null()); + close_callback.Run(); + // Here we don't know about the resource id, so just make sure + // OnCacheFileUploadNeededByOperation is called actually. + EXPECT_EQ(1U, observer()->upload_needed_resource_ids().size()); } TEST_F(OpenFileOperationTest, OpenFileTwice) { @@ -172,10 +191,12 @@ TEST_F(OpenFileOperationTest, OpenFileTwice) { FileError error = FILE_ERROR_FAILED; base::FilePath file_path; + base::Closure close_callback; operation_->OpenFile( file_in_root, OPEN_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback)); test_util::RunBlockingPoolTask(); EXPECT_EQ(FILE_ERROR_OK, error); @@ -184,15 +205,14 @@ TEST_F(OpenFileOperationTest, OpenFileTwice) { ASSERT_TRUE(file_util::GetFileSize(file_path, &local_file_size)); EXPECT_EQ(file_size, local_file_size); - // The file_path should be added into the set. - EXPECT_EQ(1, open_files_[file_in_root]); - // Open again. error = FILE_ERROR_FAILED; + base::Closure close_callback2; operation_->OpenFile( file_in_root, OPEN_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback2)); test_util::RunBlockingPoolTask(); EXPECT_EQ(FILE_ERROR_OK, error); @@ -200,8 +220,21 @@ TEST_F(OpenFileOperationTest, OpenFileTwice) { ASSERT_TRUE(file_util::GetFileSize(file_path, &local_file_size)); EXPECT_EQ(file_size, local_file_size); - // The file_path should be added into the set. - EXPECT_EQ(2, open_files_[file_in_root]); + ASSERT_FALSE(close_callback.is_null()); + ASSERT_FALSE(close_callback2.is_null()); + + close_callback.Run(); + + // There still remains a client opening the file, so it shouldn't be + // uploaded yet. + EXPECT_TRUE(observer()->upload_needed_resource_ids().empty()); + + close_callback2.Run(); + + // Here, all the clients close the file, so it should be uploaded then. + EXPECT_EQ( + 1U, + observer()->upload_needed_resource_ids().count(src_entry.resource_id())); } } // namespace file_system diff --git a/chrome/browser/chromeos/drive/file_system_interface.h b/chrome/browser/chromeos/drive/file_system_interface.h index 0c2b40b..98ccf11 100644 --- a/chrome/browser/chromeos/drive/file_system_interface.h +++ b/chrome/browser/chromeos/drive/file_system_interface.h @@ -98,8 +98,14 @@ typedef base::Callback<void( // Used to open files from the file system. |file_path| is the path on the local // file system for the opened file. +// If |close_callback| is not null, it must be called when the +// modification to the cache is done. Otherwise, Drive file system does not +// pick up the file for uploading. +// |close_callback| must not be called more than once. typedef base::Callback<void(FileError error, - const base::FilePath& file_path)> OpenFileCallback; + const base::FilePath& file_path, + const base::Closure& close_callback)> + OpenFileCallback; // Used to get available space for the account from Drive. typedef base::Callback<void(FileError error, @@ -110,6 +116,11 @@ typedef base::Callback<void(FileError error, typedef base::Callback<void(const FileSystemMetadata&)> GetFilesystemMetadataCallback; +// Used to mark cached files mounted. +typedef base::Callback<void(FileError error, + const base::FilePath& file_path)> + MarkMountedCallback; + // The mode of opening a file. enum OpenMode { // Open the file if exists. If not, failed. @@ -193,21 +204,11 @@ class FileSystemInterface { // returned to |callback|. After opening the file, both read and write // on the file can be done with normal local file operations. // - // |CloseFile| must be called when the modification to the cache is done. - // Otherwise, Drive file system does not pick up the file for uploading. - // // |callback| must not be null. virtual void OpenFile(const base::FilePath& file_path, OpenMode open_mode, const OpenFileCallback& callback) = 0; - // Closes a file at the virtual path |file_path| on the Drive file system, - // which is opened via OpenFile(). It commits the dirty flag on the cache. - // - // |callback| must not be null. - virtual void CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback) = 0; - // Copies |src_file_path| to |dest_file_path| on the file system. // |src_file_path| can be a hosted document (see limitations below). // |dest_file_path| is expected to be of the same type of |src_file_path| @@ -390,7 +391,7 @@ class FileSystemInterface { // If succeeded, the cached file path will be passed to the |callback|. // |callback| must not be null. virtual void MarkCacheFileAsMounted(const base::FilePath& drive_file_path, - const OpenFileCallback& callback) = 0; + const MarkMountedCallback& callback) = 0; // Marks the cached file as unmounted, and runs |callback| upon completion. // Note that this method expects that the |cached_file_path| is the path diff --git a/chrome/browser/chromeos/drive/file_system_unittest.cc b/chrome/browser/chromeos/drive/file_system_unittest.cc index 9a043c7..a5a7922 100644 --- a/chrome/browser/chromeos/drive/file_system_unittest.cc +++ b/chrome/browser/chromeos/drive/file_system_unittest.cc @@ -741,10 +741,12 @@ TEST_F(FileSystemTest, OpenAndCloseFile) { // Open kFileInRoot ("drive/root/File 1.txt"). FileError error = FILE_ERROR_FAILED; base::FilePath file_path; + base::Closure close_callback; file_system_->OpenFile( kFileInRoot, OPEN_FILE, - google_apis::test_util::CreateCopyResultCallback(&error, &file_path)); + google_apis::test_util::CreateCopyResultCallback( + &error, &file_path, &close_callback)); test_util::RunBlockingPoolTask(); const base::FilePath opened_file_path = file_path; @@ -780,9 +782,8 @@ TEST_F(FileSystemTest, OpenAndCloseFile) { kNewContent)); // Close kFileInRoot ("drive/root/File 1.txt"). - file_system_->CloseFile( - kFileInRoot, - google_apis::test_util::CreateCopyResultCallback(&error)); + ASSERT_FALSE(close_callback.is_null()); + close_callback.Run(); test_util::RunBlockingPoolTask(); // Verify that the file was properly closed. @@ -805,17 +806,6 @@ TEST_F(FileSystemTest, OpenAndCloseFile) { ASSERT_EQ(2u, mock_directory_observer_->changed_directories().size()); EXPECT_EQ(base::FilePath(FILE_PATH_LITERAL("drive/root")), mock_directory_observer_->changed_directories()[1]); - - // Try to close the same file twice. - file_system_->CloseFile( - kFileInRoot, - google_apis::test_util::CreateCopyResultCallback(&error)); - test_util::RunBlockingPoolTask(); - - // It must fail. - EXPECT_EQ(FILE_ERROR_NOT_FOUND, error); - // There should be no new directory change. - ASSERT_EQ(2u, mock_directory_observer_->changed_directories().size()); } TEST_F(FileSystemTest, MarkCacheFileAsMountedAndUnmounted) { diff --git a/chrome/browser/chromeos/drive/file_write_helper.cc b/chrome/browser/chromeos/drive/file_write_helper.cc index c48262c..b5623ed 100644 --- a/chrome/browser/chromeos/drive/file_write_helper.cc +++ b/chrome/browser/chromeos/drive/file_write_helper.cc @@ -4,26 +4,16 @@ #include "chrome/browser/chromeos/drive/file_write_helper.h" +#include "base/bind.h" +#include "base/callback.h" #include "base/threading/sequenced_worker_pool.h" +#include "chrome/browser/chromeos/drive/file_system_interface.h" #include "content/public/browser/browser_thread.h" using content::BrowserThread; namespace drive { -namespace { - -// Emits debug log when FileSystem::CloseFile() is complete. -void EmitDebugLogForCloseFile(const base::FilePath& file_path, - FileError file_error) { - if (file_error != FILE_ERROR_OK) { - LOG(WARNING) << "CloseFile failed: " << file_path.AsUTF8Unsafe() << ": " - << file_error; - } -} - -} // namespace - FileWriteHelper::FileWriteHelper(FileSystemInterface* file_system) : file_system_(file_system), weak_ptr_factory_(this) { @@ -52,7 +42,8 @@ void FileWriteHelper::PrepareWritableFileAndRunAfterOpenFile( const base::FilePath& file_path, const OpenFileCallback& callback, FileError error, - const base::FilePath& local_cache_path) { + const base::FilePath& local_cache_path, + const base::Closure& close_callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(!callback.is_null()); @@ -66,16 +57,7 @@ void FileWriteHelper::PrepareWritableFileAndRunAfterOpenFile( content::BrowserThread::GetBlockingPool()->PostTaskAndReply( FROM_HERE, base::Bind(callback, FILE_ERROR_OK, local_cache_path), - base::Bind(&FileWriteHelper::PrepareWritableFileAndRunAfterCallback, - weak_ptr_factory_.GetWeakPtr(), - file_path)); -} - -void FileWriteHelper::PrepareWritableFileAndRunAfterCallback( - const base::FilePath& file_path) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - file_system_->CloseFile(file_path, - base::Bind(&EmitDebugLogForCloseFile, file_path)); + close_callback); } } // namespace drive diff --git a/chrome/browser/chromeos/drive/file_write_helper.h b/chrome/browser/chromeos/drive/file_write_helper.h index 6b60a69..8a440ef 100644 --- a/chrome/browser/chromeos/drive/file_write_helper.h +++ b/chrome/browser/chromeos/drive/file_write_helper.h @@ -5,9 +5,9 @@ #ifndef CHROME_BROWSER_CHROMEOS_DRIVE_FILE_WRITE_HELPER_H_ #define CHROME_BROWSER_CHROMEOS_DRIVE_FILE_WRITE_HELPER_H_ +#include "base/callback_forward.h" #include "base/memory/weak_ptr.h" #include "chrome/browser/chromeos/drive/file_errors.h" -#include "chrome/browser/chromeos/drive/file_system_interface.h" namespace base { class FilePath; @@ -15,10 +15,17 @@ class FilePath; namespace drive { +class FileSystemInterface; + // This class provides higher level operations for writing to Drive files over // FileSystemInterface. class FileWriteHelper { public: + // Callback for PrepareWritableFileAndRun. + typedef base::Callback<void(FileError error, + const base::FilePath& file_path)> + OpenFileCallback; + explicit FileWriteHelper(FileSystemInterface* file_system); ~FileWriteHelper(); @@ -34,13 +41,13 @@ class FileWriteHelper { private: // Part of PrepareWritableFilePathAndRun(). It tries CreateFile for the case // file does not exist yet, does OpenFile to download and mark the file as - // dirty, runs |callback|, and finally calls CloseFile. + // dirty, runs |callback|, and finally closes the file. void PrepareWritableFileAndRunAfterOpenFile( const base::FilePath& file_path, const OpenFileCallback& callback, FileError result, - const base::FilePath& local_cache_path); - void PrepareWritableFileAndRunAfterCallback(const base::FilePath& file_path); + const base::FilePath& local_cache_path, + const base::Closure& close_callback); FileSystemInterface* file_system_; // Owned by DriveIntegrationService. diff --git a/chrome/browser/chromeos/drive/file_write_helper_unittest.cc b/chrome/browser/chromeos/drive/file_write_helper_unittest.cc index af7bf9d..4270349 100644 --- a/chrome/browser/chromeos/drive/file_write_helper_unittest.cc +++ b/chrome/browser/chromeos/drive/file_write_helper_unittest.cc @@ -23,6 +23,11 @@ const base::FilePath::CharType kLocalPath[] = class TestFileSystem : public DummyFileSystem { public: + TestFileSystem() : num_closed_(0) { + } + + int num_closed() const { return num_closed_; } + // Mimics OpenFile. It fails if the |file_path| points to a hosted document. virtual void OpenFile(const base::FilePath& file_path, OpenMode open_mode, @@ -31,17 +36,23 @@ class TestFileSystem : public DummyFileSystem { // Emulate a case of opening a hosted document. if (file_path == base::FilePath(kInvalidPath)) { - callback.Run(FILE_ERROR_INVALID_OPERATION, base::FilePath()); + callback.Run(FILE_ERROR_INVALID_OPERATION, base::FilePath(), + base::Closure()); return; } - callback.Run(FILE_ERROR_OK, base::FilePath(kLocalPath)); + callback.Run(FILE_ERROR_OK, base::FilePath(kLocalPath), + base::Bind(&TestFileSystem::CloseFile, + base::Unretained(this))); } - virtual void CloseFile(const base::FilePath& file_path, - const FileOperationCallback& callback) OVERRIDE { - callback.Run(FILE_ERROR_OK); + private: + + void CloseFile() { + ++num_closed_; } + + int num_closed_; }; } // namespace @@ -69,6 +80,9 @@ TEST_F(FileWriteHelperTest, PrepareFileForWritingSuccess) { EXPECT_EQ(FILE_ERROR_OK, error); EXPECT_EQ(kLocalPath, path.value()); + + // Make sure that the file is actually closed. + EXPECT_EQ(1, test_file_system_->num_closed()); } TEST_F(FileWriteHelperTest, PrepareFileForWritingCreateFail) { diff --git a/chrome/browser/chromeos/drive/fileapi_worker.cc b/chrome/browser/chromeos/drive/fileapi_worker.cc index 9f59917..3b952e9 100644 --- a/chrome/browser/chromeos/drive/fileapi_worker.cc +++ b/chrome/browser/chromeos/drive/fileapi_worker.cc @@ -136,28 +136,32 @@ void RunCreateSnapshotFileCallback(const CreateSnapshotFileCallback& callback, void RunCreateWritableSnapshotFileCallback( const CreateWritableSnapshotFileCallback& callback, FileError error, - const base::FilePath& local_path) { + const base::FilePath& local_path, + const base::Closure& close_callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - callback.Run(FileErrorToPlatformError(error), local_path); + callback.Run(FileErrorToPlatformError(error), local_path, close_callback); } // Runs |callback| with |error| and |platform_file|. void RunOpenFileCallback(const OpenFileCallback& callback, + const base::Closure& close_callback, base::PlatformFileError* error, base::PlatformFile platform_file) { - callback.Run(*error, platform_file); + callback.Run(*error, platform_file, close_callback); } // Part of OpenFile(). Called after FileSystem::OpenFile(). void OpenFileAfterFileSystemOpenFile(int file_flags, const OpenFileCallback& callback, FileError error, - const base::FilePath& local_path) { + const base::FilePath& local_path, + const base::Closure& close_callback) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); if (error != FILE_ERROR_OK) { callback.Run(FileErrorToPlatformError(error), - base::kInvalidPlatformFileValue); + base::kInvalidPlatformFileValue, + base::Closure()); return; } @@ -183,16 +187,11 @@ void OpenFileAfterFileSystemOpenFile(int file_flags, BrowserThread::GetBlockingPool(), FROM_HERE, base::Bind(&base::CreatePlatformFile, local_path, file_flags, static_cast<bool*>(NULL), result), - base::Bind(&RunOpenFileCallback, callback, base::Owned(result))); + base::Bind(&RunOpenFileCallback, + callback, close_callback, base::Owned(result))); DCHECK(posted); } -// Emits debug log when FileSystem::CloseFile() is complete. -void EmitDebugLogForCloseFile(const base::FilePath& local_path, - FileError file_error) { - DVLOG(1) << "Closed: " << local_path.AsUTF8Unsafe() << ": " << file_error; -} - } // namespace void RunFileSystemCallback( @@ -336,7 +335,8 @@ void OpenFile(const base::FilePath& file_path, FROM_HERE, base::Bind(callback, base::PLATFORM_FILE_ERROR_FAILED, - base::kInvalidPlatformFileValue)); + base::kInvalidPlatformFileValue, + base::Closure())); return; } @@ -345,13 +345,6 @@ void OpenFile(const base::FilePath& file_path, base::Bind(&OpenFileAfterFileSystemOpenFile, file_flags, callback)); } -void CloseFile(const base::FilePath& file_path, - FileSystemInterface* file_system) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - file_system->CloseFile(file_path, - base::Bind(&EmitDebugLogForCloseFile, file_path)); -} - void TouchFile(const base::FilePath& file_path, const base::Time& last_access_time, const base::Time& last_modified_time, diff --git a/chrome/browser/chromeos/drive/fileapi_worker.h b/chrome/browser/chromeos/drive/fileapi_worker.h index d26c65e..bef86f9 100644 --- a/chrome/browser/chromeos/drive/fileapi_worker.h +++ b/chrome/browser/chromeos/drive/fileapi_worker.h @@ -61,23 +61,25 @@ typedef base::Callback< CreateSnapshotFileCallback; typedef base::Callback< void(base::PlatformFileError result, - const base::FilePath& snapshot_file_path)> + const base::FilePath& snapshot_file_path, + const base::Closure& close_callback)> CreateWritableSnapshotFileCallback; typedef base::Callback< void(base::PlatformFileError result, - base::PlatformFile platform_file)> OpenFileCallback; + base::PlatformFile platform_file, + const base::Closure& close_callback)> OpenFileCallback; // Runs |file_system_getter| to obtain the instance of FileSystemInstance, // and then runs |callback| with it. -// If |file_system_getter| returns NULL, runs |on_error_callback| instead. +// If |file_system_getter| returns NULL, runs |error_callback| instead. // This function must be called on UI thread. // |file_system_getter| and |callback| must not be null, but -// |on_error_callback| can be null (if no operation is necessary for error +// |error_callback| can be null (if no operation is necessary for error // case). void RunFileSystemCallback( const FileSystemGetter& file_system_getter, const base::Callback<void(FileSystemInterface*)>& callback, - const base::Closure& on_error_callback); + const base::Closure& error_callback); // Returns the metadata info of the file at |file_path|. // Called from FileSystemProxy::GetFileInfo(). @@ -148,7 +150,7 @@ void CreateSnapshotFile(const base::FilePath& file_path, FileSystemInterface* file_system); // Creates a writable snapshot for the file at |file_path|. -// After writing operation is done, CloseFile is needed to be called. +// After writing operation is done, |close_callback| must be called. void CreateWritableSnapshotFile( const base::FilePath& file_path, const CreateWritableSnapshotFileCallback& callback, @@ -161,12 +163,6 @@ void OpenFile(const base::FilePath& file_path, const OpenFileCallback& callback, FileSystemInterface* file_system); -// Closes the file at |file_path|. -// Called from FileSystemProxy::NotifyCloseFile and -// FileSystemProxy::CloseWRitableSnapshotFile. -void CloseFile(const base::FilePath& file_path, - FileSystemInterface* file_system); - // Changes timestamp of the file at |file_path| to |last_access_time| and // |last_modified_time|. Called from FileSystemProxy::TouchFile(). void TouchFile(const base::FilePath& file_path, diff --git a/chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.cc b/chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.cc index 94e8e80..3363340 100644 --- a/chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.cc +++ b/chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.cc @@ -37,23 +37,8 @@ void CreateWritableSnapshotFile( base::Bind(&fileapi_internal::CreateWritableSnapshotFile, drive_path, google_apis::CreateRelayCallback(callback)), google_apis::CreateRelayCallback(base::Bind( - callback, base::PLATFORM_FILE_ERROR_FAILED, base::FilePath())))); -} - -// Closes the writable snapshot file opened by CreateWritableSnapshotFile. -// TODO(hidehiko): Get rid of this function. crbug.com/259184. -void CloseFile( - const WebkitFileStreamWriterImpl::FileSystemGetter& file_system_getter, - const base::FilePath& drive_path) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - - BrowserThread::PostTask( - BrowserThread::UI, - FROM_HERE, - base::Bind(&fileapi_internal::RunFileSystemCallback, - file_system_getter, - base::Bind(&fileapi_internal::CloseFile, drive_path), - base::Closure())); + callback, base::PLATFORM_FILE_ERROR_FAILED, base::FilePath(), + base::Closure())))); } } // namespace @@ -75,7 +60,10 @@ WebkitFileStreamWriterImpl::~WebkitFileStreamWriterImpl() { // If the file is opened, close it at destructor. // It is necessary to close the local file in advance. local_file_writer_.reset(); - CloseFile(file_system_getter_, file_path_); + DCHECK(!close_callback_on_ui_thread_.is_null()); + BrowserThread::PostTask(BrowserThread::UI, + FROM_HERE, + close_callback_on_ui_thread_); } } @@ -146,7 +134,8 @@ void WebkitFileStreamWriterImpl::WriteAfterCreateWritableSnapshotFile( net::IOBuffer* buf, int buf_len, base::PlatformFileError open_result, - const base::FilePath& local_path) { + const base::FilePath& local_path, + const base::Closure& close_callback_on_ui_thread) { DCHECK(!local_file_writer_); if (!pending_cancel_callback_.is_null()) { @@ -156,8 +145,10 @@ void WebkitFileStreamWriterImpl::WriteAfterCreateWritableSnapshotFile( if (open_result == base::PLATFORM_FILE_OK) { // Here the file is internally created. To revert the operation, close // the file. - DCHECK(!local_path.empty()); - CloseFile(file_system_getter_, file_path_); + DCHECK(!close_callback_on_ui_thread.is_null()); + BrowserThread::PostTask(BrowserThread::UI, + FROM_HERE, + close_callback_on_ui_thread); } base::ResetAndReturn(&pending_cancel_callback_).Run(net::OK); @@ -169,10 +160,14 @@ void WebkitFileStreamWriterImpl::WriteAfterCreateWritableSnapshotFile( const net::CompletionCallback callback = base::ResetAndReturn(&pending_write_callback_); if (open_result != base::PLATFORM_FILE_OK) { + DCHECK(close_callback_on_ui_thread.is_null()); callback.Run(net::PlatformFileErrorToNetError(open_result)); return; } + // Keep |close_callback| to close the file when the stream is destructed. + DCHECK(!close_callback_on_ui_thread.is_null()); + close_callback_on_ui_thread_ = close_callback_on_ui_thread; local_file_writer_.reset(new fileapi::LocalFileStreamWriter( file_task_runner_.get(), local_path, offset_)); int result = local_file_writer_->Write(buf, buf_len, callback); diff --git a/chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.h b/chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.h index 1ddadba..e72a692 100644 --- a/chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.h +++ b/chrome/browser/chromeos/drive/webkit_file_stream_writer_impl.h @@ -58,7 +58,8 @@ class WebkitFileStreamWriterImpl : public fileapi::FileStreamWriter { net::IOBuffer* buf, int buf_len, base::PlatformFileError open_result, - const base::FilePath& local_path); + const base::FilePath& local_path, + const base::Closure& close_callback_on_ui_thread); FileSystemGetter file_system_getter_; scoped_refptr<base::TaskRunner> file_task_runner_; @@ -66,6 +67,7 @@ class WebkitFileStreamWriterImpl : public fileapi::FileStreamWriter { const int64 offset_; scoped_ptr<fileapi::FileStreamWriter> local_file_writer_; + base::Closure close_callback_on_ui_thread_; net::CompletionCallback pending_write_callback_; net::CompletionCallback pending_cancel_callback_; diff --git a/chrome/chrome_browser_chromeos.gypi b/chrome/chrome_browser_chromeos.gypi index 08024d6..3e713d7 100644 --- a/chrome/chrome_browser_chromeos.gypi +++ b/chrome/chrome_browser_chromeos.gypi @@ -249,8 +249,6 @@ 'browser/chromeos/drive/file_errors.h', 'browser/chromeos/drive/file_system.cc', 'browser/chromeos/drive/file_system.h', - 'browser/chromeos/drive/file_system/close_file_operation.cc', - 'browser/chromeos/drive/file_system/close_file_operation.h', 'browser/chromeos/drive/file_system/copy_operation.cc', 'browser/chromeos/drive/file_system/copy_operation.h', 'browser/chromeos/drive/file_system/create_directory_operation.cc', diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 018e8be..aa457f2 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -600,7 +600,6 @@ 'browser/chromeos/drive/file_cache_metadata_unittest.cc', 'browser/chromeos/drive/file_cache_unittest.cc', 'browser/chromeos/drive/file_change_unittest.cc', - 'browser/chromeos/drive/file_system/close_file_operation_unittest.cc', 'browser/chromeos/drive/file_system/copy_operation_unittest.cc', 'browser/chromeos/drive/file_system/create_directory_operation_unittest.cc', 'browser/chromeos/drive/file_system/create_file_operation_unittest.cc', |