[OfflinePages, NetworkQualityEstimator] Use NetworkQualityEstimator to decide on triggering Backgro…

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 57 matching lines @@
 int64_t GenerateOfflineId() {
   return base::RandGenerator(std::numeric_limits<int64_t>::max()) + 1;
 }
 
 // In case we start processing from SavePageLater, we need a callback, but there
 // is nothing for it to do.
 void EmptySchedulerCallback(bool started) {}
 
 }  // namespace
 
-RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
-                                       std::unique_ptr<OfflinerFactory> factory,
-                                       std::unique_ptr<RequestQueue> queue,
-                                       std::unique_ptr<Scheduler> scheduler)
+RequestCoordinator::RequestCoordinator(
+    std::unique_ptr<OfflinerPolicy> policy,
+    std::unique_ptr<OfflinerFactory> factory,
+    std::unique_ptr<RequestQueue> queue,
+    std::unique_ptr<Scheduler> scheduler,
+    net::NetworkQualityEstimator::NetworkQualityProvider* nqe)
     : is_busy_(false),
       is_starting_(false),
       is_stopped_(false),
       use_test_connection_type_(false),
       test_connection_type_(),
       offliner_(nullptr),
       policy_(std::move(policy)),
       factory_(std::move(factory)),
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
+      nqe_(nqe),
       active_request_(nullptr),
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
       offliner_timeout_(base::TimeDelta::FromSeconds(
           policy_->GetSinglePageTimeLimitInSeconds())),
       weak_ptr_factory_(this) {
   DCHECK(policy_ != nullptr);
   picker_.reset(
       new RequestPicker(queue_.get(), policy_.get(), this, &event_logger_));
 }
 
@@ skipping 261 matching lines @@
 
   is_stopped_ = false;
   scheduler_callback_ = callback;
 
   TryNextRequest();
 
   return true;
 }
 
 void RequestCoordinator::StartProcessingIfConnected() {
-  // Makes sure not already busy processing.
+  // Make sure not already busy processing.
   if (is_busy_) return;
 
   // Make sure we are not on svelte device to start immediately.
   if (base::SysInfo::IsLowEndDevice()) return;
 
-  // Check for network connectivity.
-  net::NetworkChangeNotifier::ConnectionType connection = GetConnectionType();
-
-  if ((connection !=
-       net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE)) {
-    // Create conservative device conditions for the connectivity
-    // (assume no battery).
-    DeviceConditions device_conditions(false, 0, connection);
-    StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback));
+  // Make sure we have reasonable network quality.
+  if (nqe_) {
+    // TODO(dougarnett): Add UMA for quality type experienced.
+    net::EffectiveConnectionType quality = nqe_->GetEffectiveConnectionType();
+    if (quality >= net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) {

[tbansal1, 2016/09/16 22:05:55]

nit: I kind of liked it the old way
if(quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G)
  return;
which should lead to fewer indentations, but it is your call.

[Pete Williamson, 2016/09/16 22:26:01]

This excludes EFFECTIVE_CONNECTION_TYPE_SLOW_2G.  Do we think it should be
included or excluded?

[dougarnett, 2016/09/19 19:34:32]

Key question. Yes, as a starting point I am thinking we want poor connection
excluded (and non-poor 2G included) - otherwise, we might not bother here with
the NQE after all. We might want some more user intent understood from the UX to
know we should make every effort now (vs. user just queuing up a set of pages
for later while the user limps along in the foreground). 

[dougarnett, 2016/09/19 19:34:32]

Yeah, this was due to taking out the basic network filter and not wanting drop
through without it. Now trying it back in for else case of no NQE.

+      // Start processing with manufactured conservative battery conditions
+      // (i.e., assume no battery).
+      // TODO(dougarnett): Obtain actual battery conditions (from Android/Java).
+      DeviceConditions device_conditions(false, 0, GetConnectionType());
+      StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback));
+    }
   }
 }
 
 void RequestCoordinator::TryNextRequest() {
   // If there is no time left in the budget, return to the scheduler.
   // We do not remove the pending task that was set up earlier in case
   // we run out of time, so the background scheduler will return to us
   // at the next opportunity to run background tasks.
   if (base::Time::Now() - operation_start_time_ >
       base::TimeDelta::FromSeconds(
@@ skipping 183 matching lines @@
 void RequestCoordinator::NotifyChanged(const SavePageRequest& request) {
   FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
+void RequestCoordinator::Shutdown() {
+  nqe_ = nullptr;
+}
+
 }  // namespace offline_pages