Split the RequestPicker task into two separate tasks.

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index 8dcc4d94def87f3cd13e2d2d56eefe69908e5153..bfd747d0401852bccaaa2987d0fdd3eedbeb1d02 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -181,9 +181,11 @@ RequestCoordinator::RequestCoordinator(
       immediate_schedule_callback_(base::Bind(&EmptySchedulerCallback)),
       weak_ptr_factory_(this) {
   DCHECK(policy_ != nullptr);
-  std::unique_ptr<PickRequestTaskFactory> picker_factory(
-      new PickRequestTaskFactory(policy_.get(), this, &event_logger_));
-  queue_->SetPickerFactory(std::move(picker_factory));
+  std::unique_ptr<CleanupTaskFactory> cleanup_factory(
+      new CleanupTaskFactory(policy_.get(), this, &event_logger_));
+  queue_->SetCleanupFactory(std::move(cleanup_factory));
+  // Do a cleanup at startup time.
+  queue_->CleanupRequestQueue();
 }
 
 RequestCoordinator::~RequestCoordinator() {}
@@ -615,8 +617,8 @@ void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
   // Ask request queue to make a new PickRequestTask object, then put it on the
   // task queue.
   queue_->PickNextRequest(
-      base::Bind(&RequestCoordinator::RequestPicked,
-                 weak_ptr_factory_.GetWeakPtr()),
+      policy_.get(), base::Bind(&RequestCoordinator::RequestPicked,
+                                weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&RequestCoordinator::RequestNotPicked,
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&RequestCoordinator::RequestCounts,
@@ -629,7 +631,8 @@ void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
 }
 
 // Called by the request picker when a request has been picked.
-void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
+void RequestCoordinator::RequestPicked(const SavePageRequest& request,
+                                       const bool cleanup_needed) {

[fgorski, 2016/12/02 21:44:04]

const bool -> bool here as well.

[Pete Williamson, 2016/12/05 20:39:45]

Done.

   DVLOG(2) << request.url() << " " << __func__;
   is_starting_ = false;
 
@@ -638,10 +641,16 @@ void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
     // Send the request on to the offliner.
     SendRequestToOffliner(request);
   }
+
+  // Schedule a queue cleanup if needed.
+  // TODO(petewil): What if the request we chose expires before calling cleanup?

[dougarnett, 2016/12/02 17:20:27]

Might consider adding some delta over expiration time to cleanup task check (eg,
policy immediate request timeout value) whereas picker task won't pick over the
expiration time.

[Pete Williamson, 2016/12/05 20:39:45]

Instead, to be sure, I pass in the request_id we sent to the offliner, so that
we won't clean it up while offlining.  Timeout values are always fragile.

+  if (cleanup_needed)
+    queue_->CleanupRequestQueue();
 }
 
 void RequestCoordinator::RequestNotPicked(
-    bool non_user_requested_tasks_remaining) {
+    bool non_user_requested_tasks_remaining,
+    const bool cleanup_needed) {

[fgorski, 2016/12/02 21:44:04]

const bool -> bool 

[Pete Williamson, 2016/12/05 20:39:45]

Done.

   DVLOG(2) << __func__;
   is_starting_ = false;
 
@@ -657,6 +666,10 @@ void RequestCoordinator::RequestNotPicked(
     scheduler_->Schedule(GetTriggerConditions(!kUserRequest));
   }
 
+  // Schedule a queue cleanup if needed.
+  if (cleanup_needed)
+    queue_->CleanupRequestQueue();
+
   // Let the scheduler know we are done processing.
   scheduler_callback_.Run(true);
 }