[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

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 276 matching lines @@
     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,
-    std::vector<std::unique_ptr<SavePageRequest>> requests) {
+    std::unique_ptr<UpdateRequestsResult> result) {
+  for (const auto& request : result->updated_items)
+    NotifyChanged(request);
+
   bool available_user_request = false;
-  for (const auto& request : requests) {
-    NotifyChanged(*(request));
-    if (!available_user_request && request->user_requested() &&
-        request->request_state() == SavePageRequest::RequestState::AVAILABLE) {
-      // TODO(dougarnett): Consider avoiding prospect of N^2 in case
-      // size of bulk requests can get large (perhaps with easier to consume
-      // callback interface).
-      for (std::pair<int64_t, RequestQueue::UpdateRequestResult> pair :
-           results) {
-        if (pair.first == request->request_id() &&
-            pair.second == RequestQueue::UpdateRequestResult::SUCCESS) {
-          // We have a successfully updated, available, user request.
-          available_user_request = true;
-        }
-      }
+  for (const auto& request : result->updated_items) {

[dougarnett, 2016/09/29 21:02:49]

Could have just one iteration through updated_items

[fgorski, 2016/09/30 23:14:35]

I know, but I prefer this, because it is the same complexity, but clearer
separation (notify changed can be extracted from all methods and augmented by
event logger) and readability that way.

+    if (!available_user_request && request.user_requested() &&
+        request.request_state() == SavePageRequest::RequestState::AVAILABLE) {
+      available_user_request = true;
     }
   }
 
   if (available_user_request)
     StartProcessingIfConnected();
 }
 
 void RequestCoordinator::HandleRemovedRequestsAndCallback(
     const RemoveRequestsCallback& callback,
     BackgroundSavePageResult status,
@@ skipping 282 matching lines @@
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages