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

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 d5d37101dede68d04e3dddb2f6d4e57aee048d84..499453c7df3db6e44258b34735fc21e151ba54a9 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -75,10 +75,12 @@ 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),
@@ -89,6 +91,7 @@ RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> 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(
@@ -370,21 +373,23 @@ bool RequestCoordinator::StartProcessing(
 }
 
 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));
+    }
   }
 }
 
@@ -588,4 +593,8 @@ void RequestCoordinator::GetOffliner() {
   }
 }
 
+void RequestCoordinator::Shutdown() {
+  nqe_ = nullptr;
+}
+
 }  // namespace offline_pages