Migrate part of the BackgroundFetchJobController to the UI thread

content/browser/background_fetch/background_fetch_job_controller.cc

Index: content/browser/background_fetch/background_fetch_job_controller.cc
diff --git a/content/browser/background_fetch/background_fetch_job_controller.cc b/content/browser/background_fetch/background_fetch_job_controller.cc
index 60e3cb0a71da3a4c83647cadb3e50edd7887cd8b..a776163e6cdf35f5f1685feda3a928e2b545778c 100644
--- a/content/browser/background_fetch/background_fetch_job_controller.cc
+++ b/content/browser/background_fetch/background_fetch_job_controller.cc
@@ -15,33 +15,167 @@
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/storage_partition.h"
+#include "net/url_request/url_request_context_getter.h"
 
 namespace content {
 
+// Internal functionality of the BackgroundFetchJobController that lives on the
+// UI thread, where all interaction with the download manager must happen.
+class BackgroundFetchJobController::Core : public DownloadItem::Observer {

[harkness, 2017/03/29 22:31:10]

I don't have any objections to this approach, but I'm curious why you chose to
have the Core class which lives entirely on the UI thread rather than the
StartRequestOnUI/StartRequestOnIO approach that I see other places.

[Peter Beverloo, 2017/03/29 22:37:56]

This is what we do with push. It creates a cleaner separation between the
threads - the methods and data are now clearly separated, and it's always clear
on which thread something should be called.

+ public:
+  Core(const base::WeakPtr<BackgroundFetchJobController>& io_parent,
+       DownloadManager* download_manager,
+       scoped_refptr<net::URLRequestContextGetter> request_context)
+      : io_parent_(io_parent),
+        download_manager_(download_manager),
+        request_context_(std::move(request_context)),
+        weak_ptr_factory_(this) {}
+
+  ~Core() final {
+    for (const auto& pair : downloads_)
+      pair.first->RemoveObserver(this);
+  }
+
+  // Returns a weak pointer that can be used to talk to |this|.
+  base::WeakPtr<Core> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); }

[harkness, 2017/03/29 22:31:10]

nit: Never called?

[Peter Beverloo, 2017/03/29 22:37:56]

BackgroundFetchJobController::ctor calls it

+
+  // Starts fetching the |request| with the download manager.
+  void StartRequest(const BackgroundFetchRequestInfo& request) {
+    DCHECK(download_manager_);
+    DCHECK(request_context_);
+
+    std::unique_ptr<DownloadUrlParameters> download_parameters(
+        base::MakeUnique<DownloadUrlParameters>(request.GetURL(),
+                                                request_context_.get()));
+
+    // TODO(peter): The |download_parameters| should be populated with all the
+    // properties set in the |request|'s ServiceWorkerFetchRequest member.
+
+    download_parameters->set_callback(base::Bind(
+        &Core::DidStartRequest, weak_ptr_factory_.GetWeakPtr(), request));
+
+    download_manager_->DownloadUrl(std::move(download_parameters));
+  }
+
+  // DownloadItem::Observer overrides:
+  void OnDownloadUpdated(DownloadItem* item) override {
+    auto iter = downloads_.find(item);
+    DCHECK(iter != downloads_.end());
+
+    const BackgroundFetchRequestInfo& request = iter->second;
+
+    switch (item->GetState()) {
+      case DownloadItem::DownloadState::COMPLETE:
+        // TODO(peter): Populate the responses' information in the |request|.
+
+        item->RemoveObserver(this);
+
+        // Inform the host about |host| having completed.
+        BrowserThread::PostTask(
+            BrowserThread::IO, FROM_HERE,
+            base::Bind(&BackgroundFetchJobController::DidCompleteRequest,
+                       io_parent_, request));
+
+        // Clear the local state for the |request|, it no longer is our concern.
+        downloads_.erase(iter);
+        break;
+      case DownloadItem::DownloadState::CANCELLED:
+        // TODO(harkness): Consider how we want to handle cancelled downloads.
+        break;
+      case DownloadItem::DownloadState::INTERRUPTED:
+        // TODO(harkness): Just update the notification that it is paused.
+        break;
+      case DownloadItem::DownloadState::IN_PROGRESS:
+        // TODO(harkness): If the download was previously paused, this should
+        // now unpause the notification.
+        break;
+      case DownloadItem::DownloadState::MAX_DOWNLOAD_STATE:
+        NOTREACHED();
+        break;
+    }
+  }
+
+  void OnDownloadDestroyed(DownloadItem* item) override {
+    DCHECK_EQ(downloads_.count(item), 1u);
+    downloads_.erase(item);
+
+    item->RemoveObserver(this);
+  }
+
+ private:
+  // Called when the download manager has started the given |request|. The
+  // |download_item| continues to be owned by the download system. The
+  // |interrupt_reason| will indicate when a request could not be started.
+  void DidStartRequest(const BackgroundFetchRequestInfo& request,
+                       DownloadItem* download_item,
+                       DownloadInterruptReason interrupt_reason) {
+    DCHECK_EQ(interrupt_reason, DOWNLOAD_INTERRUPT_REASON_NONE);
+    DCHECK(download_item);
+
+    // TODO(peter): The above two DCHECKs are assumptions our implementation
+    // currently makes, but are not fit for production. We need to handle such
+    // failures gracefully.
+
+    // Register for updates on the download's progress.
+    download_item->AddObserver(this);
+
+    // Inform the host about the |request| having started.
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&BackgroundFetchJobController::DidStartRequest, io_parent_,
+                   request, download_item->GetGuid()));
+
+    // Associate the |download_item| with the |request| so that we can retrieve
+    // it's information when further updates happen.
+    downloads_.insert(std::make_pair(download_item, request));
+  }
+
+  // Weak reference to the BackgroundFetchJobController instance that owns us.
+  base::WeakPtr<BackgroundFetchJobController> io_parent_;
+
+  // The DownloadManager that Background Fetch dispatches downloads to.
+  // The instance is owned by the profile.
+  DownloadManager* download_manager_;
+
+  // The URL request context to use when issuing the requests.
+  scoped_refptr<net::URLRequestContextGetter> request_context_;
+
+  // Map from DownloadItem* to the request info for the in-progress downloads.
+  std::unordered_map<DownloadItem*, BackgroundFetchRequestInfo> downloads_;
+
+  base::WeakPtrFactory<Core> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
 BackgroundFetchJobController::BackgroundFetchJobController(
     const BackgroundFetchRegistrationId& registration_id,
     const BackgroundFetchOptions& options,
-    DownloadManager* download_manager,
-    StoragePartition* storage_partition,
     BackgroundFetchDataManager* data_manager,
+    DownloadManager* download_manager,
+    scoped_refptr<net::URLRequestContextGetter> request_context,
     CompletedCallback completed_callback)
     : registration_id_(registration_id),
       options_(options),
-      download_manager_(download_manager),
-      storage_partition_(storage_partition),
       data_manager_(data_manager),
       completed_callback_(std::move(completed_callback)),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-BackgroundFetchJobController::~BackgroundFetchJobController() {
-  // TODO(harkness): Write final status to the DataManager.
+  // Create the core, containing the internal functionality that will have to
+  // be run on the UI thread. It will respond to this class with a weak pointer.
+  ui_core_.reset(new Core(weak_ptr_factory_.GetWeakPtr(), download_manager,
+                          std::move(request_context)));

[harkness, 2017/03/29 22:31:10]

why not MakeUnique?

[Peter Beverloo, 2017/03/29 22:37:56]

Because our unique_ptr has deletion traits. (Core will be deleted on the UI
thread as opposed to the IO thread.)

 
-  for (const auto& pair : downloads_)
-    pair.first->RemoveObserver(this);
+  // Get a WeakPtr over which we can talk to the |ui_core_|.
+  ui_core_ptr_ = ui_core_->GetWeakPtr();
 }
 
+BackgroundFetchJobController::~BackgroundFetchJobController() = default;
+
 void BackgroundFetchJobController::Start(
     std::vector<BackgroundFetchRequestInfo> initial_requests) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_LE(initial_requests.size(), kMaximumBackgroundFetchParallelRequests);
   DCHECK_EQ(state_, State::INITIALIZED);
 
@@ -53,92 +187,32 @@ void BackgroundFetchJobController::Start(
 
 void BackgroundFetchJobController::StartRequest(
     const BackgroundFetchRequestInfo& request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_EQ(state_, State::FETCHING);
-
-  std::unique_ptr<DownloadUrlParameters> download_parameters(
-      base::MakeUnique<DownloadUrlParameters>(
-          request.GetURL(), storage_partition_->GetURLRequestContext()));
-
-  // TODO(peter): The |download_parameters| should be populated with all the
-  // properties set in the |request|'s ServiceWorkerFetchRequest member.
-
-  download_parameters->set_callback(
-      base::Bind(&BackgroundFetchJobController::DidStartRequest,
-                 weak_ptr_factory_.GetWeakPtr(), request));
-
-  download_manager_->DownloadUrl(std::move(download_parameters));
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&Core::StartRequest, ui_core_ptr_, request));
 }
 
 void BackgroundFetchJobController::DidStartRequest(
     const BackgroundFetchRequestInfo& request,
-    DownloadItem* download_item,
-    DownloadInterruptReason interrupt_reason) {
-  DCHECK_EQ(interrupt_reason, DOWNLOAD_INTERRUPT_REASON_NONE);
-  DCHECK(download_item);
-
-  // Update the |request|'s download GUID in the DataManager.
-  data_manager_->MarkRequestAsStarted(registration_id_, request,
-                                      download_item->GetGuid());
-
-  // Register for updates on the download's progress.
-  download_item->AddObserver(this);
-
-  // Associate the |download_item| with the |request| so that we can retrieve
-  // it's information when further updates happen.
-  downloads_.insert(std::make_pair(download_item, request));
-}
-
-void BackgroundFetchJobController::UpdateUI(const std::string& title) {
-  // TODO(harkness): Update the user interface with |title|.
+    const std::string& download_guid) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  data_manager_->MarkRequestAsStarted(registration_id_, request, download_guid);
 }
 
-void BackgroundFetchJobController::Abort() {
-  // TODO(harkness): Abort all in-progress downloads.
-
-  state_ = State::ABORTED;
+void BackgroundFetchJobController::DidCompleteRequest(
+    const BackgroundFetchRequestInfo& request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // Inform the owner of the controller about the job having completed.
-  std::move(completed_callback_).Run(this);
-}
+  // The DataManager must acknowledge that it stored the data and that there are
+  // no more pending requests to avoid marking this job as completed too early.
+  pending_completed_file_acknowledgements_++;
 
-void BackgroundFetchJobController::OnDownloadUpdated(DownloadItem* item) {
-  auto iter = downloads_.find(item);
-  DCHECK(iter != downloads_.end());
-
-  const BackgroundFetchRequestInfo& request = iter->second;
-
-  switch (item->GetState()) {
-    case DownloadItem::DownloadState::COMPLETE:
-      // TODO(peter): Populate the responses' information in the |request|.
-
-      // Remove the |item| from the list of active downloads. Expect one more
-      // completed file acknowledgement too, to prevent race conditions.
-      pending_completed_file_acknowledgements_++;
-      downloads_.erase(iter);
-
-      item->RemoveObserver(this);
-
-      // Mark the |request| as having completed and fetch the next request from
-      // storage. This may also mean that we've completed the job.
-      data_manager_->MarkRequestAsCompleteAndGetNextRequest(
-          registration_id_, request,
-          base::BindOnce(&BackgroundFetchJobController::DidGetNextRequest,
-                         weak_ptr_factory_.GetWeakPtr()));
-      break;
-    case DownloadItem::DownloadState::CANCELLED:
-      // TODO(harkness): Consider how we want to handle cancelled downloads.
-      break;
-    case DownloadItem::DownloadState::INTERRUPTED:
-      // TODO(harkness): Just update the notification that it is paused.
-      break;
-    case DownloadItem::DownloadState::IN_PROGRESS:
-      // TODO(harkness): If the download was previously paused, this should now
-      // unpause the notification.
-      break;
-    case DownloadItem::DownloadState::MAX_DOWNLOAD_STATE:
-      NOTREACHED();
-      break;
-  }
+  data_manager_->MarkRequestAsCompleteAndGetNextRequest(
+      registration_id_, request,
+      base::BindOnce(&BackgroundFetchJobController::DidGetNextRequest,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BackgroundFetchJobController::DidGetNextRequest(
@@ -162,11 +236,21 @@ void BackgroundFetchJobController::DidGetNextRequest(
   std::move(completed_callback_).Run(this);
 }
 
-void BackgroundFetchJobController::OnDownloadDestroyed(DownloadItem* item) {
-  DCHECK_EQ(downloads_.count(item), 1u);
-  downloads_.erase(item);
+void BackgroundFetchJobController::UpdateUI(const std::string& title) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // TODO(harkness): Update the user interface with |title|.
+}
+
+void BackgroundFetchJobController::Abort() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // TODO(harkness): Abort all in-progress downloads.
 
-  item->RemoveObserver(this);
+  state_ = State::ABORTED;
+
+  // Inform the owner of the controller about the job having completed.
+  std::move(completed_callback_).Run(this);
 }
 
 }  // namespace content