Refactor to make BlimpLocationProvider accessible to content layer.

content/browser/geolocation/location_arbitrator_impl_unittest.cc

 // Copyright (c) 2012 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 "content/browser/geolocation/location_arbitrator_impl.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #include "content/browser/geolocation/fake_access_token_store.h"
 #include "content/browser/geolocation/mock_location_provider.h"
 #include "content/public/common/geoposition.h"
+#include "content/test/test_content_browser_client.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::NiceMock;
 
 namespace content {
 
 class MockLocationObserver {
  public:
   // Need a vtable for GMock.
@@ skipping 34 matching lines @@
   ASSERT_TRUE(position.Validate());
   provider->HandlePositionChanged(position);
 }
 
 void SetReferencePosition(MockLocationProvider* provider) {
   SetPositionFix(provider, 51.0, -0.1, 400);
 }
 
 namespace {
 
+class OverrideContentBrowserClient : public TestContentBrowserClient {

[Michael van Ouwerkerk, 2016/06/09 13:03:11]

The name of this class trips me up a bit, as I read Override as verb when at the
start of the symbol name... making it look like a method name. How about
GeolocationTestContentBrowserClient or FakeContentBrowserClient?

[CJ, 2016/06/09 20:31:50]

Done.

+ public:
+  void SetUseNetwork(bool use_network) {
+    use_network_ = use_network;
+  }
+
+  LocationProvider* OverrideSystemLocationProvider() override {
+    provider_ = new MockLocationProvider;
+    return provider_;
+  }
+
+  bool UseNetworkLocationProviders() override {
+    return use_network_;
+  }
+
+  MockLocationProvider* provider_ = nullptr;
+ private:

[Michael van Ouwerkerk, 2016/06/09 13:03:10]

nit: I would expect to see a blank line before "private:". Did you run "git cl
format"?

[CJ, 2016/06/09 20:31:50]

Done.

+  bool use_network_ = true;
+};
+
 class TestingLocationArbitrator : public LocationArbitratorImpl {
  public:
   TestingLocationArbitrator(
       const LocationArbitratorImpl::LocationUpdateCallback& callback,
       AccessTokenStore* access_token_store)
       : LocationArbitratorImpl(callback),
-        cell_(NULL),
-        gps_(NULL),
         access_token_store_(access_token_store) {
+          cell_ = nullptr;

[Michael van Ouwerkerk, 2016/06/09 13:03:10]

Why move these inside the constructor body? Generally, prefer using the
initialization list. +1 for using nullptr though!

[CJ, 2016/06/09 20:31:50]

Done.

+          gps_ = nullptr;
   }
 
   base::Time GetTimeNow() const override { return GetTimeNowForTest(); }
 
   AccessTokenStore* NewAccessTokenStore() override {
     return access_token_store_.get();
   }
 
   LocationProvider* NewNetworkLocationProvider(
       AccessTokenStore* access_token_store,
       net::URLRequestContextGetter* context,
       const GURL& url,
       const base::string16& access_token) override {
-    return new MockLocationProvider(&cell_);
+    cell_ = new MockLocationProvider;
+    return cell_;
   }
 
   LocationProvider* NewSystemLocationProvider() override {
-    return new MockLocationProvider(&gps_);
+    gps_ = new MockLocationProvider;
+    return gps_;
   }
 
-  // Two location providers, with nice short names to make the tests more
+  // Three location providers, with nice short names to make the tests more

[Michael van Ouwerkerk, 2016/06/09 13:03:11]

Just two again.

[CJ, 2016/06/09 20:31:50]

Done.

   // readable. Note |gps_| will only be set when there is a high accuracy
   // observer registered (and |cell_| when there's at least one observer of any
-  // type).
+  // type). |override_| will be included only if the ContentBrowser specifies

[Michael van Ouwerkerk, 2016/06/09 13:03:10]

|override_| is gone again

[CJ, 2016/06/09 20:31:50]

Done.

+  // for it.
   MockLocationProvider* cell_;
   MockLocationProvider* gps_;
   scoped_refptr<AccessTokenStore> access_token_store_;
 };
 
 }  // namespace
 
 class GeolocationLocationArbitratorTest : public testing::Test {
  protected:
   // testing::Test
   void SetUp() override {
     access_token_store_ = new NiceMock<FakeAccessTokenStore>;
     observer_.reset(new MockLocationObserver);
     LocationArbitratorImpl::LocationUpdateCallback callback =
         base::Bind(&MockLocationObserver::OnLocationUpdate,
                    base::Unretained(observer_.get()));
     arbitrator_.reset(new TestingLocationArbitrator(
         callback, access_token_store_.get()));
+    override_content_browser_client_.reset(new OverrideContentBrowserClient());
   }
 
   // testing::Test
   void TearDown() override {}
 
   void CheckLastPositionInfo(double latitude,
                              double longitude,
                              double accuracy) {
     Geoposition geoposition = observer_->last_position_;
     EXPECT_TRUE(geoposition.Validate());
     EXPECT_DOUBLE_EQ(latitude, geoposition.latitude);
     EXPECT_DOUBLE_EQ(longitude, geoposition.longitude);
     EXPECT_DOUBLE_EQ(accuracy, geoposition.accuracy);
   }
 
   base::TimeDelta SwitchOnFreshnessCliff() {
     // Add 1, to ensure it meets any greater-than test.
     return base::TimeDelta::FromMilliseconds(
         LocationArbitratorImpl::kFixStaleTimeoutMilliseconds + 1);
   }
 
   MockLocationProvider* cell() {
     return arbitrator_->cell_;
   }
 
   MockLocationProvider* gps() {
     return arbitrator_->gps_;
   }
 
+  MockLocationProvider* overrider() {

[Michael van Ouwerkerk, 2016/06/09 13:03:11]

Two things here:
1) This is not a simple member getter, so it should be named in the GetFoo
style.
2) Something like GetSystemLocationProviderOverride would be more meaningful,
though admittedly much more verbose.

[CJ, 2016/06/09 20:31:50]

Done.

+    return override_content_browser_client_->provider_;
+  }
+
   scoped_refptr<FakeAccessTokenStore> access_token_store_;
   std::unique_ptr<MockLocationObserver> observer_;
   std::unique_ptr<TestingLocationArbitrator> arbitrator_;
   base::MessageLoop loop_;
+  std::unique_ptr<OverrideContentBrowserClient>
+      override_content_browser_client_;
 };
 
 TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) {
   EXPECT_TRUE(access_token_store_.get());
   EXPECT_TRUE(arbitrator_ != NULL);
   arbitrator_.reset();
   SUCCEED();
 }
 
 TEST_F(GeolocationLocationArbitratorTest, OnPermissionGranted) {
   EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
   arbitrator_->OnPermissionGranted();
   EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
   // Can't check the provider has been notified without going through the
   // motions to create the provider (see next test).
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
+  EXPECT_FALSE(overrider());
 }
 
 TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
   ASSERT_TRUE(access_token_store_.get());
   ASSERT_TRUE(arbitrator_ != NULL);
 
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
+  EXPECT_FALSE(overrider());
   arbitrator_->StartProviders(false);
 
   EXPECT_TRUE(access_token_store_->access_token_map_.empty());
   EXPECT_TRUE(access_token_store_->access_token_map_.empty());
 
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   EXPECT_TRUE(gps());
+  EXPECT_FALSE(overrider());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, gps()->state_);
   EXPECT_FALSE(observer_->last_position_.Validate());
   EXPECT_EQ(Geoposition::ERROR_CODE_NONE,
             observer_->last_position_.error_code);
 
   SetReferencePosition(cell());
 
   EXPECT_TRUE(observer_->last_position_.Validate() ||
               observer_->last_position_.error_code !=
                   Geoposition::ERROR_CODE_NONE);
   EXPECT_EQ(cell()->position_.latitude,
             observer_->last_position_.latitude);
 
   EXPECT_FALSE(cell()->is_permission_granted_);
   EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
   arbitrator_->OnPermissionGranted();
   EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
   EXPECT_TRUE(cell()->is_permission_granted_);
 }
 
+TEST_F(GeolocationLocationArbitratorTest, OverrideNoNetworkUsage) {
+  override_content_browser_client_->SetUseNetwork(false);
+  content::SetBrowserClientForTesting(override_content_browser_client_.get());

[Michael van Ouwerkerk, 2016/06/09 13:03:11]

nit: I think there's no need for "content::" as we are in that namespace.

[CJ, 2016/06/09 20:31:50]

Done.

+  ASSERT_TRUE(arbitrator_ != NULL);

[Michael van Ouwerkerk, 2016/06/09 13:03:11]

nit: use nullptr in new code, but even simpler to just write
"ASSERT_TRUE(arbitrator_);"

[CJ, 2016/06/09 20:31:50]

Done.

+
+  EXPECT_FALSE(cell());
+  EXPECT_FALSE(gps());
+  EXPECT_FALSE(overrider());
+  arbitrator_->StartProviders(false);
+
+  ASSERT_FALSE(cell());
+  EXPECT_FALSE(gps());
+  EXPECT_TRUE(overrider());
+  EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, overrider()->state_);
+  EXPECT_FALSE(observer_->last_position_.Validate());
+  EXPECT_EQ(Geoposition::ERROR_CODE_NONE,
+            observer_->last_position_.error_code);
+
+  SetReferencePosition(overrider());
+
+  EXPECT_TRUE(observer_->last_position_.Validate() ||
+              observer_->last_position_.error_code !=
+                  Geoposition::ERROR_CODE_NONE);
+  EXPECT_EQ(overrider()->position_.latitude,
+            observer_->last_position_.latitude);
+
+  EXPECT_FALSE(overrider()->is_permission_granted_);
+  EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
+  arbitrator_->OnPermissionGranted();
+  EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
+  EXPECT_TRUE(overrider()->is_permission_granted_);
+}
+
+TEST_F(GeolocationLocationArbitratorTest, OverrideAndNetworkUsage) {
+  override_content_browser_client_->SetUseNetwork(true);
+  content::SetBrowserClientForTesting(override_content_browser_client_.get());

[Michael van Ouwerkerk, 2016/06/09 13:03:11]

nit: I think there's no need for "content::" as we are in that namespace.

[CJ, 2016/06/09 20:31:50]

Done.

+  ASSERT_TRUE(arbitrator_ != NULL);

[Michael van Ouwerkerk, 2016/06/09 13:03:10]

nit: use nullptr in new code, but even simpler to just write
"ASSERT_TRUE(arbitrator_);"

[CJ, 2016/06/09 20:31:50]

Done.

+
+  EXPECT_FALSE(cell());
+  EXPECT_FALSE(gps());
+  EXPECT_FALSE(overrider());
+  arbitrator_->StartProviders(false);
+
+  EXPECT_TRUE(access_token_store_->access_token_map_.empty());
+  EXPECT_TRUE(access_token_store_->access_token_map_.empty());

[Michael van Ouwerkerk, 2016/06/09 13:03:11]

delete this duplicate line

[CJ, 2016/06/09 20:31:50]

Was wondering about it since it was in the original test. Removing there too.

+
+  access_token_store_->NotifyDelegateTokensLoaded();
+
+  ASSERT_TRUE(cell());
+  EXPECT_FALSE(gps());
+  EXPECT_TRUE(overrider());
+  EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, overrider()->state_);
+  EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
+  EXPECT_FALSE(observer_->last_position_.Validate());
+  EXPECT_EQ(Geoposition::ERROR_CODE_NONE,
+            observer_->last_position_.error_code);
+
+  SetReferencePosition(cell());
+
+  EXPECT_TRUE(observer_->last_position_.Validate() ||
+              observer_->last_position_.error_code !=
+                  Geoposition::ERROR_CODE_NONE);
+  EXPECT_EQ(cell()->position_.latitude,
+            observer_->last_position_.latitude);
+
+  EXPECT_FALSE(cell()->is_permission_granted_);
+  EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
+  arbitrator_->OnPermissionGranted();
+  EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
+  EXPECT_TRUE(cell()->is_permission_granted_);
+}
+
 TEST_F(GeolocationLocationArbitratorTest, SetObserverOptions) {
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
+  EXPECT_FALSE(overrider());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, gps()->state_);
   SetReferencePosition(cell());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, gps()->state_);
   arbitrator_->StartProviders(true);
   EXPECT_EQ(MockLocationProvider::HIGH_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::HIGH_ACCURACY, gps()->state_);
+  EXPECT_FALSE(overrider());
 }
 
 TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
+  EXPECT_FALSE(overrider());
 
   SetPositionFix(cell(), 1, 2, 150);
 
   // First position available
   EXPECT_TRUE(observer_->last_position_.Validate());
   CheckLastPositionInfo(1, 2, 150);
 
   SetPositionFix(gps(), 3, 4, 50);
 
   // More accurate fix available
@@ skipping 24 matching lines @@
   // 5 mins later switch cells while walking. Stay on gps.
   AdvanceTimeNow(base::TimeDelta::FromMinutes(5));
   SetPositionFix(cell(), 3.567832, 139.634648, 300);
   SetPositionFix(gps(), 3.5677675, 139.632314, 50);
   CheckLastPositionInfo(3.5677675, 139.632314, 50);
 
   // Ride train and gps signal degrades slightly. Stay on fresher gps
   AdvanceTimeNow(base::TimeDelta::FromMinutes(5));
   SetPositionFix(gps(), 3.5679026, 139.634777, 300);
   CheckLastPositionInfo(3.5679026, 139.634777, 300);
-
-  // 14 minutes later
-  AdvanceTimeNow(base::TimeDelta::FromMinutes(14));
+  // 14 minutes later AdvanceTimeNow(base::TimeDelta::FromMinutes(14));

[Michael van Ouwerkerk, 2016/06/09 13:03:10]

Did you intend to comment out this line?

[CJ, 2016/06/09 20:31:50]

Done.

 
   // GPS reading misses a beat, but don't switch to cell yet to avoid
   // oscillating.
   SetPositionFix(gps(), 3.5659005, 139.682579, 300);
 
   AdvanceTimeNow(base::TimeDelta::FromSeconds(7));
   SetPositionFix(cell(), 3.5689579, 139.691420, 1000);
   CheckLastPositionInfo(3.5659005, 139.682579, 300);
 
   // 1 minute later
   AdvanceTimeNow(base::TimeDelta::FromMinutes(1));
 
   // Enter tunnel. Stay on fresher gps for a moment.
   SetPositionFix(cell(), 3.5657078, 139.68922, 300);
   SetPositionFix(gps(), 3.5657104, 139.690341, 300);
   CheckLastPositionInfo(3.5657104, 139.690341, 300);
 
   // 2 minutes later
   AdvanceTimeNow(base::TimeDelta::FromMinutes(2));
   // Arrive in station. Cell moves but GPS is stale. Switch to fresher cell.
   SetPositionFix(cell(), 3.5658700, 139.069979, 1000);
   CheckLastPositionInfo(3.5658700, 139.069979, 1000);
 }
 
 TEST_F(GeolocationLocationArbitratorTest, TwoOneShotsIsNewPositionBetter) {
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
+  EXPECT_FALSE(overrider());
 
   // Set the initial position.
   SetPositionFix(cell(), 3, 139, 100);
   CheckLastPositionInfo(3, 139, 100);
 
   // Restart providers to simulate a one-shot request.
   arbitrator_->StopProviders();
 
   // To test 240956, perform a throwaway alloc.
   // This convinces the allocator to put the providers in a new memory location.
-  MockLocationProvider* fakeMockProvider = NULL;
-  LocationProvider* fakeProvider =
-      new MockLocationProvider(&fakeMockProvider);
+  std::unique_ptr<MockLocationProvider> fakeMockProvider(
+      new MockLocationProvider);
 
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
 
   // Advance the time a short while to simulate successive calls.
   AdvanceTimeNow(base::TimeDelta::FromMilliseconds(5));
 
   // Update with a less accurate position to verify 240956.
   SetPositionFix(cell(), 3, 139, 150);
   CheckLastPositionInfo(3, 139, 150);
-
-  // No delete required for fakeMockProvider. It points to fakeProvider.
-  delete fakeProvider;
 }
 
 }  // namespace content