From 06ba081e89937dea18efa31eeecf8162dafd7434 Mon Sep 17 00:00:00 2001 From: Devlin Cronin Date: Thu, 3 Aug 2017 00:23:33 +0000 Subject: [PATCH] [Task Migration][Extensions] Update FileReader Update the FileReader class to use the new task scheduling API, as well as the dedicated Extensions-related file task runner. Bug: 689520 Bug: 750122 Change-Id: I3dd119e9c6bc368ab0186e599a4a67df02b872d6 Reviewed-on: https://chromium-review.googlesource.com/594010 Commit-Queue: Devlin Reviewed-by: Karan Bhatia Cr-Commit-Position: refs/heads/master@{#491566} --- .../browser/api/execute_code_function.cc | 2 +- extensions/browser/file_reader.cc | 25 ++++---- extensions/browser/file_reader.h | 17 +++--- extensions/browser/file_reader_unittest.cc | 59 +++++++++---------- 4 files changed, 50 insertions(+), 53 deletions(-) diff --git a/extensions/browser/api/execute_code_function.cc b/extensions/browser/api/execute_code_function.cc index bf04871ed89978..0c9efcb5b301fd 100644 --- a/extensions/browser/api/execute_code_function.cc +++ b/extensions/browser/api/execute_code_function.cc @@ -237,7 +237,7 @@ bool ExecuteCodeFunction::LoadFile(const std::string& file) { resource_.relative_path().AsUTF8Unsafe(), true /* We assume this call always succeeds */)); } else { - FileReader::OptionalFileThreadTaskCallback get_file_and_l10n_callback = + FileReader::OptionalFileSequenceTask get_file_and_l10n_callback = base::Bind(&ExecuteCodeFunction::GetFileURLAndMaybeLocalizeInBackground, this, extension_id, extension_path, extension_default_locale, might_require_localization); diff --git a/extensions/browser/file_reader.cc b/extensions/browser/file_reader.cc index 3efc7d51a27e7c..d00d410e359b96 100644 --- a/extensions/browser/file_reader.cc +++ b/extensions/browser/file_reader.cc @@ -8,37 +8,36 @@ #include "base/callback_helpers.h" #include "base/files/file_util.h" #include "base/threading/thread_task_runner_handle.h" -#include "content/public/browser/browser_thread.h" - -using content::BrowserThread; +#include "extensions/browser/extension_file_task_runner.h" FileReader::FileReader( const extensions::ExtensionResource& resource, - const OptionalFileThreadTaskCallback& optional_file_thread_task_callback, + const OptionalFileSequenceTask& optional_file_sequence_task, const DoneCallback& done_callback) : resource_(resource), - optional_file_thread_task_callback_(optional_file_thread_task_callback), + optional_file_sequence_task_(optional_file_sequence_task), done_callback_(done_callback), origin_task_runner_(base::ThreadTaskRunnerHandle::Get()) {} void FileReader::Start() { - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(&FileReader::ReadFileOnBackgroundThread, this)); + extensions::GetExtensionFileTaskRunner()->PostTask( + FROM_HERE, base::Bind(&FileReader::ReadFileOnFileSequence, this)); } FileReader::~FileReader() {} -void FileReader::ReadFileOnBackgroundThread() { +void FileReader::ReadFileOnFileSequence() { + DCHECK( + extensions::GetExtensionFileTaskRunner()->RunsTasksInCurrentSequence()); + std::unique_ptr data(new std::string()); bool success = base::ReadFileToString(resource_.GetFilePath(), data.get()); - if (!optional_file_thread_task_callback_.is_null()) { + if (!optional_file_sequence_task_.is_null()) { if (success) { - base::ResetAndReturn(&optional_file_thread_task_callback_) - .Run(data.get()); + base::ResetAndReturn(&optional_file_sequence_task_).Run(data.get()); } else { - optional_file_thread_task_callback_.Reset(); + optional_file_sequence_task_.Reset(); } } diff --git a/extensions/browser/file_reader.h b/extensions/browser/file_reader.h index a9f88688cca1e1..15dc1bee82efed 100644 --- a/extensions/browser/file_reader.h +++ b/extensions/browser/file_reader.h @@ -13,37 +13,40 @@ #include "extensions/common/extension_resource.h" // This file defines an interface for reading a file asynchronously on a -// background thread. +// background sequence. // Consider abstracting out a FilePathProvider (ExtensionResource) and moving // back to chrome/browser/net if other subsystems want to use it. class FileReader : public base::RefCountedThreadSafe { public: + // TODO(devlin): Use base::OnceCallback here. // Reports success or failure and the data of the file upon success. using DoneCallback = base::Callback)>; // Lets the caller accomplish tasks on the file data, after the file content // has been read. // If the file reading doesn't succeed, this will be ignored. - using OptionalFileThreadTaskCallback = base::Callback; + using OptionalFileSequenceTask = base::Callback; FileReader(const extensions::ExtensionResource& resource, - const OptionalFileThreadTaskCallback& file_thread_task_callback, + const OptionalFileSequenceTask& file_sequence_task, const DoneCallback& done_callback); - // Called to start reading the file on a background thread. Upon completion, + // Called to start reading the file on a background sequence. Upon completion, // the callback will be notified of the results. void Start(); private: friend class base::RefCountedThreadSafe; - virtual ~FileReader(); + ~FileReader(); - void ReadFileOnBackgroundThread(); + void ReadFileOnFileSequence(); extensions::ExtensionResource resource_; - OptionalFileThreadTaskCallback optional_file_thread_task_callback_; + OptionalFileSequenceTask optional_file_sequence_task_; DoneCallback done_callback_; const scoped_refptr origin_task_runner_; + + DISALLOW_COPY_AND_ASSIGN(FileReader); }; #endif // EXTENSIONS_BROWSER_FILE_READER_H_ diff --git a/extensions/browser/file_reader_unittest.cc b/extensions/browser/file_reader_unittest.cc index 1e652420c46ee0..dcee191f88e381 100644 --- a/extensions/browser/file_reader_unittest.cc +++ b/extensions/browser/file_reader_unittest.cc @@ -2,41 +2,44 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "extensions/browser/file_reader.h" + #include "base/bind.h" #include "base/bind_helpers.h" #include "base/files/file_path.h" #include "base/files/file_util.h" -#include "base/message_loop/message_loop.h" #include "base/path_service.h" #include "base/run_loop.h" +#include "base/test/scoped_task_environment.h" #include "components/crx_file/id_util.h" -#include "content/public/test/test_browser_thread.h" -#include "extensions/browser/file_reader.h" #include "extensions/common/extension_paths.h" #include "extensions/common/extension_resource.h" #include "testing/gtest/include/gtest/gtest.h" -using content::BrowserThread; - namespace extensions { class FileReaderTest : public testing::Test { public: - FileReaderTest() : file_thread_(BrowserThread::FILE) { - file_thread_.Start(); - } + FileReaderTest() {} + private: - base::MessageLoop message_loop_; - content::TestBrowserThread file_thread_; + base::test::ScopedTaskEnvironment task_environment_; + + DISALLOW_COPY_AND_ASSIGN(FileReaderTest); }; class Receiver { public: - Receiver() : succeeded_(false) { - } - - FileReader::DoneCallback NewCallback() { - return base::Bind(&Receiver::DidReadFile, base::Unretained(this)); + Receiver(const ExtensionResource& resource) + : succeeded_(false), + file_reader_(new FileReader( + resource, + FileReader::OptionalFileSequenceTask(), + base::Bind(&Receiver::DidReadFile, base::Unretained(this)))) {} + + void Run() { + file_reader_->Start(); + run_loop_.Run(); } bool succeeded() const { return succeeded_; } @@ -46,11 +49,15 @@ class Receiver { void DidReadFile(bool success, std::unique_ptr data) { succeeded_ = success; data_ = std::move(data); - base::RunLoop::QuitCurrentWhenIdleDeprecated(); + run_loop_.QuitWhenIdle(); } bool succeeded_; std::unique_ptr data_; + scoped_refptr file_reader_; + base::RunLoop run_loop_; + + DISALLOW_COPY_AND_ASSIGN(Receiver); }; void RunBasicTest(const char* filename) { @@ -64,14 +71,8 @@ void RunBasicTest(const char* filename) { std::string file_contents; ASSERT_TRUE(base::ReadFileToString(path, &file_contents)); - Receiver receiver; - - scoped_refptr file_reader( - new FileReader(resource, FileReader::OptionalFileThreadTaskCallback(), - receiver.NewCallback())); - file_reader->Start(); - - base::RunLoop().Run(); + Receiver receiver(resource); + receiver.Run(); EXPECT_TRUE(receiver.succeeded()); EXPECT_EQ(file_contents, receiver.data()); @@ -93,14 +94,8 @@ TEST_F(FileReaderTest, NonExistantFile) { FILE_PATH_LITERAL("file_that_does_not_exist"))); path = path.AppendASCII("file_that_does_not_exist"); - Receiver receiver; - - scoped_refptr file_reader( - new FileReader(resource, FileReader::OptionalFileThreadTaskCallback(), - receiver.NewCallback())); - file_reader->Start(); - - base::RunLoop().Run(); + Receiver receiver(resource); + receiver.Run(); EXPECT_FALSE(receiver.succeeded()); }