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

components/offline_pages/background/request_coordinator_unittest.cc

Index: components/offline_pages/background/request_coordinator_unittest.cc
diff --git a/components/offline_pages/background/request_coordinator_unittest.cc b/components/offline_pages/background/request_coordinator_unittest.cc
index 1b469fd84cd2091b005807a1138a77aa2db82128..523ecfcfa3e3b01c5e010ce63a385d143757063a 100644
--- a/components/offline_pages/background/request_coordinator_unittest.cc
+++ b/components/offline_pages/background/request_coordinator_unittest.cc
@@ -127,6 +127,26 @@ class OfflinerFactoryStub : public OfflinerFactory {
   std::unique_ptr<OfflinerStub> offliner_;
 };
 
+// TODO(dougarnett): Replace with real NQE instance when/if support interface.

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

For the purposes of this test, you'd probably be better off not using the real
NQE since you just want to test the limited interface, so this TODO isn't
necessary IMO.

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

Done.

+class NetworkQualityEstimatorStub

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

nit: may be s/NetworkQualityEstimatorStub/TestNetworkQualityEstimator/

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

Acknowledged. I prefer that too but matching naming style that already exists in
this test class.

+    : public net::NetworkQualityEstimator::NetworkQualityProvider {
+ public:
+  NetworkQualityEstimatorStub()
+      : connection_type_(
+            net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G) {}
+
+  net::EffectiveConnectionType GetEffectiveConnectionType() override {
+    return connection_type_;
+  }
+
+  void SetEffectiveConnectionTypeForTesting(net::EffectiveConnectionType type) {
+    connection_type_ = type;
+  }
+
+ private:
+  net::EffectiveConnectionType connection_type_;
+};
+
 class ObserverStub : public RequestCoordinator::Observer {
  public:
   ObserverStub()
@@ -246,6 +266,10 @@ class RequestCoordinatorTest
     coordinator()->SetNetworkConditionsForTest(connection);
   }
 
+  void SetEffectiveConnectionTypeForTesting(net::EffectiveConnectionType type) {
+    nqe_->SetEffectiveConnectionTypeForTesting(type);
+  }
+
   void ScheduleForTest() { coordinator_->ScheduleAsNeeded(); }
 
   void CallRequestNotPicked(bool non_user_requested_tasks_remaining) {
@@ -282,6 +306,7 @@ class RequestCoordinatorTest
   std::vector<std::unique_ptr<SavePageRequest>> last_requests_;
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
+  NetworkQualityEstimatorStub* nqe_;
   std::unique_ptr<RequestCoordinator> coordinator_;
   OfflinerStub* offliner_;
   base::WaitableEvent waiter_;
@@ -292,6 +317,7 @@ RequestCoordinatorTest::RequestCoordinatorTest()
     : last_get_requests_result_(RequestQueue::GetRequestsResult::STORE_FAILURE),
       task_runner_(new base::TestMockTimeTaskRunner),
       task_runner_handle_(task_runner_),
+      nqe_(new NetworkQualityEstimatorStub()),

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

Is this not leaking memory? May be you can store it as a unique_ptr in 
NetworkQualityEstimatorStub so that it gets deleted automatically.

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

Done.

       offliner_(nullptr),
       waiter_(base::WaitableEvent::ResetPolicy::MANUAL,
               base::WaitableEvent::InitialState::NOT_SIGNALED) {}
@@ -310,7 +336,7 @@ void RequestCoordinatorTest::SetUp() {
   std::unique_ptr<Scheduler> scheduler_stub(new SchedulerStub());
   coordinator_.reset(new RequestCoordinator(
       std::move(policy), std::move(factory), std::move(queue),
-      std::move(scheduler_stub)));
+      std::move(scheduler_stub), nqe_));
   coordinator_->AddObserver(&observer_);
 }
 
@@ -358,6 +384,8 @@ TEST_F(RequestCoordinatorTest, StartProcessingWithNoRequests) {
 }
 
 TEST_F(RequestCoordinatorTest, StartProcessingWithRequestInProgress) {
+  SetEffectiveConnectionTypeForTesting(

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

Maybe set up a default type in the ::SetUp function to OFFLINE, and only
override it in the tests that need a different setting.

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

Done.

+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_OFFLINE);
   SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE);
   // Put the request on the queue.
@@ -962,6 +990,8 @@ TEST_F(RequestCoordinatorTest, RemoveRequest) {
 
 TEST_F(RequestCoordinatorTest,
        SavePageStartsProcessingWhenConnectedAndNotLowEndDevice) {
+  SetEffectiveConnectionTypeForTesting(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G);
   SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType::CONNECTION_3G);
   EXPECT_TRUE(coordinator()->SavePageLater(kUrl1, kClientId1, kUserRequested));
@@ -977,6 +1007,8 @@ TEST_F(RequestCoordinatorTest,
 }
 
 TEST_F(RequestCoordinatorTest, SavePageDoesntStartProcessingWhenDisconnected) {
+  SetEffectiveConnectionTypeForTesting(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_OFFLINE);

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

Maybe EFFECTIVE_CONNECTION_TYPE_SLOW_2G (unless we decide that 2G slow should
still be started).

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

Added ...WhenPoorlyConnected test variant.

   SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE);
   EXPECT_TRUE(coordinator()->SavePageLater(kUrl1, kClientId1, kUserRequested));
@@ -986,6 +1018,8 @@ TEST_F(RequestCoordinatorTest, SavePageDoesntStartProcessingWhenDisconnected) {
 
 TEST_F(RequestCoordinatorTest,
        ResumeStartsProcessingWhenConnectedAndNotLowEndDevice) {
+  SetEffectiveConnectionTypeForTesting(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_OFFLINE);
   SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE);
 
@@ -1014,6 +1048,8 @@ TEST_F(RequestCoordinatorTest,
   PumpLoop();
 
   // Now simulate being connected.
+  SetEffectiveConnectionTypeForTesting(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G);
   SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType::CONNECTION_3G);