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

components/offline_pages/background/request_coordinator_unittest.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 <memory>
 #include <utility>
 #include <vector>
 
@@ skipping 109 matching lines @@
     if (offliner_.get() == nullptr) {
       offliner_.reset(new OfflinerStub());
     }
     return offliner_.get();
   }
 
  private:
   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()
       : added_called_(false),
         completed_called_(false),
         changed_called_(false),
         last_status_(RequestCoordinator::BackgroundSavePageResult::SUCCESS),
         state_(SavePageRequest::RequestState::PRERENDERING) {}
 
   void Clear() {
@@ skipping 99 matching lines @@
 
   void EnableOfflinerCallback(bool enable) {
     offliner_->enable_callback(enable);
   }
 
   void SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType connection) {
     coordinator()->SetNetworkConditionsForTest(connection);
   }
 
+  void SetEffectiveConnectionTypeForTesting(net::EffectiveConnectionType type) {
+    nqe_->SetEffectiveConnectionTypeForTesting(type);
+  }
+
   void ScheduleForTest() { coordinator_->ScheduleAsNeeded(); }
 
   void CallRequestNotPicked(bool non_user_requested_tasks_remaining) {
     coordinator_->RequestNotPicked(non_user_requested_tasks_remaining);
   }
 
   void SetOfflinerTimeoutForTest(const base::TimeDelta& timeout) {
     coordinator_->SetOfflinerTimeoutForTest(timeout);
   }
 
@@ skipping 16 matching lines @@
   bool OfflinerWasCanceled() const { return offliner_->cancel_called(); }
 
   ObserverStub observer() { return observer_; }
 
  private:
   RequestQueue::GetRequestsResult last_get_requests_result_;
   RequestQueue::UpdateMultipleRequestResults last_remove_results_;
   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_;
   ObserverStub observer_;
 };
 
 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) {}
 
 RequestCoordinatorTest::~RequestCoordinatorTest() {}
 
 void RequestCoordinatorTest::SetUp() {
   std::unique_ptr<OfflinerPolicy> policy(new OfflinerPolicy());
   std::unique_ptr<OfflinerFactory> factory(new OfflinerFactoryStub());
   // Save the offliner for use by the tests.
   offliner_ =
       reinterpret_cast<OfflinerStub*>(factory->GetOffliner(policy.get()));
   std::unique_ptr<RequestQueueInMemoryStore>
       store(new RequestQueueInMemoryStore());
   std::unique_ptr<RequestQueue> queue(new RequestQueue(std::move(store)));
   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_);
 }
 
 void RequestCoordinatorTest::PumpLoop() {
   task_runner_->RunUntilIdle();
 }
 
 void RequestCoordinatorTest::GetRequestsDone(
     RequestQueue::GetRequestsResult result,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
@@ skipping 27 matching lines @@
   DeviceConditions device_conditions(false, 75,
                                      net::NetworkChangeNotifier::CONNECTION_3G);
   base::Callback<void(bool)> callback =
       base::Bind(
           &RequestCoordinatorTest::EmptyCallbackFunction,
           base::Unretained(this));
   EXPECT_TRUE(coordinator()->StartProcessing(device_conditions, callback));
 }
 
 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.
   EXPECT_TRUE(coordinator()->SavePageLater(kUrl1, kClientId1, kUserRequested));
 
   // Set up for the call to StartProcessing by building arguments.
   DeviceConditions device_conditions(
       false, 75, net::NetworkChangeNotifier::CONNECTION_3G);
   base::Callback<void(bool)> callback =
       base::Bind(&RequestCoordinatorTest::EmptyCallbackFunction,
@@ skipping 584 matching lines @@
 
   EXPECT_TRUE(observer().completed_called());
   EXPECT_EQ(RequestCoordinator::BackgroundSavePageResult::REMOVED,
             observer().last_status());
   EXPECT_EQ(1UL, last_remove_results().size());
   EXPECT_EQ(kRequestId1, std::get<0>(last_remove_results().at(0)));
 }
 
 TEST_F(RequestCoordinatorTest,
        SavePageStartsProcessingWhenConnectedAndNotLowEndDevice) {
+  SetEffectiveConnectionTypeForTesting(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G);
   SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType::CONNECTION_3G);
   EXPECT_TRUE(coordinator()->SavePageLater(kUrl1, kClientId1, kUserRequested));
   PumpLoop();
 
   // Now whether processing triggered immediately depends on whether test
   // is run on svelte device or not.
   if (base::SysInfo::IsLowEndDevice()) {
     EXPECT_FALSE(is_busy());
   } else {
     EXPECT_TRUE(is_busy());
   }
 }
 
 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));
   PumpLoop();
   EXPECT_FALSE(is_busy());
 }
 
 TEST_F(RequestCoordinatorTest,
        ResumeStartsProcessingWhenConnectedAndNotLowEndDevice) {
+  SetEffectiveConnectionTypeForTesting(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_OFFLINE);
   SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE);
 
   // Add a request to the queue.
   offline_pages::SavePageRequest request1(kRequestId1, kUrl1, kClientId1,
                                           base::Time::Now(), kUserRequested);
   coordinator()->queue()->AddRequest(
       request1, base::Bind(&RequestCoordinatorTest::AddRequestDone,
                            base::Unretained(this)));
   PumpLoop();
   EXPECT_FALSE(is_busy());
 
   // Pause the request.
   std::vector<int64_t> request_ids;
   request_ids.push_back(kRequestId1);
   coordinator()->PauseRequests(request_ids);
   PumpLoop();
 
   // Resume the request while disconnected.
   coordinator()->ResumeRequests(request_ids);
   PumpLoop();
   EXPECT_FALSE(is_busy());
 
   // Pause the request again.
   coordinator()->PauseRequests(request_ids);
   PumpLoop();
 
   // Now simulate being connected.
+  SetEffectiveConnectionTypeForTesting(
+      net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_3G);
   SetNetworkConditionsForTest(
       net::NetworkChangeNotifier::ConnectionType::CONNECTION_3G);
 
   // Resume the request while connected.
   coordinator()->ResumeRequests(request_ids);
   EXPECT_FALSE(is_busy());
   PumpLoop();
 
   // Now whether processing triggered immediately depends on whether test
   // is run on svelte device or not.
   if (base::SysInfo::IsLowEndDevice()) {
     EXPECT_FALSE(is_busy());
   } else {
     EXPECT_TRUE(is_busy());
   }
 }
 
 }  // namespace offline_pages