[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..9aeaf65772f89b1f4bc596720e905defd32d74bc 100644
--- a/content/browser/background_fetch/background_fetch_job_controller.cc
+++ b/content/browser/background_fetch/background_fetch_job_controller.cc
@@ -260,22 +260,28 @@ 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_));
@@ -297,7 +303,7 @@ void BackgroundFetchJobController::DidCompleteRequest(
   // 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()));