Split the RequestPicker task into two separate tasks.

components/offline_pages/background/request_coordinator.cc

 // Copyright 2016 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 "components/offline_pages/background/request_coordinator.h"
 
 #include <limits>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 163 matching lines @@
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
       policy_controller_(new ClientPolicyController()),
       network_quality_estimator_(network_quality_estimator),
       active_request_(nullptr),
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
       scheduler_callback_(base::Bind(&EmptySchedulerCallback)),
       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() {}
 
 int64_t RequestCoordinator::SavePageLater(const GURL& url,
                                           const ClientId& client_id,
                                           bool user_requested,
                                           RequestAvailability availability) {
   DVLOG(2) << "URL is " << url << " " << __func__;
 
@@ skipping 411 matching lines @@
     // TODO: Make sure the scheduler callback is valid before running it.
     scheduler_callback_.Run(true);
     DVLOG(2) << " out of time, giving up. " << __func__;
 
     return;
   }
 
   // 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,
                  weak_ptr_factory_.GetWeakPtr(), is_start_of_processing),
       *current_conditions_.get(), disabled_requests_);
   // TODO(petewil): Verify current_conditions has a good value on all calling
   // paths.  It is really more of a "last known conditions" than "current
   // conditions".  Consider having a call to Java to check the current
   // conditions.
 }
 
 // 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;
 
   // Make sure we were not stopped while picking.
   if (processing_state_ != ProcessingWindowState::STOPPED) {
     // 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;
 
   // Clear the outstanding "safety" task in the scheduler.
   scheduler_->Unschedule();
 
   // If disabled tasks remain, post a new safety task for 5 sec from now.
   if (disabled_requests_.size() > 0) {
     scheduler_->BackupSchedule(GetTriggerConditions(kUserRequest),
                                kDisabledTaskRecheckSeconds);
   } else if (non_user_requested_tasks_remaining) {
     // If we don't have any of those, check for non-user-requested tasks.
     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);
 }
 
 void RequestCoordinator::RequestCounts(bool is_start_of_processing,
                                        size_t total_requests,
                                        size_t available_requests) {
   // Only capture request counts for the start of processing (not for
   // continued processing in the same window).
   if (!is_start_of_processing)
@@ skipping 286 matching lines @@
 
 ClientPolicyController* RequestCoordinator::GetPolicyController() {
   return policy_controller_.get();
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages