Update the request count when a request fails.

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 e6ac73f55fc065eb4b25c8fee76eb8ad2f1f444c..37fead016d15aca189b7137d87c3abc53425a2c8 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -95,7 +95,14 @@ void RequestCoordinator::AddRequestResultCallback(
 
 // Called in response to updating a request in the request queue.
 void RequestCoordinator::UpdateRequestCallback(
-    RequestQueue::UpdateRequestResult result) {}
+    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) {
+    LOG(WARNING) << "Failed to update a request retry count. "

[fgorski, 2016/07/22 16:14:58]

why not DVLOG?

Also, do we have a UMA for this case? It would be reasonable to report that (and
maybe add a todo), and of course it would not be conditional on success.

Another thing is to consider showing that stuff on the internals page.

[Pete Williamson, 2016/07/22 18:27:00]

TODO added, Changed to DLOG (DVLOG doesn't work with warning levels).

+                 << static_cast<int>(result);
+  }
+}
 
 void RequestCoordinator::StopProcessing() {
   is_canceled_ = true;
@@ -194,18 +201,32 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
 
   is_busy_ = false;
 
-  // If the request succeeded, remove it from the Queue and maybe schedule
-  // another one.
-  if (status == Offliner::RequestStatus::SAVED) {
+  int64_t new_attempt_count = request.attempt_count() + 1;
+
+  // Remove the request from the queue if it either succeeded or exceeded the
+  // max number of retries.
+  if (status == Offliner::RequestStatus::SAVED
+      /* || new_attempt_count > policy_->GetMaxRetries()*/) {

[Pete Williamson, 2016/07/22 01:12:43]

This commented out code will be commented back in once changelist 2113383002
lands (implements request picker and policy, we need the policy to be
implemented for this line to compile).  In any case, this code will be either
removed or commented back in before this changelist is submitted.

     queue_->RemoveRequest(request.request_id(),
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                                      weak_ptr_factory_.GetWeakPtr()));
-
-    // TODO(petewil): Check time budget. Return to the scheduler if we are out.
-
-    // Start another request if we have time.
-    TryNextRequest();
+  } else {
+    // If we failed, but are not over the limit, update the request in the
+    // queue.
+    SavePageRequest updated_request(request);
+    updated_request.set_attempt_count(new_attempt_count);
+    updated_request.set_last_attempt_time(base::Time::Now());
+    RequestQueue::UpdateRequestCallback update_callback =
+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr());
+    queue_->UpdateRequest(updated_request,

[fgorski, 2016/07/22 16:14:58]

nit: is this how git cl format worked on this call?

[Pete Williamson, 2016/07/22 18:27:00]

Nope, that was me.  Changed to be a bit more uniform.

+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
+
+  // TODO(petewil): Check time budget. Return to the scheduler if we are out.
+  // Start another request if we have time.
+  TryNextRequest();
 }
 
 const Scheduler::TriggerConditions&