[Offline Pages] Improve RequestCoordinator state tracking.

components/offline_pages/core/background/request_coordinator.cc

Index: components/offline_pages/core/background/request_coordinator.cc
diff --git a/components/offline_pages/core/background/request_coordinator.cc b/components/offline_pages/core/background/request_coordinator.cc
index 8060d6af4ab0678672c381b76ee821aabf821870..3b887ce979ee8546dba22cee61819df67eccf296 100644
--- a/components/offline_pages/core/background/request_coordinator.cc
+++ b/components/offline_pages/core/background/request_coordinator.cc
@@ -197,8 +197,7 @@ RequestCoordinator::RequestCoordinator(
     net::NetworkQualityEstimator::NetworkQualityProvider*
         network_quality_estimator)
     : is_low_end_device_(base::SysInfo::IsLowEndDevice()),
-      is_busy_(false),
-      is_starting_(false),
+      state_(RequestCoordinatorState::IDLE),
       processing_state_(ProcessingWindowState::STOPPED),
       use_test_device_conditions_(false),
       offliner_(std::move(offliner)),
@@ -287,7 +286,7 @@ void RequestCoordinator::GetQueuedRequestsCallback(
 void RequestCoordinator::StopPrerendering(
     const Offliner::CancelCallback& final_callback,
     Offliner::RequestStatus stop_status) {
-  if (offliner_ && is_busy_) {
+  if (offliner_ && state_ == RequestCoordinatorState::OFFLINING) {
     DCHECK(active_request_.get());
     offliner_->Cancel(base::Bind(
         &RequestCoordinator::HandleCancelUpdateStatusCallback,
@@ -555,7 +554,7 @@ void RequestCoordinator::UpdateStatusForCancel(
     RecordOfflinerResultUMA(active_request_->client_id(),
                             active_request_->creation_time(),
                             last_offlining_status_);
-    is_busy_ = false;
+    state_ = RequestCoordinatorState::IDLE;
     active_request_.reset();
   }
 }
@@ -624,7 +623,7 @@ bool RequestCoordinator::StartImmediateProcessing(
 bool RequestCoordinator::StartProcessingInternal(
     const ProcessingWindowState processing_state,
     const base::Callback<void(bool)>& callback) {
-  if (is_starting_ || is_busy_)
+  if (state_ != RequestCoordinatorState::IDLE)
     return false;
   processing_state_ = processing_state;
   scheduler_callback_ = callback;
@@ -647,7 +646,7 @@ RequestCoordinator::TryImmediateStart(
     const base::Callback<void(bool)>& callback) {
   DVLOG(2) << "Immediate " << __func__;
   // Make sure not already busy processing.
-  if (is_busy_)
+  if (state_ == RequestCoordinatorState::OFFLINING)
     return OfflinerImmediateStartStatus::BUSY;
 
   // Make sure we are not on svelte device to start immediately.
@@ -702,7 +701,7 @@ void RequestCoordinator::UpdateCurrentConditionsFromAndroid() {
 }
 
 void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
-  is_starting_ = true;
+  state_ = RequestCoordinatorState::PICKING;
 
   // If this is the first call, the device conditions are current, no need to
   // update them.
@@ -737,7 +736,7 @@ void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
   if (connection_type ==
           net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE ||
       (base::Time::Now() - operation_start_time_) > processing_time_budget) {
-    is_starting_ = false;
+    state_ = RequestCoordinatorState::IDLE;
 
     // If we were doing immediate processing, try to start it again
     // when we get connected.
@@ -773,11 +772,18 @@ void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
 void RequestCoordinator::RequestPicked(const SavePageRequest& request,
                                        bool cleanup_needed) {
   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.
+  state_ = RequestCoordinatorState::OFFLINING;
+
+  // If we were stopped while picking or offliner got killed then reset the
+  // state of request coordinator, otherwise send the request to offliner.
+  if (processing_state_ == ProcessingWindowState::STOPPED || !offliner_) {
+    if (!offliner_) {

[Pete Williamson, 2017/03/28 17:12:43]

Let's remove this if !offliner check from both if statements, and the TODO.  We
were originally concerned that the offliner pointer could be null if the
offliner crashed, but that doesn't happen (it just points at garbage), which
makes this check useless.  That will make this if statement easier to read.

[romax, 2017/03/29 03:02:09]

Done.

+      // TODO(chili,petewil): We should have UMA here to track frequency of
+      // this, if it happens at all.
+      DVLOG(0) << "Offliner crashed. Cannot background offline page.";
+    }
+    state_ = RequestCoordinatorState::IDLE;
+  } else {
     SendRequestToOffliner(request);

[Pete Williamson, 2017/03/28 17:12:43]

Since we aren't checking the return value anymore, should we put the code back
to what it used to be?

[romax, 2017/03/29 03:02:09]

Done.

   }
 
@@ -790,7 +796,7 @@ void RequestCoordinator::RequestNotPicked(
     bool non_user_requested_tasks_remaining,
     bool cleanup_needed) {
   DVLOG(2) << __func__;
-  is_starting_ = false;
+  state_ = RequestCoordinatorState::IDLE;
 
   // Clear the outstanding "safety" task in the scheduler.
   scheduler_->Unschedule();
@@ -859,22 +865,8 @@ void RequestCoordinator::RequestCounts(bool is_start_of_processing,
   }
 }
 
-void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
-  // Check that offlining didn't get cancelled while performing some async
-  // steps.
-  if (processing_state_ == ProcessingWindowState::STOPPED)
-    return;
-
-  if (!offliner_) {
-    // TODO(chili,petewil): We should have UMA here to track frequency of this,
-    // if it happens at all.
-    DVLOG(0) << "Offliner crashed. Cannot background offline page.";
-    return;
-  }
-
-  DCHECK(!is_busy_);
-  is_busy_ = true;
-
+bool RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
+  DCHECK(state_ == RequestCoordinatorState::OFFLINING);
   // Record start time if this is first attempt.
   if (request.started_attempt_count() == 0) {
     RecordStartTimeUMA(request);
@@ -886,6 +878,7 @@ void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
       base::Bind(&RequestCoordinator::StartOffliner,
                  weak_ptr_factory_.GetWeakPtr(), request.request_id(),
                  request.client_id().name_space));
+  return true;
 }
 
 void RequestCoordinator::StartOffliner(
@@ -896,7 +889,7 @@ void RequestCoordinator::StartOffliner(
       update_result->item_statuses.size() != 1 ||
       update_result->item_statuses.at(0).first != request_id ||
       update_result->item_statuses.at(0).second != ItemActionStatus::SUCCESS) {
-    is_busy_ = false;
+    state_ = RequestCoordinatorState::IDLE;
     StopProcessing(Offliner::QUEUE_UPDATE_FAILED);
     DVLOG(1) << "Failed to mark attempt started: " << request_id;
     UpdateRequestResult request_result =
@@ -936,7 +929,7 @@ void RequestCoordinator::StartOffliner(
     watchdog_timer_.Start(FROM_HERE, timeout, this,
                           &RequestCoordinator::HandleWatchdogTimeout);
   } else {
-    is_busy_ = false;
+    state_ = RequestCoordinatorState::IDLE;
     DVLOG(0) << "Unable to start LoadAndSave";
     StopProcessing(Offliner::LOADING_NOT_ACCEPTED);
 
@@ -959,7 +952,7 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
   RecordOfflinerResultUMA(request.client_id(), request.creation_time(),
                           last_offlining_status_);
   watchdog_timer_.Stop();
-  is_busy_ = false;
+  state_ = RequestCoordinatorState::IDLE;
   active_request_.reset(nullptr);
 
   UpdateRequestForCompletedAttempt(request, status);