[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 (!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(
     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