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

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 e6588f93757dde5a1862efe193c0fb89d5e2b4bb..22cff93a15c3812b8dfc1d4285a2a5b2fc1e482c 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -207,21 +207,8 @@ void RequestCoordinator::AddRequestResultCallback(
   // 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.
@@ -241,8 +228,17 @@ void RequestCoordinator::UpdateRequestCallback(
 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(
@@ -290,6 +286,22 @@ bool RequestCoordinator::StartProcessing(
   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