[Background Fetch] Tidy up getting/activating pending requests

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 1032fa2e70bcb75cc3dc4bb7213a26aa07c25d77..e7df5debb92fd580d15b953968c3b3609fd676dd 100644
--- a/content/browser/background_fetch/background_fetch_job_controller.cc
+++ b/content/browser/background_fetch/background_fetch_job_controller.cc
@@ -260,22 +260,33 @@ BackgroundFetchJobController::~BackgroundFetchJobController() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 };
 
-void BackgroundFetchJobController::Start(
-    std::vector<scoped_refptr<BackgroundFetchRequestInfo>> initial_requests) {
+void BackgroundFetchJobController::Start() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK_LE(initial_requests.size(), kMaximumBackgroundFetchParallelRequests);
   DCHECK_EQ(state_, State::INITIALIZED);
 
   state_ = State::FETCHING;
 
-  for (const auto& request : initial_requests)
-    StartRequest(request);
+  // TODO(johnme): Enforce kMaximumBackgroundFetchParallelRequests globally
+  // and/or per origin rather than per fetch (crbug.com/741609).

[Peter Beverloo, 2017/07/12 13:30:50]

note: TODO(crbug.com/123456) is allowed syntax, and probably more appropriate in
this case

[johnme, 2017/07/12 14:13:58]

Done.

+  for (size_t i = 0; i < kMaximumBackgroundFetchParallelRequests; i++) {
+    data_manager_->PopNextRequest(
+        registration_id_,
+        base::BindOnce(&BackgroundFetchJobController::StartRequest,
+                       weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 void BackgroundFetchJobController::StartRequest(
     scoped_refptr<BackgroundFetchRequestInfo> request) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_EQ(state_, State::FETCHING);
+  if (!request) {
+    // This can happen when |Start| tries to start multiple initial requests,
+    // but the fetch does not contain that many pending requests; or when
+    // |DidMarkRequestCompleted| tries to start the next request but there are
+    // none left.
+    return;
+  }
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&Core::StartRequest, ui_core_ptr_,
                                      std::move(request), traffic_annotation_));
@@ -295,34 +306,31 @@ void BackgroundFetchJobController::DidCompleteRequest(
 
   // 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_++;
-
-  data_manager_->MarkRequestAsCompleteAndGetNextRequest(
+  data_manager_->MarkRequestAsComplete(
       registration_id_, request.get(),
-      base::BindOnce(&BackgroundFetchJobController::DidGetNextRequest,
+      base::BindOnce(&BackgroundFetchJobController::DidMarkRequestCompleted,
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
-void BackgroundFetchJobController::DidGetNextRequest(
-    scoped_refptr<BackgroundFetchRequestInfo> request) {
+void BackgroundFetchJobController::DidMarkRequestCompleted(
+    bool has_pending_or_active_requests) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK_LE(pending_completed_file_acknowledgements_, 1);
-  pending_completed_file_acknowledgements_--;
+  DCHECK_EQ(state_, State::FETCHING);
 
-  // If a |request| has been given, start downloading the file and bail.
-  if (request) {
-    StartRequest(std::move(request));
+  // If not all requests have completed, start a pending request if there are
+  // any left, and bail.
+  if (has_pending_or_active_requests) {
+    data_manager_->PopNextRequest(
+        registration_id_,
+        base::BindOnce(&BackgroundFetchJobController::StartRequest,
+                       weak_ptr_factory_.GetWeakPtr()));
     return;
   }
 
-  // If there are outstanding completed file acknowlegements, bail as well.
-  if (pending_completed_file_acknowledgements_ > 0)
-    return;
-
-  state_ = State::COMPLETED;
-
   // Otherwise the job this controller is responsible for has completed.
+  state_ = State::COMPLETED;
   std::move(completed_callback_).Run(this);
+  completed_callback_.Reset();
 }
 
 void BackgroundFetchJobController::UpdateUI(const std::string& title) {
@@ -333,13 +341,14 @@ void BackgroundFetchJobController::UpdateUI(const std::string& title) {
 
 void BackgroundFetchJobController::Abort() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_EQ(state_, State::FETCHING);

[Peter Beverloo, 2017/07/12 13:30:50]

Hmm. Might we end up with a scheduling solution where we won't immediately start
background fetch jobs, making it possible (and valid!) to call Abort() prior to
Start()?

[johnme, 2017/07/12 14:13:58]

Good point. Changed this to just early out if state is completed.

 
   // TODO(harkness): Abort all in-progress downloads.
 
   state_ = State::ABORTED;
-
-  // Inform the owner of the controller about the job having completed.
+  // Inform the owner of the controller about the job having aborted.
   std::move(completed_callback_).Run(this);
+  completed_callback_.Reset();

[Peter Beverloo, 2017/07/12 13:30:50]

delete, that's what std::move() will conceptually do. on line 333 too.

[johnme, 2017/07/12 14:13:58]

Done.

 }
 
 }  // namespace content