Added DownloadItem::Observer to JobController.

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 fda6f10daef8c00e728df2087ebddd158b73a4d0..ceae868f8f4e3f0678540d930f7a5980000075bc 100644
--- a/content/browser/background_fetch/background_fetch_job_controller.cc
+++ b/content/browser/background_fetch/background_fetch_job_controller.cc
@@ -24,14 +24,17 @@ BackgroundFetchJobController::BackgroundFetchJobController(
     BackgroundFetchJobData* job_data)
     : browser_context_(browser_context),
       storage_partition_(storage_partition),
-      job_data_(base::WrapUnique(job_data)) {}
+      job_data_(base::WrapUnique(job_data)),
+      weak_ptr_factory_(this) {}
 
-BackgroundFetchJobController::~BackgroundFetchJobController() = default;
+BackgroundFetchJobController::~BackgroundFetchJobController() {
+  StopObservations();

[Peter Beverloo, 2017/03/08 14:58:19]

We should either call this from the destructor, or from the Shutdown() method,
not both. (Shutdown() should semantically invalidate this class already.)

[harkness, 2017/03/10 13:33:53]

Done.

+}
 
 void BackgroundFetchJobController::Shutdown() {
-  // TODO(harkness): Remove any observers.
   // TODO(harkness): Write final status to the DataManager. After this call, the
   // |job_data_| is no longer valid.
+  StopObservations();
   job_data_.reset(nullptr);

[Peter Beverloo, 2017/03/08 14:58:19]

nit: drop `nullptr`

[harkness, 2017/03/10 13:33:53]

Done.

 }
 
@@ -43,22 +46,80 @@ void BackgroundFetchJobController::StartProcessing() {
       job_data_->GetNextBackgroundFetchRequestInfo();
   ProcessRequest(fetch_request);
 
-  // Currently this only supports jobs of size 1.
-  // TODO(crbug.com/692544)
-  DCHECK(!job_data_->HasRequestsRemaining());
+  // Currently, this processes a single request at a time.
+}
+
+void BackgroundFetchJobController::DownloadStarted(
+    const std::string& request_guid,
+    DownloadItem* item,
+    DownloadInterruptReason reason) {
+  // Start observing the DownloadItem. No need to store the pointer because it
+  // can be retrieved from the DownloadManager.
+  item->AddObserver(this);
+  download_guid_map_[item->GetGuid()] = request_guid;
+  job_data_->SetRequestDownloadGuid(request_guid, item->GetGuid());
+}
+
+void BackgroundFetchJobController::OnDownloadUpdated(DownloadItem* item) {
+  DCHECK(download_guid_map_.find(item->GetGuid()) != download_guid_map_.end());
+
+  bool requests_remaining = false;
+  switch (item->GetState()) {
+    case DownloadItem::DownloadState::CANCELLED:
+    // TODO(harkness): Expand the "complete" flag on the RequestInfo to
+    // include error states.
+    case DownloadItem::DownloadState::COMPLETE:
+      requests_remaining = job_data_->BackgroundFetchRequestInfoComplete(
+          download_guid_map_[item->GetGuid()]);
+      // TODO(harkness): Tell the notification service to update the download
+      // notification.
+
+      if (requests_remaining) {
+        ProcessRequest(job_data_->GetNextBackgroundFetchRequestInfo());
+      } else if (job_data_->IsComplete()) {
+        // TODO(harkness): Return the data to the context.
+      }
+      return;
+    case DownloadItem::DownloadState::INTERRUPTED:
+      // TODO(harkness): Just update the notification that it is paused.
+      return;
+    default:

[Peter Beverloo, 2017/03/08 14:58:19]

Let's drop `default`, and instead add empty cases for the other DownloadStates.
This will mean that we get compile-time coverage for making sure we handle
everything.

case DownloadItem::DownloadState::IN_PROGRESS:
case DownloadItem::DownloadState::MAX_DOWNLOAD_STATE:
  break;

(I'd personally also s/return/break/ here and above.)

[harkness, 2017/03/10 13:33:53]

Done.

+      return;
+  }
+}
+
+void BackgroundFetchJobController::OnDownloadDestroyed(DownloadItem* item) {
+  // This can be called as part of Shutdown. Make sure the observers are
+  // removed.
+  item->RemoveObserver(this);
+}
+
+void BackgroundFetchJobController::StopObservations() {
+  DownloadManager* manager =
+      BrowserContext::GetDownloadManager(browser_context_);
+  // For any downloads in progress, remove the observer.
+  for (auto& guid_map_entry : download_guid_map_) {
+    DownloadItem* item = manager->GetDownloadByGuid(guid_map_entry.first);
+    if (item)
+      item->RemoveObserver(this);
+  }
+  download_guid_map_.clear();
 }
 
 void BackgroundFetchJobController::ProcessRequest(
     const BackgroundFetchRequestInfo& fetch_request) {
-  // TODO(harkness): Start the observer watching for this download.
   // TODO(harkness): Check if the download is already in progress or completed.
 
-  // Send the fetch request to the DownloadManager.
+  // Set up the download parameters and the OnStartedCallback.
   std::unique_ptr<DownloadUrlParameters> params(
       base::MakeUnique<DownloadUrlParameters>(
           fetch_request.url(), storage_partition_->GetURLRequestContext()));
-  // TODO(harkness): Currently this is the only place the browser_context is
-  // used. Evaluate whether we can just pass in the download manager explicitly.
+  DownloadUrlParameters::OnStartedCallback on_started_callback =

[Peter Beverloo, 2017/03/08 14:58:19]

nit: I'd use `auto` here. The variable name is clear enough, and the compiler
will warn us if there's a type mismatch. In fact, I'd just inline it in the call
below.

[harkness, 2017/03/10 13:33:53]

Done.

+      base::Bind(&BackgroundFetchJobController::DownloadStarted,
+                 weak_ptr_factory_.GetWeakPtr(), fetch_request.guid());
+  params->set_callback(on_started_callback);
+
+  // Send the fetch request to the DownloadManager.

[Peter Beverloo, 2017/03/08 14:58:19]

nit: I don't think this adds any value.

[harkness, 2017/03/10 13:33:53]

Removed.

   BrowserContext::GetDownloadManager(browser_context_)
       ->DownloadUrl(std::move(params));
 }