Update the request count when a request fails.

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 77 matching lines @@
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {
 
   // Inform the scheduler that we have an outstanding task.
   // TODO(petewil): Determine trigger conditions from policy.
   scheduler_->Schedule(GetTriggerConditionsForUserRequest());
 }
 
 // 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;
   if (offliner_ && is_busy_)
     offliner_->Cancel();
 
   // Stopping offliner means it will not call callback.
   last_offlining_status_ =
       Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED;
   RecordOfflinerResultUMA(last_offlining_status_);
@@ skipping 78 matching lines @@
   event_logger_.RecordSavePageRequestUpdated(
       request.client_id().name_space,
       "Saved",
       request.request_id());
   last_offlining_status_ = status;
   RecordOfflinerResultUMA(last_offlining_status_);
   watchdog_timer_.Stop();
 
   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()));
+  } 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();
-  }
+  // TODO(petewil): Check time budget. Return to the scheduler if we are out.
+  // Start another request if we have time.
+  TryNextRequest();
 }
 
 const Scheduler::TriggerConditions&
 RequestCoordinator::GetTriggerConditionsForUserRequest() {
   return kUserRequestTriggerConditions;
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages