[Offline Pages] Converts MarkAttemptCompleted to use TaskQueue

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 <limits>
-#include <string>
 #include <utility>
-#include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "base/sys_info.h"
 #include "base/time/time.h"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
@@ skipping 340 matching lines @@
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {
   NotifyAdded(request);
   // Inform the scheduler that we have an outstanding task.
   scheduler_->Schedule(GetTriggerConditions(kUserRequest));
 
   if (request.user_requested())
     StartImmediatelyIfConnected();
 }
 
-// Called in response to updating a request in the request queue.
-void RequestCoordinator::UpdateRequestCallback(
+void RequestCoordinator::MarkAttemptCompletedDoneCallback(
+    int64_t request_id,
     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);
+    std::unique_ptr<UpdateRequestsResult> result) {
+  if (result->store_state != StoreState::LOADED ||

[fgorski, 2016/10/31 20:33:50]

see if in line 301 please and consider if you could extract some code.

[dougarnett, 2016/10/31 22:05:52]

Done.

+      result->item_statuses.size() != 1 ||
+      result->item_statuses.at(0).second != ItemActionStatus::SUCCESS) {
+    DVLOG(1) << "Failed to mark request completed " << request_id;
+    event_logger_.RecordUpdateRequestFailed(
+        client_id.name_space,
+        result->store_state != StoreState::LOADED

[Pete Williamson, 2016/10/31 21:38:26]

I think having the : ? inside the parameter list makes the code less readable, I
think the code would be easier to read if the check is put outside the function
call, and just use the result of the check here.  Yes, that is an extra local
variable, but the compiler optimizes it out, and I think the extra variable
pulls its weight in making the code more readable.

[dougarnett, 2016/10/31 22:51:18]

Actually mirroring pattern already in this file, not introducing new pattern.

[Pete Williamson, 2016/10/31 23:10:08]

Let's still fix it, the pattern is not a great one.  It looks like the other use
case got abstracted out into a function, perhaps we can do that here too.

[dougarnett, 2016/11/01 00:16:37]

ok, done

+            ? RequestQueue::UpdateRequestResult::STORE_FAILURE
+            : RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST);
+  } else {
+    NotifyChanged(result->updated_items.at(0));

[Pete Williamson, 2016/10/31 21:38:26]

The old code notified changed even if the database update failed, perhaps we
should still do that.

[dougarnett, 2016/10/31 22:51:17]

Great question. I'm not sure. Would we want to NotifyCompleted() instead with
some type of failed status? Also, we may not have the request to call
NotifyChange
with (or NotifyCompleted) - especially if we stop caching the full active
request.

   }
 }
 
 void RequestCoordinator::UpdateMultipleRequestsCallback(
     std::unique_ptr<UpdateRequestsResult> result) {
   for (const auto& request : result->updated_items)
     NotifyChanged(request);
 
   bool available_user_request = false;
   for (const auto& request : result->updated_items) {
@@ skipping 313 matching lines @@
     // MarkAttemptCompleted within the if branches, the completed_attempt_count
     // has not yet been updated when we are checking the if condition.
     const BackgroundSavePageResult result(
         BackgroundSavePageResult::RETRY_COUNT_EXCEEDED);
     event_logger_.RecordDroppedSavePageRequest(request.client_id().name_space,
                                                result, request.request_id());
     RemoveAttemptedRequest(request, result);
   } else {
     // If we failed, but are not over the limit, update the request in the
     // queue.
-    SavePageRequest updated_request(request);
-    updated_request.MarkAttemptCompleted();
-    queue_->UpdateRequest(updated_request,
-                          base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                                     weak_ptr_factory_.GetWeakPtr(),
-                                     updated_request.client_id()));
-    NotifyChanged(updated_request);
+    queue_->MarkAttemptCompleted(
+        request.request_id(),
+        base::Bind(&RequestCoordinator::MarkAttemptCompletedDoneCallback,

[Pete Williamson, 2016/10/31 21:38:26]

Hmm, if this is really the only place left we used UpdateRequestCallback, it
should be OK to rename.

[dougarnett, 2016/10/31 22:51:17]

Yes. Also have now deleted RequestQueue::UpdateRequest() as well

+                   weak_ptr_factory_.GetWeakPtr(), request.request_id(),
+                   request.client_id()));
   }
 
   // Determine whether we might try another request in this
   // processing window based on how the previous request completed.
   switch (status) {
     case Offliner::RequestStatus::SAVED:
     case Offliner::RequestStatus::SAVE_FAILED:
     case Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED:
     case Offliner::RequestStatus::REQUEST_COORDINATOR_TIMED_OUT:
     case Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY:
@@ skipping 83 matching lines @@
 
 ClientPolicyController* RequestCoordinator::GetPolicyController() {
   return policy_controller_.get();
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages