Skips processing more SavePageLater requests in current processing window when one completes unsucc…

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 210 matching lines @@
     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,
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 
-  TryNextRequest();
+  // Determine whether we might try another request in this
+  // processing window based on how the previous request completed.
+  //
+  // TODO(dougarnett): Need to split PRERENDERING_FAILED into separate
+  // codes as to whether we should try another request or not.
+  switch (status) {
+    case Offliner::RequestStatus::SAVED:

[fgorski, 2016/08/03 03:40:04]

It is a bit worrying that you have two scope operators here. Perhaps the enum
does not belong inside offliner?

[dougarnett, 2016/08/03 15:33:26]

worrying? This is part of the Offliner interface so not sure of better place to
define it. Happy to hear more specific recommendation for reducing scope or just
using "using".

[fgorski, 2016/08/03 16:18:59]

Acknowledged.

+    case Offliner::RequestStatus::SAVE_FAILED:
+    case Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED:  // timeout
+    case Offliner::RequestStatus::PRERENDERING_CANCELED:

[Pete Williamson, 2016/08/02 22:20:00]

Good!
Let's be a bit more explicit for the cases where we don't keep processing by
calling out the cases:

   case Offliner::RequestStatus::PRERENDER_FAILED:
   case Offliner::RequestStatus::FOREGOUND_CANCELLED:
...
  default:
      break;

[dougarnett, 2016/08/03 15:33:26]

Done.

+      TryNextRequest();
+      break;
+    default:
+      // Stop further processing in this service window.

[fgorski, 2016/08/03 03:40:05]

https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

[dougarnett, 2016/08/03 15:33:26]

Done.

+      break;
+  }
 }
 
 const Scheduler::TriggerConditions
 RequestCoordinator::GetTriggerConditionsForUserRequest() {
   Scheduler::TriggerConditions trigger_conditions(
       policy_->PowerRequiredForUserRequestedPage(),
       policy_->BatteryPercentageRequiredForUserRequestedPage(),
       policy_->UnmeteredNetworkRequiredForUserRequestedPage());
   return trigger_conditions;
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages