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

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 67 matching lines @@
   scheduler_->Schedule(GetTriggerConditionsForUserRequest());
 }
 
 // Called in response to updating a request in the request queue.
 void RequestCoordinator::UpdateRequestCallback(
     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) {
     // 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);
   }
 }
 
 void RequestCoordinator::StopProcessing() {
   is_canceled_ = true;
   if (offliner_ && is_busy_)
     offliner_->Cancel();
 
   // Stopping offliner means it will not call callback.
@@ skipping 71 matching lines @@
   GetOffliner();
   if (!offliner_) {
     DVLOG(0) << "Unable to create Offliner. "
              << "Cannot background offline page.";
     return;
   }
 
   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,
                                               Offliner::RequestStatus status) {
   DVLOG(2) << "offliner finished, saved: "
            << (status == Offliner::RequestStatus::SAVED)
            << ", status: " << static_cast<int>(status) << ", " << __func__;
   DCHECK_NE(status, Offliner::RequestStatus::UNKNOWN);
   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_);
   watchdog_timer_.Stop();
 
   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()));
   } 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());
+    updated_request.MarkAttemptCompleted();
     RequestQueue::UpdateRequestCallback update_callback =
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr());
     queue_->UpdateRequest(
         updated_request,
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 
   // Determine whether we might try another request in this
@@ skipping 29 matching lines @@
   return trigger_conditions;
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages