[OfflinePages] Resuming a SavePageRequest now may start processing if network is connected.

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 <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 189 matching lines @@
   return net::NetworkChangeNotifier::GetConnectionType();
 }
 
 void RequestCoordinator::AddRequestResultCallback(
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {
   NotifyAdded(request);
   // Inform the scheduler that we have an outstanding task..
   scheduler_->Schedule(GetTriggerConditionsForUserRequest());
 
-  // If it makes sense, start processing now.
-  if (is_busy_) return;
-
-  // Check for network
-  net::NetworkChangeNotifier::ConnectionType connection = GetConnectionType();
-
-  if ((connection !=
-       net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE) &&
-      request.user_requested()) {
-    // Create device conditions.
-    DeviceConditions device_conditions(false, 0, connection);
-
-    // Start processing if it makes sense. (net, user requested)
-    StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback));
-  }
+  if (request.user_requested())
+    StartProcessingIfConnected();
 }
 
 // Called in response to updating a request in the request queue.
 void RequestCoordinator::UpdateRequestCallback(
     const ClientId& client_id,
     RequestQueue::UpdateRequestResult result) {
   // If the request succeeded, nothing to do.  If it failed, we can't really do
   // much, so just log it.
   if (result != RequestQueue::UpdateRequestResult::SUCCESS) {
     DVLOG(1) << "Failed to update request attempt details. "
              << static_cast<int>(result);
     event_logger_.RecordUpdateRequestFailed(client_id.name_space, result);
   }
 }
 
 // Called in response to updating multiple requests in the request queue.
 void RequestCoordinator::UpdateMultipleRequestsCallback(
     const RequestQueue::UpdateMultipleRequestResults& results,
     const std::vector<SavePageRequest>& requests) {
-  for (SavePageRequest request : requests)
+
+  bool available_user_request = false;
+  for (SavePageRequest request : requests) {
     NotifyChanged(request);
+    if (request.user_requested() &&
+        request.request_state() == SavePageRequest::RequestState::AVAILABLE)

[Pete Williamson, 2016/08/26 20:39:12]

We aren't checking the results, which makes me wonder - what would happen if we
tried to pause a request, and pausing failed, and we returned the request in the
request list, but status was available.  Would we start trying to process it
right away?

[dougarnett, 2016/08/26 20:57:47]

Good point - we could start processing whatever next request is picked (if not
already processing).

Would be better to guard it by result. Should we also guard call
NotifyChanged()?

[dougarnett, 2016/08/26 21:20:19]

uploaded change to just guard the new flag for calling start processing

+      available_user_request = true;
+  }
+
+  if (available_user_request)
+    StartProcessingIfConnected();
 }
 
 void RequestCoordinator::HandleRemovedRequestsAndCallback(
     const RemoveRequestsCallback& callback,
     SavePageStatus status,
     const RequestQueue::UpdateMultipleRequestResults& results,
     const std::vector<SavePageRequest>& requests) {
   callback.Run(results);
   HandleRemovedRequests(status, results, requests);
 }
@@ skipping 27 matching lines @@
   operation_start_time_ = base::Time::Now();
 
   is_stopped_ = false;
   scheduler_callback_ = callback;
 
   TryNextRequest();
 
   return true;
 }
 
+void RequestCoordinator::StartProcessingIfConnected() {
+  // Makes sure not already busy processing.
+  if (is_busy_) return;
+
+  // Check for network connectivity.
+  net::NetworkChangeNotifier::ConnectionType connection = GetConnectionType();
+
+  if ((connection !=
+       net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE)) {
+    // Create conservative device conditions for the connectivity
+    // (assume no battery).
+    DeviceConditions device_conditions(false, 0, connection);
+    StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback));
+  }
+}
+
 void RequestCoordinator::TryNextRequest() {
   // If there is no time left in the budget, return to the scheduler.
   // We do not remove the pending 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 (base::Time::Now() - operation_start_time_ >
       base::TimeDelta::FromSeconds(
           policy_->GetBackgroundProcessingTimeBudgetSeconds())) {
     // Let the scheduler know we are done processing.
     scheduler_callback_.Run(true);
@@ skipping 180 matching lines @@
   FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages