[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..2ac4474844ab38c22755aa6e24c801a7d6f0bc0f 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,24 @@ 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 (!available_user_request && request.user_requested() &&
+        request.request_state() == SavePageRequest::RequestState::AVAILABLE) {
+      for (std::pair<int64_t, RequestQueue::UpdateRequestResult> pair :
+           results) {

[Pete Williamson, 2016/08/26 21:23:55]

What if we resume a large batch of results?  If so, we get N^2 behavior.  Should
we use a std::unordered_set here (it wraps a hash map, so we get O(1) lookup). 
I got the same feedback on a similar change.

[dougarnett, 2016/08/26 21:40:51]

Yeah, bulk Pause looks like N^2 - bulk Resume does not. So perhaps could "know"
what the operation was, to avoid the N^2 (if it meant simpler code).

I'm not clear about your suggestion. Do you mean change the returned result
object from RequestQueue or something locally? Seems good to do something at
RequestQueue interface level at some point rather than (prematurely?) worry
about N^2 in every code review trying to cope with this result vector. Initially
I wondered about vector of successful requests and vector of pairs failed
requests with result codes in case that might generally be easier to consume. 



 

+        if (pair.first == request.request_id() &&
+            pair.second == RequestQueue::UpdateRequestResult::SUCCESS) {
+          // We have a successfully updated, available, user request.
+          available_user_request = true;

[Dmitry Titov, 2016/08/26 21:46:57]

+1 to what Pete says, we could load all request_ids of AVAILABLE into a
hash_map, then walk over results and .find() every SUCCESSful update... then
break;

[dougarnett, 2016/08/26 22:33:44]

I added a TODO for the moment to flag the topic in the code. 

I do wonder if we would be better off looking to improve the interface from
RequestQueue (for M55) to avoid the need for consumers to create a hash map to
cope with it. Is the hash map allocation cheap enough in C++ environment that we
don't mind creating it for the existing N==1 case? 

[Pete Williamson, 2016/08/26 23:38:02]

It's really pretty easy.  I think I'd prefer to fix it now.  We can leave the
TODO to change the design in M55  Here's an example from one of my patches out
for review.  However, if you need to get this into M54, I am OK with just doing
a TODO.

   std::unordered_set<int64_t> request_id_set(
       request_ids.begin(), request_ids.end());
   for (const SavePageRequest& request : requests) {
     // If this request is in our list, update it.
     if (request_id_set.count(request.request_id()) > 0) {
       SavePageRequest updated_request = request;
       updated_request.set_request_state(new_state);
       // TODO(petewil): Add a batch method so we only make a single call to the
       // store.
       queue_->UpdateRequest(
           updated_request,
           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                      weak_ptr_factory_.GetWeakPtr(), request.request_id()));
     }
   }

[Pete Williamson, 2016/08/26 23:48:59]

DougArnett pointed out that the UI only supports pause and resume of a single
request at a time, so we can postpone the map optimization until we have a way
to resume multiples.

+        }
+      }
+    }
+  }
+
+  if (available_user_request)
+    StartProcessingIfConnected();
 }
 
 void RequestCoordinator::HandleRemovedRequestsAndCallback(
@@ -290,6 +293,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