[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..883e0c48640cb44c5622d21a32c42cb5020ed956 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -75,10 +75,13 @@ 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,
+    std::unique_ptr<net::NetworkQualityEstimator::NetworkQualityProvider>
+        network_quality_provider)
     : is_busy_(false),
       is_starting_(false),
       is_stopped_(false),
@@ -89,6 +92,7 @@ RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
       factory_(std::move(factory)),
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
+      nqe_(std::move(network_quality_provider)),
       active_request_(nullptr),
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
       offliner_timeout_(base::TimeDelta::FromSeconds(
@@ -378,14 +382,22 @@ void RequestCoordinator::StartProcessingIfConnected() {
 
   // Check for network connectivity.
   net::NetworkChangeNotifier::ConnectionType connection = GetConnectionType();
+  if (connection == net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE)

[tbansal1, 2016/09/16 19:13:50]

GetEffectiveConnection type will return E_C_T_OFFLINE if the device is actually
offline. So, you do not need to check net::NetworkChangeNotifier separately.

[dougarnett, 2016/09/16 21:24:42]

Done.

+    return;
 
-  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));
+  // Check for reasonable network quality.
+  if (nqe_.get()) {
+    // TODO(dougarnett): Add UMA for quality type experienced.
+    net::EffectiveConnectionType quality = nqe_->GetEffectiveConnectionType();
+    if (quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G)

[RyanSturm, 2016/09/16 18:54:46]

Normally, we use Slow 2g instead of just 2g, I'm not sure of the exact
requirements for this though. Check if quality >= offline:

quality >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE
&&
quality <= net::EFFECTIVE_CONNECTION_TYPE_SLOW_2G;

https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/off...

[tbansal1, 2016/09/16 19:13:50]

Currently, the method returns if ECT enum is UNKNOWN which I think is WAI.

[dougarnett, 2016/09/16 21:24:42]

So this linked code is for deciding to navigate to offline page or not if the
network IsNetworkProhibitivelySlow, right? UNKNOWN does not indicate that it is
slow.

This new case is trying to decide whether to kick off a background load or not
if the network is known to be recently good. So I think for this case UNKNOWN
should not be included as known good condition. 

[RyanSturm, 2016/09/19 17:15:40]

Totally misread this chunk of code. Seems like the right comparison, feel free
to go back to the < and early return like tbansal@ suggested in another comment.

+      return;
   }
+
+  // Start processing with manufactured conservative device conditions for the
+  // connectivity (assume no battery).
+  // TODO(dougarnett): Obtain actual conditions (from Android/Java).
+  DeviceConditions device_conditions(false, 0, connection);
+  StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback));
 }
 
 void RequestCoordinator::TryNextRequest() {