[Background Fetch] Tidy up getting/activating pending requests

content/browser/background_fetch/background_fetch_job_controller.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/background_fetch/background_fetch_job_controller.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 242 matching lines @@
   // be unsafe to obtain a weak pointer on the IO thread from a factory that
   // lives on the UI thread, but it's ok in this constructor since the Core
   // can't be destroyed before this constructor finishes.
   ui_core_ptr_ = ui_core_->GetWeakPtrOnUI();
 }
 
 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.

[Peter Beverloo, 2017/07/11 16:10:55]

Please file a bug for this - scheduling will be a huge task by itself, since a
holistic solution requires insight in what other users of the download service
are doing as well.

[johnme, 2017/07/12 13:13:10]

Done.

+  for (size_t i = 0; i < kMaximumBackgroundFetchParallelRequests; i++) {
+    data_manager_->ActivateNextRequest(
+        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)
+    return;  // No more pending requests to start.

[Peter Beverloo, 2017/07/11 16:10:55]

Please document that this bail-out is only applicable for the initial set of
requests.

[johnme, 2017/07/12 13:13:10]

Done.

   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&Core::StartRequest, ui_core_ptr_,
                                      std::move(request), traffic_annotation_));
 }
 
 void BackgroundFetchJobController::DidStartRequest(
     scoped_refptr<BackgroundFetchRequestInfo> request,
     const std::string& download_guid) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   data_manager_->MarkRequestAsStarted(registration_id_, request.get(),
                                       download_guid);
 }
 
 void BackgroundFetchJobController::DidCompleteRequest(
     scoped_refptr<BackgroundFetchRequestInfo> request) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // 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_->MarkRequestAsCompleteAndActivateNextRequest(

[Peter Beverloo, 2017/07/11 16:10:55]

What about splitting up MarkRequestAsComplete so that {Activate,Get}NextRequest
can be used from both sources?

[johnme, 2017/07/12 13:13:10]

Done (yeah, I'd been considering that; initially I was reluctant because it made
the pending_completed_file_acknowledgements_ logic more complicated, but I've
since realised that the pending_completed_file_acknowledgements_ logic was
brittle and incorrect anyway, so I've gotten rid of it and now just check if any
requests are yet to complete).

       registration_id_, request.get(),
       base::BindOnce(&BackgroundFetchJobController::DidGetNextRequest,
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BackgroundFetchJobController::DidGetNextRequest(
     scoped_refptr<BackgroundFetchRequestInfo> request) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_LE(pending_completed_file_acknowledgements_, 1);
   pending_completed_file_acknowledgements_--;
@@ skipping 25 matching lines @@
 
   // TODO(harkness): Abort all in-progress downloads.
 
   state_ = State::ABORTED;
 
   // Inform the owner of the controller about the job having completed.
   std::move(completed_callback_).Run(this);
 }
 
 }  // namespace content