[Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API.

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 68551e273d06c55ef526f9d609b50e744f240a3d..c94a1a9894c757c3898433b0da89d4a65e57150d 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -85,7 +85,7 @@ void RequestCoordinator::UpdateRequestCallback(
   // much, so just log it.
   if (result != RequestQueue::UpdateRequestResult::SUCCESS) {
     // TODO(petewil): Consider adding UMA or showing on offline-internals page.
-    DLOG(WARNING) << "Failed to update a request retry count. "
+    DLOG(WARNING) << "Failed to update request attempt details. "

[Pete Williamson, 2016/08/03 20:44:55]

Now that more is hanging off the update request, we may need to do more if an
attempted update fails.  What happens to the new cases where an update fails?

[dougarnett, 2016/08/03 21:40:01]

What can we do if persisting fails? Diagnostic things we could do would be some
UMA and log to event_logger I suppose.

For LoadAndSave, it will carry the MarkAttemptStarted state now.
Otherwise, we will see the previous state next Get.

[dougarnett, 2016/08/03 23:33:51]

Added event logger event

                   << static_cast<int>(result);
   }
 }
@@ -177,14 +177,31 @@ void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
   DCHECK(!is_busy_);
   is_busy_ = true;
 
+  // Prepare an updated request to attempt.
+  SavePageRequest updated_request(request);
+  updated_request.MarkAttemptStarted(base::Time::Now());
+
   // Start the load and save process in the offliner (Async).
-  offliner_->LoadAndSave(request,
+  if (offliner_->LoadAndSave(updated_request,
                          base::Bind(&RequestCoordinator::OfflinerDoneCallback,
-                                    weak_ptr_factory_.GetWeakPtr()));
+                                    weak_ptr_factory_.GetWeakPtr()))) {
+    // Offliner accepted request so update it in the queue.
+    RequestQueue::UpdateRequestCallback update_callback =
+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr());

[Pete Williamson, 2016/08/04 00:17:14]

Good catch to delete this, looks like leftover code from a debugging session.  I
wonder how the presubmit let the unused variable through...

[dougarnett, 2016/08/04 00:23:35]

I am curious as well.

+    queue_->UpdateRequest(
+        updated_request,
+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr()));
 
-  // Start a watchdog timer to catch pre-renders running too long
-  watchdog_timer_.Start(FROM_HERE, offliner_timeout_, this,
-                        &RequestCoordinator::StopProcessing);
+    // Start a watchdog timer to catch pre-renders running too long
+    watchdog_timer_.Start(FROM_HERE, offliner_timeout_, this,
+                          &RequestCoordinator::StopProcessing);
+  } else {
+    is_busy_ = false;
+    DVLOG(0) << "Unable to start LoadAndSave";
+    StopProcessing();

[Pete Williamson, 2016/08/03 20:44:55]

So do we always need to call StopProcessing if the prerender returns failure
here?  How and when will it get started again?

[dougarnett, 2016/08/03 21:40:01]

This was a hole before - LoadAndSave will never call callback if returns false -
so I guess watchdog timeout is what we would see. 

3 cases where false returned:
  1. already have pending request
  2. prerender manager says that it can't/won't prerender at all (per configured
mode and control group check)
  3. can't save the url

Maybe we should treat the last two cases as failures and delete the request.

[Pete Williamson, 2016/08/03 22:29:12]

Case 1 - Should never happen (I'm happy to add a DCHECK somewhere)
Case 2 - Sure it makes sense to StopProcessing, and I don't care if it ever gets
started again, but we should probably surface this to the caller somehow so they
know to stop calling, and that our virtual feature is disabled
Case 3 - We shouldn't stop processing here, but just move on to the next item.

[dougarnett, 2016/08/03 23:33:51]

Added CanSaveURL check in SavePageLater API to not accept such a request in
first place; and added DCHECKs in offliner to catch logic errors in Debug
builds.

+  }
 }
 
 void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
@@ -196,7 +213,7 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
   DCHECK_NE(status, Offliner::RequestStatus::LOADED);
   event_logger_.RecordSavePageRequestUpdated(
       request.client_id().name_space,
-      "Saved",
+      Offliner::RequestStatusToString(status),
       request.request_id());
   last_offlining_status_ = status;
   RecordOfflinerResultUMA(last_offlining_status_);
@@ -204,12 +221,24 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
 
   is_busy_ = false;
 
-  int64_t new_attempt_count = request.attempt_count() + 1;
+  if (status == Offliner::RequestStatus::FOREGROUND_CANCELED) {
+    // Update the request for the canceled attempt.

[Pete Williamson, 2016/08/03 20:44:55]

Aborted cases include:
* StopProcessing called by an outside caller of our API
* Chrome being swapped out by android
* Exceeding our maintenance window time slice

[dougarnett, 2016/08/03 21:40:01]

Yeah, we need to add these - none of them in OfflinerDoneCallback.
Specifically thinking about Render Process Killed (possible prelude to being
swapped out completely) and Prerendering Canceled here. 

[Pete Williamson, 2016/08/03 22:29:12]

Acknowledged.

[dougarnett, 2016/08/03 23:33:51]

Added a TODO in StopProcessing for is_busy_ case to find current request and
mark aborted

+    // TODO(dougarnett): See if we can conclusively identify other
+    // attempt aborted cases to treat this way.
+    SavePageRequest updated_request(request);
+    updated_request.MarkAttemptAborted();
+    RequestQueue::UpdateRequestCallback update_callback =
+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr());
+    queue_->UpdateRequest(
+        updated_request,
+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr()));
 
-  // 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_->GetMaxTries()) {
+  } else if (status == Offliner::RequestStatus::SAVED
+       || request.attempt_count() >= policy_->GetMaxTries()) {
+    // Remove the request from the queue if it either succeeded or exceeded the
+    // max number of retries.
     queue_->RemoveRequest(request.request_id(),
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                                      weak_ptr_factory_.GetWeakPtr()));
@@ -217,8 +246,7 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
     // 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());
+    updated_request.MarkAttemptCompleted();
     RequestQueue::UpdateRequestCallback update_callback =
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr());