[Offline Pages] Improve RequestCoordinator state tracking.

components/offline_pages/core/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/core/background/request_coordinator.h"
 
 #include <limits>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 179 matching lines @@
 }
 
 RequestCoordinator::RequestCoordinator(
     std::unique_ptr<OfflinerPolicy> policy,
     std::unique_ptr<Offliner> offliner,
     std::unique_ptr<RequestQueue> queue,
     std::unique_ptr<Scheduler> scheduler,
     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)),
       policy_(std::move(policy)),
       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),
@@ skipping 68 matching lines @@
 void RequestCoordinator::GetQueuedRequestsCallback(
     const GetRequestsCallback& callback,
     GetRequestsResult result,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   callback.Run(std::move(requests));
 }
 
 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,
         weak_ptr_factory_.GetWeakPtr(), final_callback, stop_status));
     return;
   }
 
   UpdateStatusForCancel(stop_status);
   int64_t request_id =
       active_request_.get() ? active_request_->request_id() : 0LL;
@@ skipping 247 matching lines @@
   // Stopping offliner means it will not call callback so set last status.
   last_offlining_status_ = stop_status;
 
   if (active_request_) {
     event_logger_.RecordOfflinerResult(active_request_->client_id().name_space,
                                        last_offlining_status_,
                                        active_request_->request_id());
     RecordOfflinerResultUMA(active_request_->client_id(),
                             active_request_->creation_time(),
                             last_offlining_status_);
-    is_busy_ = false;
     active_request_.reset();
   }
+  state_ = RequestCoordinatorState::IDLE;
 }
 
 void RequestCoordinator::ResetActiveRequestCallback(int64_t offline_id) {
   active_request_.reset();
 }
 
 void RequestCoordinator::StartSchedulerCallback(int64_t offline_id) {
   scheduler_callback_.Run(true);
 }
 
@@ skipping 46 matching lines @@
       "OfflinePages.Background.ImmediateStartStatus", immediate_start_status,
       RequestCoordinator::OfflinerImmediateStartStatus::STATUS_COUNT);
   return immediate_start_status == OfflinerImmediateStartStatus::STARTED;
 }
 
 // The current_conditions_ must be set sometime before calling
 // StartProcessingInternal on all calling code paths.
 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;
 
   // Mark the time at which we started processing so we can check our time
   // budget.
   operation_start_time_ = base::Time::Now();
 
   TryNextRequest(kStartOfProcessing);
 
   return true;
 }
 
 void RequestCoordinator::StartImmediatelyIfConnected() {
   StartImmediateProcessing(internal_start_processing_callback_);
 }
 
 RequestCoordinator::OfflinerImmediateStartStatus
 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.
   if (is_low_end_device_ &&
       !offline_pages::IsOfflinePagesSvelteConcurrentLoadingEnabled()) {
     DVLOG(2) << "low end device, returning";
     // Let the scheduler know we are done processing and failed due to svelte.
     callback.Run(false);
     return OfflinerImmediateStartStatus::NOT_STARTED_ON_SVELTE;
   }
@@ skipping 34 matching lines @@
   // If we have already set the connection type for testing, don't get it from
   // android, but use what the test already set up.
   if (use_test_device_conditions_)
     return;
 
   current_conditions_ = base::MakeUnique<DeviceConditions>(
       scheduler_->GetCurrentDeviceConditions());
 }
 
 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.
   // TODO(petewil): Now that we can get conditions any time, consider getting
   // them now instead of passing them in earlier when we start scheduled
   // processing.
   if (!is_start_of_processing) {
     // Get current device conditions from the Java side across the bridge.
     // NetworkChangeNotifier will not have the right conditions if chromium is
     // in the background in android, so prefer to always get the conditions via
@@ skipping 14 matching lines @@
   net::NetworkChangeNotifier::ConnectionType connection_type =
       current_conditions_->GetNetConnectionType();
 
   // If there is no network or no time left in the budget, return to the
   // scheduler. We do not remove the pending scheduler task that was set
   // up earlier in case we run out of time, so the background scheduler
   // will return to us at the next opportunity to run background tasks.
   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.
     if (processing_state_ == ProcessingWindowState::IMMEDIATE_WINDOW)
       RequestConnectedEventForStarting();
 
     // Let the scheduler know we are done processing.
     // TODO: Make sure the scheduler callback is valid before running it.
     scheduler_callback_.Run(true);
     DVLOG(2) << " out of time, giving up. " << __func__;
@@ skipping 15 matching lines @@
   // 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,
                                        bool cleanup_needed) {
   DVLOG(2) << request.url() << " " << __func__;
-  is_starting_ = false;
 
-  // Make sure we were not stopped while picking.
-  if (processing_state_ != ProcessingWindowState::STOPPED) {
+  // Make sure we were not stopped while picking, since any kind of cancel/stop
+  // will reset the state back to IDLE.
+  if (state_ == RequestCoordinatorState::PICKING) {
+    state_ = RequestCoordinatorState::OFFLINING;
     // Send the request on to the offliner.
     SendRequestToOffliner(request);
   }
 
   // Schedule a queue cleanup if needed.
   if (cleanup_needed)
     queue_->CleanupRequestQueue();
 }
 
 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();
 
   // 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.
@@ skipping 49 matching lines @@
           "OfflinePages.Background.ImmediateStart.AvailableRequestCount",
           available_requests);
       UMA_HISTOGRAM_COUNTS_1000(
           "OfflinePages.Background.ImmediateStart.UnavailableRequestCount",
           total_requests - available_requests);
     }
   }
 }
 
 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;
-
+  DCHECK(state_ == RequestCoordinatorState::OFFLINING);
   // Record start time if this is first attempt.
   if (request.started_attempt_count() == 0) {
     RecordStartTimeUMA(request);
   }
 
   // Mark attempt started in the database and start offliner when completed.
   queue_->MarkAttemptStarted(
       request.request_id(),
       base::Bind(&RequestCoordinator::StartOffliner,
                  weak_ptr_factory_.GetWeakPtr(), request.request_id(),
                  request.client_id().name_space));
 }
 
 void RequestCoordinator::StartOffliner(
     int64_t request_id,
     const std::string& client_namespace,
     std::unique_ptr<UpdateRequestsResult> update_result) {
   if (update_result->store_state != StoreState::LOADED ||
       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 =
         update_result->store_state != StoreState::LOADED
             ? UpdateRequestResult::STORE_FAILURE
             : UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
     event_logger_.RecordUpdateRequestFailed(client_namespace, request_result);
     return;
   }
 
@@ skipping 19 matching lines @@
           policy_->GetSinglePageTimeLimitForImmediateLoadInSeconds());
     }
 
     // Inform observer of active request.
     NotifyChanged(*active_request_.get());
 
     // Start a watchdog timer to catch pre-renders running too long
     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);
 
     // We need to undo the MarkAttemptStarted that brought us to this
     // method since we didn't success in starting after all.
     MarkAttemptAborted(request_id, client_namespace);
   }
 }
 
 void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                                               Offliner::RequestStatus status) {
   DVLOG(2) << "offliner finished, saved: "
            << (status == Offliner::RequestStatus::SAVED)
            << ", status: " << static_cast<int>(status) << ", " << __func__;
   DCHECK_NE(status, Offliner::RequestStatus::UNKNOWN);
   DCHECK_NE(status, Offliner::RequestStatus::LOADED);
   event_logger_.RecordOfflinerResult(request.client_id().name_space, status,
                                      request.request_id());
   last_offlining_status_ = status;
   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);
   if (ShouldTryNextRequest(status))
     TryNextRequest(!kStartOfProcessing);
   else
     scheduler_callback_.Run(true);
 }
 
 void RequestCoordinator::OfflinerProgressCallback(
@@ skipping 145 matching lines @@
 
 ClientPolicyController* RequestCoordinator::GetPolicyController() {
   return policy_controller_.get();
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages