[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,
+    std::unique_ptr<net::NetworkQualityEstimator::NetworkQualityProvider>
+        network_quality_provider)
     : 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_(std::move(network_quality_provider)),
       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 269 matching lines @@
 
 void RequestCoordinator::StartProcessingIfConnected() {
   // Makes 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)

[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() {
   // 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(
           policy_->GetBackgroundProcessingTimeBudgetSeconds())) {
@@ skipping 183 matching lines @@
   FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages