Remove explicit usage of SequencedWorkerPool from UploadList.

It merely needs a TaskRunner to PostTask() to.

This allows the unittest to merely use a base::Thread and alleviates
issue 646443.

Also did some modern style cleanup:
 - ThreadChecker => SequenceChecker (no state is thread-affine)
 - const scoped_refptr<>& => scoped_refptr<> + std::move
   (avoids a refcount bump when handing off ownership:
    https://groups.google.com/a/chromium.org/d/topic/chromium-dev/TlL1D-Djta0/discussion)

BUG=646443
NO_DEPENDENCY_CHECKS=true

Review-Url: https://codereview.chromium.org/2335193007
Cr-Commit-Position: refs/heads/master@{#418981}

chrome/browser/crash_upload_list/crash_upload_list.cc

@@ -6,7 +6,6 @@
 
 #include "base/files/file_path.h"
 #include "base/path_service.h"
-#include "base/threading/sequenced_worker_pool.h"
 #include "build/build_config.h"
 #include "chrome/common/chrome_paths.h"
 #include "content/public/browser/browser_thread.h"

chrome/browser/crash_upload_list/crash_upload_list_android.cc

@@ -4,17 +4,19 @@
 
 #include "chrome/browser/crash_upload_list/crash_upload_list_android.h"
 
+#include <utility>
+
 #include "base/files/file.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
-#include "base/threading/sequenced_worker_pool.h"
+#include "base/task_runner.h"
 #include "ui/base/text/bytes_formatting.h"
 
 CrashUploadListAndroid::CrashUploadListAndroid(
     Delegate* delegate,
     const base::FilePath& upload_log_path,
-    const scoped_refptr<base::SequencedWorkerPool>& worker_pool)
-    : CrashUploadList(delegate, upload_log_path, worker_pool) {}
+    scoped_refptr<base::TaskRunner> task_runner)
+    : CrashUploadList(delegate, upload_log_path, std::move(task_runner)) {}
 
 CrashUploadListAndroid::~CrashUploadListAndroid() {}
 

chrome/browser/crash_upload_list/crash_upload_list_android.h

@@ -10,17 +10,16 @@
 
 namespace base {
 class FilePath;
-class SequencedWorkerPool;
+class TaskRunner;
 }
 
 // A CrashUploadList that retrieves the list of uploaded reports from the
 // Android crash reporter.
 class CrashUploadListAndroid : public CrashUploadList {
  public:
-  CrashUploadListAndroid(
-      Delegate* delegate,
-      const base::FilePath& upload_log_path,
-      const scoped_refptr<base::SequencedWorkerPool>& worker_pool);
+  CrashUploadListAndroid(Delegate* delegate,
+                         const base::FilePath& upload_log_path,
+                         scoped_refptr<base::TaskRunner> task_runner);
 
  protected:
   ~CrashUploadListAndroid() override;

chrome/browser/crash_upload_list/crash_upload_list_crashpad.cc

@@ -6,7 +6,9 @@
 
 #include <stddef.h>
 
-#include "base/threading/sequenced_worker_pool.h"
+#include <utility>
+
+#include "base/task_runner.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/common/chrome_constants.h"
@@ -80,8 +82,8 @@ UploadList::UploadInfo::State ReportUploadStateToUploadInfoState(
 
 CrashUploadListCrashpad::CrashUploadListCrashpad(
     Delegate* delegate,
-    const scoped_refptr<base::SequencedWorkerPool>& worker_pool)
-    : CrashUploadList(delegate, base::FilePath(), worker_pool) {}
+    scoped_refptr<base::TaskRunner> task_runner)
+    : CrashUploadList(delegate, base::FilePath(), std::move(task_runner)) {}
 
 CrashUploadListCrashpad::~CrashUploadListCrashpad() {}
 

chrome/browser/crash_upload_list/crash_upload_list_crashpad.h

@@ -10,16 +10,15 @@
 
 namespace base {
 class FilePath;
-class SequencedWorkerPool;
+class TaskRunner;
 }
 
 // A CrashUploadList that retrieves the list of uploaded reports from the
 // Crashpad database.
 class CrashUploadListCrashpad : public CrashUploadList {
  public:
-  CrashUploadListCrashpad(
-      Delegate* delegate,
-      const scoped_refptr<base::SequencedWorkerPool>& worker_pool);
+  CrashUploadListCrashpad(Delegate* delegate,
+                          scoped_refptr<base::TaskRunner> task_runner);
 
  protected:
   ~CrashUploadListCrashpad() override;

components/upload_list/crash_upload_list.cc

@@ -4,17 +4,17 @@
 
 #include "components/upload_list/crash_upload_list.h"
 
+#include <utility>
+
 #include "base/files/file_path.h"
 #include "base/path_service.h"
-#include "base/threading/sequenced_worker_pool.h"
 
 // static
 const char CrashUploadList::kReporterLogFilename[] = "uploads.log";
 
-CrashUploadList::CrashUploadList(
-    Delegate* delegate,
-    const base::FilePath& upload_log_path,
-    const scoped_refptr<base::SequencedWorkerPool>& worker_pool)
-    : UploadList(delegate, upload_log_path, worker_pool) {}
+CrashUploadList::CrashUploadList(Delegate* delegate,
+                                 const base::FilePath& upload_log_path,
+                                 scoped_refptr<base::TaskRunner> task_runner)
+    : UploadList(delegate, upload_log_path, std::move(task_runner)) {}
 
-CrashUploadList::~CrashUploadList() {}
+CrashUploadList::~CrashUploadList() = default;

components/upload_list/crash_upload_list.h

@@ -11,7 +11,7 @@
 
 namespace base {
 class FilePath;
-class SequencedWorkerPool;
+class TaskRunner;
 }
 
 // An upload list manager for crash reports from breakpad.
@@ -24,7 +24,7 @@ class CrashUploadList : public UploadList {
   // Creates a new crash upload list with the given callback delegate.
   CrashUploadList(Delegate* delegate,
                   const base::FilePath& upload_log_path,
-                  const scoped_refptr<base::SequencedWorkerPool>& worker_pool);
+                  scoped_refptr<base::TaskRunner> task_runner);
 
  protected:
   ~CrashUploadList() override;

components/upload_list/upload_list.cc

@@ -11,11 +11,12 @@
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
-#include "base/threading/sequenced_worker_pool.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/task_runner.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 
 UploadList::UploadInfo::UploadInfo(const std::string& upload_id,
                                    const base::Time& upload_time,
@@ -49,33 +50,31 @@ UploadList::UploadInfo::UploadInfo(const UploadInfo& upload_info)
       state(upload_info.state),
       file_size(upload_info.file_size) {}
 
-UploadList::UploadInfo::~UploadInfo() {}
+UploadList::UploadInfo::~UploadInfo() = default;
 
-UploadList::UploadList(
-    Delegate* delegate,
-    const base::FilePath& upload_log_path,
-    const scoped_refptr<base::SequencedWorkerPool>& worker_pool)
+UploadList::UploadList(Delegate* delegate,
+                       const base::FilePath& upload_log_path,
+                       scoped_refptr<base::TaskRunner> task_runner)
     : delegate_(delegate),
       upload_log_path_(upload_log_path),
-      worker_pool_(worker_pool) {}
+      task_runner_(std::move(task_runner)) {}
 
-UploadList::~UploadList() {}
+UploadList::~UploadList() = default;
 
 void UploadList::LoadUploadListAsynchronously() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  worker_pool_->PostTask(
-      FROM_HERE,
-      base::Bind(&UploadList::PerformLoadAndNotifyDelegate,
-                 this, base::ThreadTaskRunnerHandle::Get()));
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  task_runner_->PostTask(
+      FROM_HERE, base::Bind(&UploadList::PerformLoadAndNotifyDelegate, this,
+                            base::SequencedTaskRunnerHandle::Get()));
 }
 
 void UploadList::ClearDelegate() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   delegate_ = NULL;
 }
 
 void UploadList::PerformLoadAndNotifyDelegate(
-    const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
+    scoped_refptr<base::SequencedTaskRunner> task_runner) {
   std::vector<UploadInfo> uploads;
   LoadUploadList(&uploads);
   task_runner->PostTask(
@@ -141,7 +140,7 @@ void UploadList::ParseLogEntries(
 }
 
 void UploadList::SetUploadsAndNotifyDelegate(std::vector<UploadInfo> uploads) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   uploads_ = std::move(uploads);
   if (delegate_)
     delegate_->OnUploadListAvailable();
@@ -149,16 +148,16 @@ void UploadList::SetUploadsAndNotifyDelegate(std::vector<UploadInfo> uploads) {
 
 void UploadList::GetUploads(size_t max_count,
                             std::vector<UploadInfo>* uploads) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(sequence_checker_.CalledOnValidSequence());
   std::copy(uploads_.begin(),
             uploads_.begin() + std::min(uploads_.size(), max_count),
             std::back_inserter(*uploads));
 }
 
 void UploadList::RequestSingleCrashUploadAsync(const std::string& local_id) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
 #if defined(OS_WIN) || defined(OS_MACOSX)
-  DCHECK(thread_checker_.CalledOnValidThread());
-  worker_pool_->PostTask(
+  task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&UploadList::RequestSingleCrashUpload, this, local_id));
 #endif

components/upload_list/upload_list.h

@@ -13,12 +13,12 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/threading/thread_checker.h"
+#include "base/sequence_checker.h"
 #include "base/time/time.h"
 
 namespace base {
 class SequencedTaskRunner;
-class SequencedWorkerPool;
+class TaskRunner;
 }
 
 // Loads and parses an upload list text file of the format
@@ -84,7 +84,7 @@ class UploadList : public base::RefCountedThreadSafe<UploadList> {
   // Creates a new upload list with the given callback delegate.
   UploadList(Delegate* delegate,
              const base::FilePath& upload_log_path,
-             const scoped_refptr<base::SequencedWorkerPool>& worker_pool);
+             scoped_refptr<base::TaskRunner> task_runner);
 
   // Starts loading the upload list. OnUploadListAvailable will be called when
   // loading is complete.
@@ -118,7 +118,7 @@ class UploadList : public base::RefCountedThreadSafe<UploadList> {
 
   // Manages the background thread work for LoadUploadListAsynchronously().
   void PerformLoadAndNotifyDelegate(
-      const scoped_refptr<base::SequencedTaskRunner>& task_runner);
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
 
   // Calls the delegate's callback method, if there is a delegate. Stores
   // the newly loaded |uploads| into |uploads_| on the delegate's task runner.
@@ -128,15 +128,17 @@ class UploadList : public base::RefCountedThreadSafe<UploadList> {
   void ParseLogEntries(const std::vector<std::string>& log_entries,
                        std::vector<UploadInfo>* uploads);
 
-  // |thread_checker_| ensures that |uploads_| is only set from the task runner
-  // that created the UploadList.
-  base::ThreadChecker thread_checker_;
+  // Ensures that this class' thread unsafe state is only accessed from the
+  // sequence that owns this UploadList.
+  base::SequenceChecker sequence_checker_;
+
   std::vector<UploadInfo> uploads_;
+
   Delegate* delegate_;
 
   const base::FilePath upload_log_path_;
 
-  scoped_refptr<base::SequencedWorkerPool> worker_pool_;
+  const scoped_refptr<base::TaskRunner> task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(UploadList);
 };

components/upload_list/upload_list_unittest.cc

@@ -11,11 +11,13 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/test/sequenced_worker_pool_owner.h"
+#include "base/task_runner.h"
+#include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -29,12 +31,14 @@ const char kTestCaptureTime[] = "2345678901";
 class UploadListTest : public testing::Test,
                        public UploadList::Delegate {
  public:
-  UploadListTest() : worker_pool_owner_(1, "UploadListTest") {}
+  UploadListTest() : worker_thread_("UploadListTest") {}
 
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    ASSERT_TRUE(worker_thread_.Start());
   }
 
+ protected:
   void WriteUploadLog(const std::string& log_data) {
     ASSERT_GT(base::WriteFile(log_path(), log_data.c_str(),
                               static_cast<int>(log_data.size())),
@@ -53,9 +57,10 @@ class UploadListTest : public testing::Test,
     quit_closure_.Run();
   }
 
-  const scoped_refptr<base::SequencedWorkerPool> worker_pool() {
-    return worker_pool_owner_.pool();
+  scoped_refptr<base::TaskRunner> task_runner() const {
+    return worker_thread_.task_runner();
   }
+
   base::FilePath log_path() {
     return temp_dir_.GetPath().Append(FILE_PATH_LITERAL("uploads.log"));
   }
@@ -63,8 +68,10 @@ class UploadListTest : public testing::Test,
  private:
   base::MessageLoop message_loop_;
   base::ScopedTempDir temp_dir_;
-  base::SequencedWorkerPoolOwner worker_pool_owner_;
+  base::Thread worker_thread_;
   base::Closure quit_closure_;
+
+  DISALLOW_COPY_AND_ASSIGN(UploadListTest);
 };
 
 // These tests test that UploadList can parse a vector of log entry strings of
@@ -80,7 +87,7 @@ TEST_F(UploadListTest, ParseUploadTimeUploadId) {
   WriteUploadLog(test_entry);
 
   scoped_refptr<UploadList> upload_list =
-      new UploadList(this, log_path(), worker_pool());
+      new UploadList(this, log_path(), task_runner());
 
   upload_list->LoadUploadListAsynchronously();
   WaitForUploadList();
@@ -108,7 +115,7 @@ TEST_F(UploadListTest, ParseUploadTimeUploadIdLocalId) {
   WriteUploadLog(test_entry);
 
   scoped_refptr<UploadList> upload_list =
-      new UploadList(this, log_path(), worker_pool());
+      new UploadList(this, log_path(), task_runner());
 
   upload_list->LoadUploadListAsynchronously();
   WaitForUploadList();
@@ -137,7 +144,7 @@ TEST_F(UploadListTest, ParseUploadTimeUploadIdCaptureTime) {
   WriteUploadLog(test_entry);
 
   scoped_refptr<UploadList> upload_list =
-      new UploadList(this, log_path(), worker_pool());
+      new UploadList(this, log_path(), task_runner());
 
   upload_list->LoadUploadListAsynchronously();
   WaitForUploadList();
@@ -165,7 +172,7 @@ TEST_F(UploadListTest, ParseLocalIdCaptureTime) {
   WriteUploadLog(test_entry);
 
   scoped_refptr<UploadList> upload_list =
-      new UploadList(this, log_path(), worker_pool());
+      new UploadList(this, log_path(), task_runner());
 
   upload_list->LoadUploadListAsynchronously();
   WaitForUploadList();
@@ -197,7 +204,7 @@ TEST_F(UploadListTest, ParseUploadTimeUploadIdLocalIdCaptureTime) {
   WriteUploadLog(test_entry);
 
   scoped_refptr<UploadList> upload_list =
-      new UploadList(this, log_path(), worker_pool());
+      new UploadList(this, log_path(), task_runner());
 
   upload_list->LoadUploadListAsynchronously();
   WaitForUploadList();
@@ -229,7 +236,7 @@ TEST_F(UploadListTest, ParseMultipleEntries) {
   WriteUploadLog(test_entry);
 
   scoped_refptr<UploadList> upload_list =
-      new UploadList(this, log_path(), worker_pool());
+      new UploadList(this, log_path(), task_runner());
 
   upload_list->LoadUploadListAsynchronously();
   WaitForUploadList();
@@ -266,7 +273,7 @@ TEST_F(UploadListTest, ParseWithState) {
   WriteUploadLog(test_entry);
 
   scoped_refptr<UploadList> upload_list =
-      new UploadList(this, log_path(), worker_pool());
+      new UploadList(this, log_path(), task_runner());
 
   upload_list->LoadUploadListAsynchronously();
   WaitForUploadList();
@@ -298,7 +305,7 @@ TEST_F(UploadListTest, SimultaneousAccess) {
   WriteUploadLog(test_entry);
 
   scoped_refptr<UploadList> upload_list =
-      new UploadList(this, log_path(), worker_pool());
+      new UploadList(this, log_path(), task_runner());
 
   // Queue up a bunch of loads, waiting only for the first one to complete.
   // Clearing the delegate prevents the QuitClosure from being Run more than