diff options
author | mtomasz <mtomasz@chromium.org> | 2014-11-09 18:02:04 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-11-10 02:02:28 +0000 |
commit | 4c9bba032dd4eb99cec715caae9225a9c93dde00 (patch) | |
tree | b0815343e3b683c6d70101154ab8c51eb118d41f | |
parent | ff48ecc9a42f24889384503488208db7b495f549 (diff) | |
download | chromium_src-4c9bba032dd4eb99cec715caae9225a9c93dde00.zip chromium_src-4c9bba032dd4eb99cec715caae9225a9c93dde00.tar.gz chromium_src-4c9bba032dd4eb99cec715caae9225a9c93dde00.tar.bz2 |
[fsp] Pass more detailed errors to the providing extension.
Before, many methods were returning a simple bool, which was later converted
to a generic SECURITY error code (if false).
This patch converts such bools to base::File::Error, so we can convey the
specific error code.
Note, that base::File::Error is used to pass all error codes within the File
System API implementation, even if it's not really related to files. We may
want to introduce our own type in the future.
TEST=unit_tests, browser_tests: *FileSystemProvider*
BUG=248427
Review URL: https://codereview.chromium.org/703123003
Cr-Commit-Position: refs/heads/master@{#303413}
23 files changed, 292 insertions, 243 deletions
diff --git a/chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc b/chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc index aa43ff7..c315d40 100644 --- a/chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc +++ b/chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc @@ -77,13 +77,13 @@ bool FileSystemProviderMountFunction::RunSync() { // It's an error if the file system Id is empty. if (params->options.file_system_id.empty()) { - SetError(FileErrorToString(base::File::FILE_ERROR_SECURITY)); + SetError(FileErrorToString(base::File::FILE_ERROR_INVALID_OPERATION)); return false; } // It's an error if the display name is empty. if (params->options.display_name.empty()) { - SetError(FileErrorToString(base::File::FILE_ERROR_SECURITY)); + SetError(FileErrorToString(base::File::FILE_ERROR_INVALID_OPERATION)); return false; } @@ -96,9 +96,10 @@ bool FileSystemProviderMountFunction::RunSync() { options.writable = params->options.writable; options.supports_notify_tag = params->options.supports_notify_tag; - // TODO(mtomasz): Pass more detailed errors, rather than just a bool. - if (!service->MountFileSystem(extension_id(), options)) { - SetError(FileErrorToString(base::File::FILE_ERROR_SECURITY)); + const base::File::Error result = + service->MountFileSystem(extension_id(), options); + if (result != base::File::FILE_OK) { + SetError(FileErrorToString(result)); return false; } @@ -113,11 +114,11 @@ bool FileSystemProviderUnmountFunction::RunSync() { Service* const service = Service::Get(GetProfile()); DCHECK(service); - if (!service->UnmountFileSystem(extension_id(), - params->options.file_system_id, - Service::UNMOUNT_REASON_USER)) { - // TODO(mtomasz): Pass more detailed errors, rather than just a bool. - SetError(FileErrorToString(base::File::FILE_ERROR_SECURITY)); + const base::File::Error result = + service->UnmountFileSystem(extension_id(), params->options.file_system_id, + Service::UNMOUNT_REASON_USER); + if (result != base::File::FILE_OK) { + SetError(FileErrorToString(result)); return false; } @@ -171,7 +172,7 @@ bool FileSystemProviderGetAllFunction::RunSync() { return true; } -bool FileSystemProviderNotifyFunction::RunSync() { +bool FileSystemProviderNotifyFunction::RunAsync() { using api::file_system_provider::Notify::Params; scoped_ptr<Params> params(Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params); @@ -183,25 +184,33 @@ bool FileSystemProviderNotifyFunction::RunSync() { service->GetProvidedFileSystem(extension_id(), params->options.file_system_id); if (!file_system) { - SetError(FileErrorToString(base::File::FILE_ERROR_SECURITY)); + SetError(FileErrorToString(base::File::FILE_ERROR_NOT_FOUND)); return false; } - if (!file_system->Notify( - base::FilePath::FromUTF8Unsafe(params->options.observed_path), - params->options.recursive, - ParseChangeType(params->options.change_type), - params->options.changes.get() - ? ParseChanges(*params->options.changes.get()) - : make_scoped_ptr(new ProvidedFileSystemObserver::Changes), - params->options.tag.get() ? *params->options.tag.get() : "")) { - SetError(FileErrorToString(base::File::FILE_ERROR_SECURITY)); - return false; - } + file_system->Notify( + base::FilePath::FromUTF8Unsafe(params->options.observed_path), + params->options.recursive, ParseChangeType(params->options.change_type), + params->options.changes.get() + ? ParseChanges(*params->options.changes.get()) + : make_scoped_ptr(new ProvidedFileSystemObserver::Changes), + params->options.tag.get() ? *params->options.tag.get() : "", + base::Bind(&FileSystemProviderNotifyFunction::OnNotifyCompleted, this)); return true; } +void FileSystemProviderNotifyFunction::OnNotifyCompleted( + base::File::Error result) { + if (result != base::File::FILE_OK) { + SetError(FileErrorToString(result)); + SendResponse(false); + return; + } + + SendResponse(true); +} + bool FileSystemProviderInternalUnmountRequestedSuccessFunction::RunWhenValid() { using api::file_system_provider_internal::UnmountRequestedSuccess::Params; scoped_ptr<Params> params(Params::Create(*args_)); diff --git a/chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.h b/chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.h index 5f5c8ad..23c783a 100644 --- a/chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.h +++ b/chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.h @@ -40,14 +40,18 @@ class FileSystemProviderGetAllFunction : public ChromeSyncExtensionFunction { virtual bool RunSync() override; }; -class FileSystemProviderNotifyFunction : public ChromeSyncExtensionFunction { +class FileSystemProviderNotifyFunction : public ChromeAsyncExtensionFunction { public: DECLARE_EXTENSION_FUNCTION("fileSystemProvider.notify", FILESYSTEMPROVIDER_NOTIFY) protected: virtual ~FileSystemProviderNotifyFunction() {} - virtual bool RunSync() override; + virtual bool RunAsync() override; + + private: + // Called when notifying is completed. + void OnNotifyCompleted(base::File::Error result); }; class FileSystemProviderInternalUnmountRequestedSuccessFunction diff --git a/chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc b/chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc index 3b24d086..3ad1db8 100644 --- a/chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc +++ b/chrome/browser/chromeos/extensions/file_system_provider/provider_function.cc @@ -126,9 +126,10 @@ FileSystemProviderInternalFunction::FileSystemProviderInternalFunction() bool FileSystemProviderInternalFunction::RejectRequest( scoped_ptr<chromeos::file_system_provider::RequestValue> value, base::File::Error error) { - if (!request_manager_->RejectRequest(request_id_, value.Pass(), error)) { - SetError(FileErrorToString(base::File::FILE_ERROR_SECURITY)); - SendResponse(false); + const base::File::Error result = + request_manager_->RejectRequest(request_id_, value.Pass(), error); + if (result != base::File::FILE_OK) { + SetError(FileErrorToString(result)); return false; } @@ -138,9 +139,10 @@ bool FileSystemProviderInternalFunction::RejectRequest( bool FileSystemProviderInternalFunction::FulfillRequest( scoped_ptr<RequestValue> value, bool has_more) { - if (!request_manager_->FulfillRequest(request_id_, value.Pass(), has_more)) { - SetError(FileErrorToString(base::File::FILE_ERROR_SECURITY)); - SendResponse(false); + const base::File::Error result = + request_manager_->FulfillRequest(request_id_, value.Pass(), has_more); + if (result != base::File::FILE_OK) { + SetError(FileErrorToString(result)); return false; } @@ -150,10 +152,9 @@ bool FileSystemProviderInternalFunction::FulfillRequest( bool FileSystemProviderInternalFunction::RunSync() { DCHECK(args_); if (!Parse()) - return true; + return false; - SendResponse(RunWhenValid()); - return true; + return RunWhenValid(); } bool FileSystemProviderInternalFunction::Parse() { @@ -162,13 +163,11 @@ bool FileSystemProviderInternalFunction::Parse() { if (!args_->GetString(0, &file_system_id) || !args_->GetInteger(1, &request_id_)) { bad_message_ = true; - SendResponse(false); return false; } Service* service = Service::Get(GetProfile()); if (!service) { - SendResponse(false); return false; } @@ -176,7 +175,6 @@ bool FileSystemProviderInternalFunction::Parse() { service->GetProvidedFileSystem(extension_id(), file_system_id); if (!file_system) { SetError(FileErrorToString(base::File::FILE_ERROR_NOT_FOUND)); - SendResponse(false); return false; } diff --git a/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc b/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc index 3f415e26..cc676f8 100644 --- a/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc +++ b/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.cc @@ -367,14 +367,15 @@ void FakeProvidedFileSystem::RemoveObserver( observers_.RemoveObserver(observer); } -bool FakeProvidedFileSystem::Notify( +void FakeProvidedFileSystem::Notify( const base::FilePath& entry_path, bool recursive, storage::WatcherManager::ChangeType change_type, scoped_ptr<ProvidedFileSystemObserver::Changes> changes, - const std::string& tag) { + const std::string& tag, + const storage::AsyncFileUtil::StatusCallback& callback) { NOTREACHED(); - return false; + callback.Run(base::File::FILE_ERROR_SECURITY); } ProvidedFileSystemInterface* FakeProvidedFileSystem::Create( diff --git a/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h b/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h index 37f4bde..63ebc5d 100644 --- a/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h +++ b/chrome/browser/chromeos/file_system_provider/fake_provided_file_system.h @@ -146,11 +146,13 @@ class FakeProvidedFileSystem : public ProvidedFileSystemInterface { virtual Watchers* GetWatchers() override; virtual void AddObserver(ProvidedFileSystemObserver* observer) override; virtual void RemoveObserver(ProvidedFileSystemObserver* observer) override; - virtual bool Notify(const base::FilePath& entry_path, - bool recursive, - storage::WatcherManager::ChangeType change_type, - scoped_ptr<ProvidedFileSystemObserver::Changes> changes, - const std::string& tag) override; + virtual void Notify( + const base::FilePath& entry_path, + bool recursive, + storage::WatcherManager::ChangeType change_type, + scoped_ptr<ProvidedFileSystemObserver::Changes> changes, + const std::string& tag, + const storage::AsyncFileUtil::StatusCallback& callback) override; virtual base::WeakPtr<ProvidedFileSystemInterface> GetWeakPtr() override; // Factory callback, to be used in Service::SetFileSystemFactory(). The diff --git a/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader_unittest.cc b/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader_unittest.cc index d0bfc6d..dcf30d5 100644 --- a/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader_unittest.cc +++ b/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader_unittest.cc @@ -96,9 +96,9 @@ class FileSystemProviderFileStreamReader : public testing::Test { service->SetFileSystemFactoryForTesting( base::Bind(&FakeProvidedFileSystem::Create)); - const bool result = service->MountFileSystem( + const base::File::Error result = service->MountFileSystem( kExtensionId, MountOptions(kFileSystemId, "Testing File System")); - ASSERT_TRUE(result); + ASSERT_EQ(base::File::FILE_OK, result); FakeProvidedFileSystem* provided_file_system = static_cast<FakeProvidedFileSystem*>( service->GetProvidedFileSystem(kExtensionId, kFileSystemId)); diff --git a/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_writer_unittest.cc b/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_writer_unittest.cc index 8da0ba5..8d3282c 100644 --- a/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_writer_unittest.cc +++ b/chrome/browser/chromeos/file_system_provider/fileapi/file_stream_writer_unittest.cc @@ -80,9 +80,9 @@ class FileSystemProviderFileStreamWriter : public testing::Test { service->SetFileSystemFactoryForTesting( base::Bind(&FakeProvidedFileSystem::Create)); - const bool result = service->MountFileSystem( + const base::File::Error result = service->MountFileSystem( kExtensionId, MountOptions(kFileSystemId, "Testing File System")); - ASSERT_TRUE(result); + ASSERT_EQ(base::File::FILE_OK, result); provided_file_system_ = static_cast<FakeProvidedFileSystem*>( service->GetProvidedFileSystem(kExtensionId, kFileSystemId)); ASSERT_TRUE(provided_file_system_); diff --git a/chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util_unittest.cc b/chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util_unittest.cc index 06024b2..6e38f5f 100644 --- a/chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util_unittest.cc +++ b/chrome/browser/chromeos/file_system_provider/fileapi/provider_async_file_util_unittest.cc @@ -135,9 +135,9 @@ class FileSystemProviderProviderAsyncFileUtilTest : public testing::Test { service->SetFileSystemFactoryForTesting( base::Bind(&FakeProvidedFileSystem::Create)); - const bool result = service->MountFileSystem( + const base::File::Error result = service->MountFileSystem( kExtensionId, MountOptions(kFileSystemId, "Testing File System")); - ASSERT_TRUE(result); + ASSERT_EQ(base::File::FILE_OK, result); const ProvidedFileSystemInfo& file_system_info = service->GetProvidedFileSystem(kExtensionId, kFileSystemId) ->GetFileSystemInfo(); diff --git a/chrome/browser/chromeos/file_system_provider/mount_path_util_unittest.cc b/chrome/browser/chromeos/file_system_provider/mount_path_util_unittest.cc index cc2bf5d..e14f47e 100644 --- a/chrome/browser/chromeos/file_system_provider/mount_path_util_unittest.cc +++ b/chrome/browser/chromeos/file_system_provider/mount_path_util_unittest.cc @@ -127,9 +127,10 @@ TEST_F(FileSystemProviderMountPathUtilTest, IsFileSystemProviderLocalPath) { } TEST_F(FileSystemProviderMountPathUtilTest, Parser) { - const bool result = file_system_provider_service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName)); - ASSERT_TRUE(result); + const base::File::Error result = + file_system_provider_service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName)); + ASSERT_EQ(base::File::FILE_OK, result); const ProvidedFileSystemInfo file_system_info = file_system_provider_service_->GetProvidedFileSystem(kExtensionId, kFileSystemId) @@ -151,9 +152,10 @@ TEST_F(FileSystemProviderMountPathUtilTest, Parser) { } TEST_F(FileSystemProviderMountPathUtilTest, Parser_RootPath) { - const bool result = file_system_provider_service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName)); - ASSERT_TRUE(result); + const base::File::Error result = + file_system_provider_service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName)); + ASSERT_EQ(base::File::FILE_OK, result); const ProvidedFileSystemInfo file_system_info = file_system_provider_service_->GetProvidedFileSystem(kExtensionId, kFileSystemId) @@ -191,9 +193,10 @@ TEST_F(FileSystemProviderMountPathUtilTest, Parser_WrongUrl) { } TEST_F(FileSystemProviderMountPathUtilTest, Parser_IsolatedURL) { - const bool result = file_system_provider_service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName)); - ASSERT_TRUE(result); + const base::File::Error result = + file_system_provider_service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName)); + ASSERT_EQ(base::File::FILE_OK, result); const ProvidedFileSystemInfo file_system_info = file_system_provider_service_->GetProvidedFileSystem(kExtensionId, kFileSystemId) @@ -237,9 +240,10 @@ TEST_F(FileSystemProviderMountPathUtilTest, Parser_IsolatedURL) { } TEST_F(FileSystemProviderMountPathUtilTest, LocalPathParser) { - const bool result = file_system_provider_service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName)); - ASSERT_TRUE(result); + const base::File::Error result = + file_system_provider_service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName)); + ASSERT_EQ(base::File::FILE_OK, result); const ProvidedFileSystemInfo file_system_info = file_system_provider_service_->GetProvidedFileSystem(kExtensionId, kFileSystemId) @@ -261,9 +265,10 @@ TEST_F(FileSystemProviderMountPathUtilTest, LocalPathParser) { } TEST_F(FileSystemProviderMountPathUtilTest, LocalPathParser_RootPath) { - const bool result = file_system_provider_service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName)); - ASSERT_TRUE(result); + const base::File::Error result = + file_system_provider_service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName)); + ASSERT_EQ(base::File::FILE_OK, result); const ProvidedFileSystemInfo file_system_info = file_system_provider_service_->GetProvidedFileSystem(kExtensionId, kFileSystemId) diff --git a/chrome/browser/chromeos/file_system_provider/provided_file_system.cc b/chrome/browser/chromeos/file_system_provider/provided_file_system.cc index cdf2a46..c4e6cd3 100644 --- a/chrome/browser/chromeos/file_system_provider/provided_file_system.cc +++ b/chrome/browser/chromeos/file_system_provider/provided_file_system.cc @@ -501,34 +501,40 @@ void ProvidedFileSystem::RemoveObserver(ProvidedFileSystemObserver* observer) { observers_.RemoveObserver(observer); } -bool ProvidedFileSystem::Notify( +void ProvidedFileSystem::Notify( const base::FilePath& entry_path, bool recursive, storage::WatcherManager::ChangeType change_type, scoped_ptr<ProvidedFileSystemObserver::Changes> changes, - const std::string& tag) { + const std::string& tag, + const storage::AsyncFileUtil::StatusCallback& callback) { const WatcherKey key(entry_path, recursive); const auto& watcher_it = watchers_.find(key); - if (watcher_it == watchers_.end()) - return false; + if (watcher_it == watchers_.end()) { + callback.Run(base::File::FILE_ERROR_NOT_FOUND); + return; + } // The tag must be provided if and only if it's explicitly supported. - if (file_system_info_.supports_notify_tag() == tag.empty()) - return false; + if (file_system_info_.supports_notify_tag() == tag.empty()) { + callback.Run(base::File::FILE_ERROR_INVALID_OPERATION); + return; + } + + // It's illegal to provide a tag which is not unique. + if (!tag.empty() && tag == watcher_it->second.last_tag) { + callback.Run(base::File::FILE_ERROR_INVALID_OPERATION); + return; + } // The object is owned by AutoUpdated, so the reference is valid as long as // callbacks created with AutoUpdater::CreateCallback(). const ProvidedFileSystemObserver::Changes& changes_ref = *changes.get(); - scoped_refptr<AutoUpdater> auto_updater( - new AutoUpdater(base::Bind(&ProvidedFileSystem::OnNotifyCompleted, - weak_ptr_factory_.GetWeakPtr(), - entry_path, - recursive, - change_type, - base::Passed(&changes), - watcher_it->second.last_tag, - tag))); + scoped_refptr<AutoUpdater> auto_updater(new AutoUpdater( + base::Bind(&ProvidedFileSystem::OnNotifyCompleted, + weak_ptr_factory_.GetWeakPtr(), entry_path, recursive, + change_type, base::Passed(&changes), tag, callback))); // Call all notification callbacks (if any). for (const auto& subscriber_it : watcher_it->second.subscribers) { @@ -546,8 +552,6 @@ bool ProvidedFileSystem::Notify( change_type, changes_ref, auto_updater->CreateCallback())); - - return true; } base::WeakPtr<ProvidedFileSystemInterface> ProvidedFileSystem::GetWeakPtr() { @@ -607,23 +611,18 @@ void ProvidedFileSystem::OnNotifyCompleted( bool recursive, storage::WatcherManager::ChangeType change_type, scoped_ptr<ProvidedFileSystemObserver::Changes> /* changes */, - const std::string& last_tag, - const std::string& tag) { + const std::string& tag, + const storage::AsyncFileUtil::StatusCallback& callback) { const WatcherKey key(entry_path, recursive); const Watchers::iterator it = watchers_.find(key); // Check if the entry is still watched. - if (it == watchers_.end()) - return; - - // Another following notification finished earlier. - if (it->second.last_tag != last_tag) + if (it == watchers_.end()) { + callback.Run(base::File::FILE_ERROR_NOT_FOUND); return; + } - // It's illegal to provide a tag which is not unique. As for now only an error - // message is printed, but we may want to pass the error to the providing - // extension. TODO(mtomasz): Consider it. - if (!tag.empty() && tag == it->second.last_tag) - LOG(ERROR) << "Tag specified, but same as the previous one."; + // TODO(mtomasz): Add an async queue around notify and other watcher related + // methods so there is no race. it->second.last_tag = tag; @@ -643,6 +642,8 @@ void ProvidedFileSystem::OnNotifyCompleted( base::Bind(&EmptyStatusCallback)); } } + + callback.Run(base::File::FILE_OK); } } // namespace file_system_provider diff --git a/chrome/browser/chromeos/file_system_provider/provided_file_system.h b/chrome/browser/chromeos/file_system_provider/provided_file_system.h index 3f8b7a6..9c5dd3a 100644 --- a/chrome/browser/chromeos/file_system_provider/provided_file_system.h +++ b/chrome/browser/chromeos/file_system_provider/provided_file_system.h @@ -156,11 +156,13 @@ class ProvidedFileSystem : public ProvidedFileSystemInterface { virtual Watchers* GetWatchers() override; virtual void AddObserver(ProvidedFileSystemObserver* observer) override; virtual void RemoveObserver(ProvidedFileSystemObserver* observer) override; - virtual bool Notify(const base::FilePath& entry_path, - bool recursive, - storage::WatcherManager::ChangeType change_type, - scoped_ptr<ProvidedFileSystemObserver::Changes> changes, - const std::string& tag) override; + virtual void Notify( + const base::FilePath& entry_path, + bool recursive, + storage::WatcherManager::ChangeType change_type, + scoped_ptr<ProvidedFileSystemObserver::Changes> changes, + const std::string& tag, + const storage::AsyncFileUtil::StatusCallback& callback) override; virtual base::WeakPtr<ProvidedFileSystemInterface> GetWeakPtr() override; private: @@ -179,14 +181,14 @@ class ProvidedFileSystem : public ProvidedFileSystemInterface { base::File::Error result); // Called when all observers finished handling the change notification. It - // updates the tag from |last_tag| to |tag| for the entry at |entry_path|. + // updates the tag to |tag| for the entry at |entry_path|. void OnNotifyCompleted( const base::FilePath& entry_path, bool recursive, storage::WatcherManager::ChangeType change_type, scoped_ptr<ProvidedFileSystemObserver::Changes> changes, - const std::string& last_tag, - const std::string& tag); + const std::string& tag, + const storage::AsyncFileUtil::StatusCallback& callback); Profile* profile_; // Not owned. extensions::EventRouter* event_router_; // Not owned. May be NULL. diff --git a/chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h b/chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h index 4e8c4b8..9395a64 100644 --- a/chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h +++ b/chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h @@ -198,14 +198,17 @@ class ProvidedFileSystemInterface { const storage::AsyncFileUtil::StatusCallback& callback) = 0; // Notifies about changes related to the watcher within the file system. - // Invoked by the file system implementation. Returns false if the - // notification arguments are malformed or the entry is not watched anymore. + // Invoked by the file system implementation. Returns an error code via the + // callback if the notification arguments are malformed or the entry is not + // watched anymore. On success, returns base::File::FILE_OK. // TODO(mtomasz): Replace [entry_path, recursive] with a watcher id. - virtual bool Notify(const base::FilePath& entry_path, - bool recursive, - storage::WatcherManager::ChangeType change_type, - scoped_ptr<ProvidedFileSystemObserver::Changes> changes, - const std::string& tag) = 0; + virtual void Notify( + const base::FilePath& entry_path, + bool recursive, + storage::WatcherManager::ChangeType change_type, + scoped_ptr<ProvidedFileSystemObserver::Changes> changes, + const std::string& tag, + const storage::AsyncFileUtil::StatusCallback& callback) = 0; // Returns a provided file system info for this file system. virtual const ProvidedFileSystemInfo& GetFileSystemInfo() const = 0; diff --git a/chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc b/chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc index 905458d..649e1ac 100644 --- a/chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc +++ b/chrome/browser/chromeos/file_system_provider/provided_file_system_unittest.cc @@ -757,12 +757,12 @@ TEST_F(FileSystemProviderProvidedFileSystemTest, Notify) { const storage::WatcherManager::ChangeType change_type = storage::WatcherManager::CHANGED; const std::string tag = "hello-world"; - EXPECT_TRUE(provided_file_system_->Notify( - base::FilePath::FromUTF8Unsafe(kDirectoryPath), - false /* recursive */, - change_type, - make_scoped_ptr(new ProvidedFileSystemObserver::Changes), - tag)); + + Log log; + provided_file_system_->Notify( + base::FilePath::FromUTF8Unsafe(kDirectoryPath), false /* recursive */, + change_type, make_scoped_ptr(new ProvidedFileSystemObserver::Changes), + tag, base::Bind(&LogStatus, base::Unretained(&log))); // Confirm that the notification callback was called. ASSERT_EQ(1u, notification_log.size()); @@ -785,6 +785,9 @@ TEST_F(FileSystemProviderProvidedFileSystemTest, Notify) { // Wait until all observers finish handling the notification. base::RunLoop().RunUntilIdle(); + ASSERT_EQ(1u, log.size()); + EXPECT_EQ(base::File::FILE_OK, log[0]); + // Confirm, that the watcher still exists, and that the tag is updated. ASSERT_EQ(1u, watchers->size()); EXPECT_EQ(tag, watchers->begin()->second.last_tag); @@ -798,13 +801,15 @@ TEST_F(FileSystemProviderProvidedFileSystemTest, Notify) { storage::WatcherManager::DELETED; const ProvidedFileSystemObserver::Changes changes; const std::string tag = "chocolate-disco"; - EXPECT_TRUE(provided_file_system_->Notify( - base::FilePath::FromUTF8Unsafe(kDirectoryPath), - false /* recursive */, - change_type, - make_scoped_ptr(new ProvidedFileSystemObserver::Changes), - tag)); + + Log log; + provided_file_system_->Notify( + base::FilePath::FromUTF8Unsafe(kDirectoryPath), false /* recursive */, + change_type, make_scoped_ptr(new ProvidedFileSystemObserver::Changes), + tag, base::Bind(&LogStatus, base::Unretained(&log))); base::RunLoop().RunUntilIdle(); + ASSERT_EQ(1u, log.size()); + EXPECT_EQ(base::File::FILE_OK, log[0]); // Confirm that the notification callback was called. ASSERT_EQ(2u, notification_log.size()); diff --git a/chrome/browser/chromeos/file_system_provider/request_manager.cc b/chrome/browser/chromeos/file_system_provider/request_manager.cc index 214eab2..73bdbb3 100644 --- a/chrome/browser/chromeos/file_system_provider/request_manager.cc +++ b/chrome/browser/chromeos/file_system_provider/request_manager.cc @@ -119,13 +119,14 @@ int RequestManager::CreateRequest(RequestType type, return request_id; } -bool RequestManager::FulfillRequest(int request_id, - scoped_ptr<RequestValue> response, - bool has_more) { +base::File::Error RequestManager::FulfillRequest( + int request_id, + scoped_ptr<RequestValue> response, + bool has_more) { CHECK(response.get()); RequestMap::iterator request_it = requests_.find(request_id); if (request_it == requests_.end()) - return false; + return base::File::FILE_ERROR_NOT_FOUND; FOR_EACH_OBSERVER(Observer, observers_, @@ -141,16 +142,17 @@ bool RequestManager::FulfillRequest(int request_id, ResetTimer(request_id); } - return true; + return base::File::FILE_OK; } -bool RequestManager::RejectRequest(int request_id, - scoped_ptr<RequestValue> response, - base::File::Error error) { +base::File::Error RequestManager::RejectRequest( + int request_id, + scoped_ptr<RequestValue> response, + base::File::Error error) { CHECK(response.get()); RequestMap::iterator request_it = requests_.find(request_id); if (request_it == requests_.end()) - return false; + return base::File::FILE_ERROR_NOT_FOUND; FOR_EACH_OBSERVER(Observer, observers_, @@ -158,7 +160,7 @@ bool RequestManager::RejectRequest(int request_id, request_it->second->handler->OnError(request_id, response.Pass(), error); DestroyRequest(request_id); - return true; + return base::File::FILE_OK; } void RequestManager::SetTimeoutForTesting(const base::TimeDelta& timeout) { diff --git a/chrome/browser/chromeos/file_system_provider/request_manager.h b/chrome/browser/chromeos/file_system_provider/request_manager.h index 913408b..ccc87d8 100644 --- a/chrome/browser/chromeos/file_system_provider/request_manager.h +++ b/chrome/browser/chromeos/file_system_provider/request_manager.h @@ -113,17 +113,19 @@ class RequestManager { int CreateRequest(RequestType type, scoped_ptr<HandlerInterface> handler); // Handles successful response for the |request_id|. If |has_more| is false, - // then the request is disposed, after handling the |response|. On error, - // returns false, and the request is disposed. |response| must not be NULL. - bool FulfillRequest(int request_id, - scoped_ptr<RequestValue> response, - bool has_more); - - // Handles error response for the |request_id|. If handling the error fails, - // returns false. Always disposes the request. |response| must not be NULL. - bool RejectRequest(int request_id, - scoped_ptr<RequestValue> response, - base::File::Error error); + // then the request is disposed, after handling the |response|. On success, + // returns base::File::FILE_OK. Otherwise returns an error code. |response| + // must not be NULL. + base::File::Error FulfillRequest(int request_id, + scoped_ptr<RequestValue> response, + bool has_more); + + // Handles error response for the |request_id|. If handling the error + // succeeds, theen returns base::File::FILE_OK. Otherwise returns an error + // code. Always disposes the request. |response| must not be NULL. + base::File::Error RejectRequest(int request_id, + scoped_ptr<RequestValue> response, + base::File::Error error); // Sets a custom timeout for tests. The new timeout value will be applied to // new requests diff --git a/chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc b/chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc index b2363c3..4bb8339 100644 --- a/chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc +++ b/chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc @@ -377,9 +377,9 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill) { RequestValue::CreateForTesting("i-like-vanilla")); const bool has_more = false; - bool result = + const base::File::Error result = request_manager_->FulfillRequest(request_id, response.Pass(), has_more); - EXPECT_TRUE(result); + EXPECT_EQ(base::File::FILE_OK, result); ASSERT_EQ(1u, observer.fulfilled().size()); EXPECT_EQ(request_id, observer.fulfilled()[0].request_id()); @@ -402,19 +402,18 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill) { request_manager_->GetActiveRequestIds(); EXPECT_EQ(0u, active_request_ids.size()); - bool retry = request_manager_->FulfillRequest( + const base::File::Error retry = request_manager_->FulfillRequest( request_id, scoped_ptr<RequestValue>(new RequestValue), has_more); - EXPECT_FALSE(retry); + EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, retry); EXPECT_EQ(1u, observer.fulfilled().size()); } // Rejecting should also fail. { - bool retry = request_manager_->RejectRequest( - request_id, - scoped_ptr<RequestValue>(new RequestValue()), + const base::File::Error retry = request_manager_->RejectRequest( + request_id, scoped_ptr<RequestValue>(new RequestValue()), base::File::FILE_ERROR_FAILED); - EXPECT_FALSE(retry); + EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, retry); EXPECT_EQ(0u, observer.rejected().size()); } @@ -448,9 +447,9 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill_WithHasNext) { const bool has_more = true; - bool result = request_manager_->FulfillRequest( + const base::File::Error result = request_manager_->FulfillRequest( request_id, scoped_ptr<RequestValue>(new RequestValue), has_more); - EXPECT_TRUE(result); + EXPECT_EQ(base::File::FILE_OK, result); // Validate if the callback has correct arguments. ASSERT_EQ(1u, logger.success_events().size()); @@ -471,10 +470,10 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill_WithHasNext) { ASSERT_EQ(1u, active_request_ids.size()); EXPECT_EQ(request_id, active_request_ids[0]); - bool new_has_more = false; - bool retry = request_manager_->FulfillRequest( + const bool new_has_more = false; + const base::File::Error retry = request_manager_->FulfillRequest( request_id, scoped_ptr<RequestValue>(new RequestValue), new_has_more); - EXPECT_TRUE(retry); + EXPECT_EQ(base::File::FILE_OK, retry); ASSERT_EQ(2u, observer.fulfilled().size()); EXPECT_EQ(request_id, observer.fulfilled()[1].request_id()); @@ -488,10 +487,10 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndFulFill_WithHasNext) { request_manager_->GetActiveRequestIds(); EXPECT_EQ(0u, active_request_ids.size()); - bool new_has_more = false; - bool retry = request_manager_->FulfillRequest( + const bool new_has_more = false; + const base::File::Error retry = request_manager_->FulfillRequest( request_id, scoped_ptr<RequestValue>(new RequestValue), new_has_more); - EXPECT_FALSE(retry); + EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, retry); EXPECT_EQ(0u, observer.rejected().size()); } @@ -523,10 +522,10 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndReject) { ASSERT_EQ(1u, observer.executed().size()); EXPECT_EQ(request_id, observer.executed()[0].request_id()); - base::File::Error error = base::File::FILE_ERROR_NO_MEMORY; - bool result = request_manager_->RejectRequest( + const base::File::Error error = base::File::FILE_ERROR_NO_MEMORY; + const base::File::Error result = request_manager_->RejectRequest( request_id, scoped_ptr<RequestValue>(new RequestValue()), error); - EXPECT_TRUE(result); + EXPECT_EQ(base::File::FILE_OK, result); // Validate if the callback has correct arguments. ASSERT_EQ(1u, logger.error_events().size()); @@ -541,18 +540,18 @@ TEST_F(FileSystemProviderRequestManagerTest, CreateAndReject) { // Confirm, that the request is removed. Basically, fulfilling again for the // same request, should fail. { - bool has_more = false; - bool retry = request_manager_->FulfillRequest( + const bool has_more = false; + const base::File::Error retry = request_manager_->FulfillRequest( request_id, scoped_ptr<RequestValue>(new RequestValue), has_more); - EXPECT_FALSE(retry); + EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, retry); EXPECT_EQ(0u, observer.fulfilled().size()); } // Rejecting should also fail. { - bool retry = request_manager_->RejectRequest( + const base::File::Error retry = request_manager_->RejectRequest( request_id, scoped_ptr<RequestValue>(new RequestValue()), error); - EXPECT_FALSE(retry); + EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, retry); EXPECT_EQ(1u, observer.rejected().size()); } @@ -587,9 +586,9 @@ TEST_F(FileSystemProviderRequestManagerTest, const bool has_more = true; - const bool result = request_manager_->FulfillRequest( + const base::File::Error result = request_manager_->FulfillRequest( request_id + 1, scoped_ptr<RequestValue>(new RequestValue), has_more); - EXPECT_FALSE(result); + EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, result); // Callbacks should not be called. EXPECT_EQ(0u, logger.error_events().size()); @@ -600,9 +599,9 @@ TEST_F(FileSystemProviderRequestManagerTest, // Confirm, that the request hasn't been removed, by fulfilling it correctly. { - const bool retry = request_manager_->FulfillRequest( + const base::File::Error retry = request_manager_->FulfillRequest( request_id, scoped_ptr<RequestValue>(new RequestValue), has_more); - EXPECT_TRUE(retry); + EXPECT_EQ(base::File::FILE_OK, retry); EXPECT_EQ(1u, observer.fulfilled().size()); } @@ -631,10 +630,10 @@ TEST_F(FileSystemProviderRequestManagerTest, ASSERT_EQ(1u, observer.executed().size()); EXPECT_EQ(request_id, observer.executed()[0].request_id()); - base::File::Error error = base::File::FILE_ERROR_NO_MEMORY; - bool result = request_manager_->RejectRequest( + const base::File::Error error = base::File::FILE_ERROR_NO_MEMORY; + const base::File::Error result = request_manager_->RejectRequest( request_id + 1, scoped_ptr<RequestValue>(new RequestValue()), error); - EXPECT_FALSE(result); + EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, result); // Callbacks should not be called. EXPECT_EQ(0u, logger.error_events().size()); @@ -644,9 +643,9 @@ TEST_F(FileSystemProviderRequestManagerTest, // Confirm, that the request hasn't been removed, by rejecting it correctly. { - bool retry = request_manager_->RejectRequest( + const base::File::Error retry = request_manager_->RejectRequest( request_id, scoped_ptr<RequestValue>(new RequestValue()), error); - EXPECT_TRUE(retry); + EXPECT_EQ(base::File::FILE_OK, retry); EXPECT_EQ(1u, observer.rejected().size()); } diff --git a/chrome/browser/chromeos/file_system_provider/service.cc b/chrome/browser/chromeos/file_system_provider/service.cc index 2cd2467..ccba53b 100644 --- a/chrome/browser/chromeos/file_system_provider/service.cc +++ b/chrome/browser/chromeos/file_system_provider/service.cc @@ -60,9 +60,9 @@ Service::~Service() { const std::string extension_id = it->second->GetFileSystemInfo().extension_id(); ++it; - const bool unmount_result = UnmountFileSystem( + const base::File::Error unmount_result = UnmountFileSystem( extension_id, file_system_id, UNMOUNT_REASON_SHUTDOWN); - DCHECK(unmount_result); + DCHECK_EQ(base::File::FILE_OK, unmount_result); } DCHECK_EQ(0u, file_system_map_.size()); @@ -95,8 +95,8 @@ void Service::SetRegistryForTesting(scoped_ptr<RegistryInterface> registry) { registry_.reset(registry.release()); } -bool Service::MountFileSystem(const std::string& extension_id, - const MountOptions& options) { +base::File::Error Service::MountFileSystem(const std::string& extension_id, + const MountOptions& options) { DCHECK(thread_checker_.CalledOnValidThread()); // If already exists a file system provided by the same extension with this @@ -106,7 +106,7 @@ bool Service::MountFileSystem(const std::string& extension_id, observers_, OnProvidedFileSystemMount(ProvidedFileSystemInfo(), base::File::FILE_ERROR_EXISTS)); - return false; + return base::File::FILE_ERROR_EXISTS; } // Restrict number of file systems to prevent system abusing. @@ -116,7 +116,7 @@ bool Service::MountFileSystem(const std::string& extension_id, observers_, OnProvidedFileSystemMount(ProvidedFileSystemInfo(), base::File::FILE_ERROR_TOO_MANY_OPENED)); - return false; + return base::File::FILE_ERROR_TOO_MANY_OPENED; } storage::ExternalMountPoints* const mount_points = @@ -138,7 +138,7 @@ bool Service::MountFileSystem(const std::string& extension_id, observers_, OnProvidedFileSystemMount(ProvidedFileSystemInfo(), base::File::FILE_ERROR_INVALID_OPERATION)); - return false; + return base::File::FILE_ERROR_INVALID_OPERATION; } // Store the file system descriptor. Use the mount point name as the file @@ -165,12 +165,12 @@ bool Service::MountFileSystem(const std::string& extension_id, observers_, OnProvidedFileSystemMount(file_system_info, base::File::FILE_OK)); - return true; + return base::File::FILE_OK; } -bool Service::UnmountFileSystem(const std::string& extension_id, - const std::string& file_system_id, - UnmountReason reason) { +base::File::Error Service::UnmountFileSystem(const std::string& extension_id, + const std::string& file_system_id, + UnmountReason reason) { DCHECK(thread_checker_.CalledOnValidThread()); const ProvidedFileSystemMap::iterator file_system_it = @@ -182,7 +182,7 @@ bool Service::UnmountFileSystem(const std::string& extension_id, observers_, OnProvidedFileSystemUnmount(empty_file_system_info, base::File::FILE_ERROR_NOT_FOUND)); - return false; + return base::File::FILE_ERROR_NOT_FOUND; } storage::ExternalMountPoints* const mount_points = @@ -200,7 +200,7 @@ bool Service::UnmountFileSystem(const std::string& extension_id, observers_, OnProvidedFileSystemUnmount(file_system_info, base::File::FILE_ERROR_INVALID_OPERATION)); - return false; + return base::File::FILE_ERROR_INVALID_OPERATION; } FOR_EACH_OBSERVER( @@ -218,7 +218,7 @@ bool Service::UnmountFileSystem(const std::string& extension_id, delete file_system_it->second; file_system_map_.erase(file_system_it); - return true; + return base::File::FILE_OK; } bool Service::RequestUnmount(const std::string& extension_id, @@ -275,13 +275,12 @@ void Service::OnExtensionUnloaded( // by the UnmountFileSystem() call. ++it; if (file_system_info.extension_id() == extension->id()) { - const bool unmount_result = UnmountFileSystem( - file_system_info.extension_id(), - file_system_info.file_system_id(), + const base::File::Error unmount_result = UnmountFileSystem( + file_system_info.extension_id(), file_system_info.file_system_id(), reason == extensions::UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN ? UNMOUNT_REASON_SHUTDOWN : UNMOUNT_REASON_USER); - DCHECK(unmount_result); + DCHECK_EQ(base::File::FILE_OK, unmount_result); } } } @@ -292,9 +291,9 @@ void Service::OnExtensionLoaded(content::BrowserContext* browser_context, registry_->RestoreFileSystems(extension->id()); for (const auto& restored_file_system : *restored_file_systems) { - const bool result = MountFileSystem(restored_file_system.extension_id, - restored_file_system.options); - if (!result) { + const base::File::Error result = MountFileSystem( + restored_file_system.extension_id, restored_file_system.options); + if (result != base::File::FILE_OK) { LOG(ERROR) << "Failed to restore a provided file system from " << "registry: " << restored_file_system.extension_id << ", " << restored_file_system.options.file_system_id << ", " diff --git a/chrome/browser/chromeos/file_system_provider/service.h b/chrome/browser/chromeos/file_system_provider/service.h index e6eb3cf..5d6e979 100644 --- a/chrome/browser/chromeos/file_system_provider/service.h +++ b/chrome/browser/chromeos/file_system_provider/service.h @@ -82,15 +82,16 @@ class Service : public KeyedService, // Otherwise, only read-only operations are supported. If change notification // tags are supported, then |supports_notify_tag| must be true. Note, that // it is required in order to enable the internal cache. For success, returns - // true, otherwise false. - bool MountFileSystem(const std::string& extension_id, - const MountOptions& options); + // base::File::FILE_OK, otherwise an error code. + base::File::Error MountFileSystem(const std::string& extension_id, + const MountOptions& options); // Unmounts a file system with the specified |file_system_id| for the - // |extension_id|. For success returns true, otherwise false. - bool UnmountFileSystem(const std::string& extension_id, - const std::string& file_system_id, - UnmountReason reason); + // |extension_id|. For success returns base::File::FILE_OK, otherwise an error + // code. + base::File::Error UnmountFileSystem(const std::string& extension_id, + const std::string& file_system_id, + UnmountReason reason); // Requests unmounting of the file system. The callback is called when the // request is accepted or rejected, with an error code. Returns false if the diff --git a/chrome/browser/chromeos/file_system_provider/service_unittest.cc b/chrome/browser/chromeos/file_system_provider/service_unittest.cc index 372f61d..90bf652 100644 --- a/chrome/browser/chromeos/file_system_provider/service_unittest.cc +++ b/chrome/browser/chromeos/file_system_provider/service_unittest.cc @@ -216,8 +216,9 @@ TEST_F(FileSystemProviderServiceTest, MountFileSystem) { LoggingObserver observer; service_->AddObserver(&observer); - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); ASSERT_EQ(1u, observer.mounts.size()); EXPECT_EQ(kExtensionId, observer.mounts[0].file_system_info().extension_id()); @@ -248,7 +249,8 @@ TEST_F(FileSystemProviderServiceTest, MountOptions options(kFileSystemId, kDisplayName); options.writable = true; options.supports_notify_tag = true; - EXPECT_TRUE(service_->MountFileSystem(kExtensionId, options)); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem(kExtensionId, options)); ASSERT_EQ(1u, observer.mounts.size()); EXPECT_TRUE(observer.mounts[0].file_system_info().writable()); @@ -265,10 +267,12 @@ TEST_F(FileSystemProviderServiceTest, MountFileSystem_UniqueIds) { LoggingObserver observer; service_->AddObserver(&observer); - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); - EXPECT_FALSE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_ERROR_EXISTS, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); ASSERT_EQ(2u, observer.mounts.size()); EXPECT_EQ(base::File::FILE_OK, observer.mounts[0].error()); @@ -289,14 +293,16 @@ TEST_F(FileSystemProviderServiceTest, MountFileSystem_StressTest) { for (size_t i = 0; i < kMaxFileSystems; ++i) { const std::string file_system_id = std::string("test-") + base::IntToString(i); - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(file_system_id, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(file_system_id, kDisplayName))); } ASSERT_EQ(kMaxFileSystems, observer.mounts.size()); // The next file system is out of limit, and registering it should fail. - EXPECT_FALSE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_ERROR_TOO_MANY_OPENED, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); ASSERT_EQ(kMaxFileSystems + 1, observer.mounts.size()); EXPECT_EQ(base::File::FILE_ERROR_TOO_MANY_OPENED, @@ -313,12 +319,14 @@ TEST_F(FileSystemProviderServiceTest, UnmountFileSystem) { LoggingObserver observer; service_->AddObserver(&observer); - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); ASSERT_EQ(1u, observer.mounts.size()); - EXPECT_TRUE(service_->UnmountFileSystem( - kExtensionId, kFileSystemId, Service::UNMOUNT_REASON_USER)); + EXPECT_EQ(base::File::FILE_OK, + service_->UnmountFileSystem(kExtensionId, kFileSystemId, + Service::UNMOUNT_REASON_USER)); ASSERT_EQ(1u, observer.unmounts.size()); EXPECT_EQ(base::File::FILE_OK, observer.unmounts[0].error()); @@ -338,8 +346,9 @@ TEST_F(FileSystemProviderServiceTest, UnmountFileSystem_OnExtensionUnload) { LoggingObserver observer; service_->AddObserver(&observer); - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); ASSERT_EQ(1u, observer.mounts.size()); // Directly call the observer's method. @@ -369,13 +378,15 @@ TEST_F(FileSystemProviderServiceTest, UnmountFileSystem_WrongExtensionId) { const std::string kWrongExtensionId = "helloworldhelloworldhelloworldhe"; - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); ASSERT_EQ(1u, observer.mounts.size()); ASSERT_EQ(1u, service_->GetProvidedFileSystemInfoList().size()); - EXPECT_FALSE(service_->UnmountFileSystem( - kWrongExtensionId, kFileSystemId, Service::UNMOUNT_REASON_USER)); + EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, + service_->UnmountFileSystem(kWrongExtensionId, kFileSystemId, + Service::UNMOUNT_REASON_USER)); ASSERT_EQ(1u, observer.unmounts.size()); EXPECT_EQ(base::File::FILE_ERROR_NOT_FOUND, observer.unmounts[0].error()); ASSERT_EQ(1u, service_->GetProvidedFileSystemInfoList().size()); @@ -448,8 +459,9 @@ TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnMount) { EXPECT_FALSE(registry_->file_system_info()); EXPECT_FALSE(registry_->watchers()); - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); ASSERT_EQ(1u, observer.mounts.size()); ASSERT_TRUE(registry_->file_system_info()); @@ -470,8 +482,9 @@ TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnUnmountOnShutdown) { { EXPECT_FALSE(registry_->file_system_info()); EXPECT_FALSE(registry_->watchers()); - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); EXPECT_EQ(1u, observer.mounts.size()); EXPECT_TRUE(registry_->file_system_info()); @@ -479,8 +492,9 @@ TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnUnmountOnShutdown) { } { - EXPECT_TRUE(service_->UnmountFileSystem( - kExtensionId, kFileSystemId, Service::UNMOUNT_REASON_SHUTDOWN)); + EXPECT_EQ(base::File::FILE_OK, + service_->UnmountFileSystem(kExtensionId, kFileSystemId, + Service::UNMOUNT_REASON_SHUTDOWN)); EXPECT_EQ(1u, observer.unmounts.size()); EXPECT_TRUE(registry_->file_system_info()); @@ -497,8 +511,9 @@ TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnUnmountByUser) { { EXPECT_FALSE(registry_->file_system_info()); EXPECT_FALSE(registry_->watchers()); - EXPECT_TRUE(service_->MountFileSystem( - kExtensionId, MountOptions(kFileSystemId, kDisplayName))); + EXPECT_EQ(base::File::FILE_OK, + service_->MountFileSystem( + kExtensionId, MountOptions(kFileSystemId, kDisplayName))); EXPECT_EQ(1u, observer.mounts.size()); EXPECT_TRUE(registry_->file_system_info()); @@ -506,8 +521,9 @@ TEST_F(FileSystemProviderServiceTest, RememberFileSystem_OnUnmountByUser) { } { - EXPECT_TRUE(service_->UnmountFileSystem( - kExtensionId, kFileSystemId, Service::UNMOUNT_REASON_USER)); + EXPECT_EQ(base::File::FILE_OK, + service_->UnmountFileSystem(kExtensionId, kFileSystemId, + Service::UNMOUNT_REASON_USER)); EXPECT_EQ(1u, observer.unmounts.size()); EXPECT_FALSE(registry_->file_system_info()); diff --git a/chrome/test/data/extensions/api_test/file_system_provider/get_all/test.js b/chrome/test/data/extensions/api_test/file_system_provider/get_all/test.js index 0e48c51..d5a83aa 100644 --- a/chrome/test/data/extensions/api_test/file_system_provider/get_all/test.js +++ b/chrome/test/data/extensions/api_test/file_system_provider/get_all/test.js @@ -46,7 +46,7 @@ chrome.test.runTests([ function mountError() { chrome.fileSystemProvider.mount( {fileSystemId: '', displayName: ''}, - chrome.test.callbackFail('SECURITY', function() { + chrome.test.callbackFail('INVALID_OPERATION', function() { chrome.fileSystemProvider.getAll(chrome.test.callbackPass( function(fileSystems) { chrome.test.assertEq(0, fileSystems.length); diff --git a/chrome/test/data/extensions/api_test/file_system_provider/mount/test.js b/chrome/test/data/extensions/api_test/file_system_provider/mount/test.js index 25c6e04..81b6ae7 100644 --- a/chrome/test/data/extensions/api_test/file_system_provider/mount/test.js +++ b/chrome/test/data/extensions/api_test/file_system_provider/mount/test.js @@ -19,14 +19,14 @@ chrome.test.runTests([ function emptyDisplayName() { chrome.fileSystemProvider.mount( {fileSystemId: 'file-system-id', displayName: ''}, - chrome.test.callbackFail('SECURITY')); + chrome.test.callbackFail('INVALID_OPERATION')); }, // Verifies that mounting fails, when an empty string is provided as an Id function emptyFileSystemId() { chrome.fileSystemProvider.mount( {fileSystemId: '', displayName: 'File System Name'}, - chrome.test.callbackFail('SECURITY')); + chrome.test.callbackFail('INVALID_OPERATION')); }, // End to end test. Mounts a volume using fileSystemProvider.mount(), then @@ -97,7 +97,7 @@ chrome.test.runTests([ fileSystemId: 'over-the-limit-fs-id', displayName: 'Over The Limit File System' }, - chrome.test.callbackFail('SECURITY')); + chrome.test.callbackFail('TOO_MANY_OPENED')); } }; tryNextOne(); diff --git a/chrome/test/data/extensions/api_test/file_system_provider/notify/test.js b/chrome/test/data/extensions/api_test/file_system_provider/notify/test.js index f375401..0beaa1c 100644 --- a/chrome/test/data/extensions/api_test/file_system_provider/notify/test.js +++ b/chrome/test/data/extensions/api_test/file_system_provider/notify/test.js @@ -140,7 +140,7 @@ function runTests() { observedPath: fileEntry.fullPath, recursive: false, changeType: 'CHANGED', - }, chrome.test.callbackFail('SECURITY')); + }, chrome.test.callbackFail('INVALID_OPERATION')); }), function(error) { chrome.test.fail(error.name); @@ -165,7 +165,7 @@ function runTests() { recursive: true, changeType: 'CHANGED', tag: TESTING_ANOTHER_TAG, - }, chrome.test.callbackFail('SECURITY')); + }, chrome.test.callbackFail('NOT_FOUND')); })); }, @@ -223,7 +223,7 @@ function runTests() { recursive: false, changeType: 'CHANGED', tag: TESTING_ANOTHER_TAG - }, chrome.test.callbackFail('SECURITY')); + }, chrome.test.callbackFail('NOT_FOUND')); })); } ]); diff --git a/chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js b/chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js index 77e55a4..f6f9523 100644 --- a/chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js +++ b/chrome/test/data/extensions/api_test/file_system_provider/unmount/test.js @@ -70,7 +70,7 @@ function runTests() { function unmountWrongId() { chrome.fileSystemProvider.unmount( {fileSystemId: 'wrong-fs-id'}, - chrome.test.callbackFail('SECURITY')); + chrome.test.callbackFail('NOT_FOUND')); }, // Tests if fileManagerPrivate.removeMount() for provided file systems emits |