[Offline Pages] Converts MarkAttemptCompleted to use TaskQueue

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 cfc32ed6421536e1e1db56baee43602706767d15..f41ca2f715c7c8ea1c6b3acc16f3afa5f410de98 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -5,9 +5,7 @@
 #include "components/offline_pages/background/request_coordinator.h"
 
 #include <limits>
-#include <string>
 #include <utility>
-#include <vector>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ -368,16 +366,21 @@ void RequestCoordinator::AddRequestResultCallback(
     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.

   }
 }
 
@@ -711,13 +714,11 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
   } 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