Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider return unique_ptr

content/browser/geolocation/location_arbitrator_impl_unittest.cc

Index: content/browser/geolocation/location_arbitrator_impl_unittest.cc
diff --git a/content/browser/geolocation/location_arbitrator_impl_unittest.cc b/content/browser/geolocation/location_arbitrator_impl_unittest.cc
index 706f28a5b088347347c505fee12016adfd8718df..af76f59fc8c87a5382f0f5a69134377f0fd444d9 100644
--- a/content/browser/geolocation/location_arbitrator_impl_unittest.cc
+++ b/content/browser/geolocation/location_arbitrator_impl_unittest.cc
@@ -74,32 +74,31 @@ class FakeGeolocationDelegate : public GeolocationDelegate {
   bool UseNetworkLocationProviders() override { return use_network_; }
   void set_use_network(bool use_network) { use_network_ = use_network; }
 
-  LocationProvider* OverrideSystemLocationProvider() override {
-    if (!system_location_provider_)
-      system_location_provider_ = base::WrapUnique(new MockLocationProvider);
-    return system_location_provider_.get();
+  std::unique_ptr<LocationProvider> OverrideSystemLocationProvider() override {
+    mock_location_provider_ = new MockLocationProvider;

[Wez, 2016/07/11 17:10:10]

Looks like you want to ASSERT/DCHECK(!mock_location_provider_) here.

[mcasas, 2016/07/11 17:18:19]

Done.

+    return base::WrapUnique(mock_location_provider_);
   }
 
-  LocationProvider* system_location_provider() const {
-    return system_location_provider_.get();
+  MockLocationProvider* mock_location_provider() const {
+    return mock_location_provider_;
   }
 
  private:
   bool use_network_ = true;
-  std::unique_ptr<LocationProvider> system_location_provider_;
+  MockLocationProvider* mock_location_provider_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(FakeGeolocationDelegate);
 };
 
+}  // namespace
+
 class TestingLocationArbitrator : public LocationArbitratorImpl {
  public:
   TestingLocationArbitrator(
       const LocationArbitratorImpl::LocationUpdateCallback& callback,
       AccessTokenStore* access_token_store,
-      GeolocationDelegate* delegate,
-      bool is_fake_delegate)
+      GeolocationDelegate* delegate)
       : LocationArbitratorImpl(callback, delegate),
-        is_fake_delegate_(is_fake_delegate),
         cell_(nullptr),
         gps_(nullptr),
         access_token_store_(access_token_store) {}
@@ -124,53 +123,37 @@ class TestingLocationArbitrator : public LocationArbitratorImpl {
     return base::WrapUnique(gps_);
   }
 
-  FakeGeolocationDelegate* GetFakeGeolocationDelegate() {
-    DCHECK(is_fake_delegate_);
-    return static_cast<FakeGeolocationDelegate*>(GetDelegateForTesting());
-  }
-
-  LocationProvider* GetLocationProvider() {
-    if (is_fake_delegate_)
-      return GetFakeGeolocationDelegate()->system_location_provider();
-    return GetDelegateForTesting()->OverrideSystemLocationProvider();
-  }
-
-  const bool is_fake_delegate_;
-
   // Two location providers, with nice short names to make the tests more
   // 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).
-
   // TODO(mvanouwerkerk): rename |cell_| to |network_location_provider_| and
-  // |gps_| to |system_location_provider_|
+  // |gps_| to |gps_location_provider_|
   MockLocationProvider* cell_;
   MockLocationProvider* gps_;
+  std::unique_ptr<LocationProvider> system_location_provider_;
   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);
+    delegate_.reset(new GeolocationDelegate);
   }
 
-  // There are two types of test cases: those using FakeGeolocationDelegate and
-  // the ones exercising whatever the embedder provides. Test cases call this
-  // method to choose the appropriate one.
-  void InitializeArbitrator(bool use_fake_delegate) {
-    delegate_.reset(use_fake_delegate ? new FakeGeolocationDelegate()
-                                      : new GeolocationDelegate);
+  // Initializes |arbitrator_|, with the possibility of injecting a specific
+  // |delegate|, otherwise a default, no-op GeolocationDelegate is used.
+  void InitializeArbitrator(std::unique_ptr<GeolocationDelegate> delegate) {
+    if (delegate)
+      delegate_ = std::move(delegate);
     const LocationArbitratorImpl::LocationUpdateCallback callback =
         base::Bind(&MockLocationObserver::OnLocationUpdate,
                    base::Unretained(observer_.get()));
-    arbitrator_.reset(
-        new TestingLocationArbitrator(callback, access_token_store_.get(),
-                                      delegate_.get(), use_fake_delegate));
+    arbitrator_.reset(new TestingLocationArbitrator(
+        callback, access_token_store_.get(), delegate_.get()));
   }
 
   // testing::Test
@@ -200,11 +183,6 @@ class GeolocationLocationArbitratorTest : public testing::Test {
     return arbitrator_->gps_;
   }
 
-  MockLocationProvider* GetSystemLocationProviderOverride() {
-    return static_cast<MockLocationProvider*>(
-        arbitrator_->GetLocationProvider());
-  }
-
   scoped_refptr<FakeAccessTokenStore> access_token_store_;
   std::unique_ptr<MockLocationObserver> observer_;
   std::unique_ptr<TestingLocationArbitrator> arbitrator_;
@@ -214,14 +192,14 @@ class GeolocationLocationArbitratorTest : public testing::Test {
 
 TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) {
   EXPECT_TRUE(access_token_store_.get());
-  InitializeArbitrator(true /* use_fake_delegate */);
+  InitializeArbitrator(nullptr);
   EXPECT_TRUE(arbitrator_);
   arbitrator_.reset();
   SUCCEED();
 }
 
 TEST_F(GeolocationLocationArbitratorTest, OnPermissionGranted) {
-  InitializeArbitrator(false /* use_fake_delegate */);
+  InitializeArbitrator(nullptr);
   EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
   arbitrator_->OnPermissionGranted();
   EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
@@ -229,17 +207,15 @@ TEST_F(GeolocationLocationArbitratorTest, OnPermissionGranted) {
   // motions to create the provider (see next test).
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
 }
 
 TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
-  InitializeArbitrator(false /* use_fake_delegate */);
+  InitializeArbitrator(nullptr);
   ASSERT_TRUE(access_token_store_.get());
   ASSERT_TRUE(arbitrator_);
 
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
   arbitrator_->StartProviders(false);
 
   EXPECT_TRUE(access_token_store_->access_token_map_.empty());
@@ -247,7 +223,6 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   EXPECT_TRUE(gps());
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, gps()->state_);
   EXPECT_FALSE(observer_->last_position_.Validate());
@@ -270,8 +245,11 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
 }
 
 TEST_F(GeolocationLocationArbitratorTest, CustomSystemProviderOnly) {
-  InitializeArbitrator(true /* use_fake_delegate */);
-  arbitrator_->GetFakeGeolocationDelegate()->set_use_network(false);
+  FakeGeolocationDelegate* fake_delegate = new FakeGeolocationDelegate;
+  fake_delegate->set_use_network(false);
+
+  std::unique_ptr<GeolocationDelegate> delegate(fake_delegate);
+  InitializeArbitrator(std::move(delegate));
   ASSERT_TRUE(arbitrator_);
 
   EXPECT_FALSE(cell());
@@ -280,31 +258,34 @@ TEST_F(GeolocationLocationArbitratorTest, CustomSystemProviderOnly) {
 
   ASSERT_FALSE(cell());
   EXPECT_FALSE(gps());
-  ASSERT_TRUE(GetSystemLocationProviderOverride());
+  ASSERT_TRUE(fake_delegate->mock_location_provider());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY,
-            GetSystemLocationProviderOverride()->state_);
+            fake_delegate->mock_location_provider()->state_);
   EXPECT_FALSE(observer_->last_position_.Validate());
   EXPECT_EQ(Geoposition::ERROR_CODE_NONE, observer_->last_position_.error_code);
 
-  SetReferencePosition(GetSystemLocationProviderOverride());
+  SetReferencePosition(fake_delegate->mock_location_provider());
 
   EXPECT_TRUE(observer_->last_position_.Validate() ||
               observer_->last_position_.error_code !=
                   Geoposition::ERROR_CODE_NONE);
-  EXPECT_EQ(GetSystemLocationProviderOverride()->position_.latitude,
+  EXPECT_EQ(fake_delegate->mock_location_provider()->position_.latitude,
             observer_->last_position_.latitude);
 
-  EXPECT_FALSE(GetSystemLocationProviderOverride()->is_permission_granted_);
+  EXPECT_FALSE(fake_delegate->mock_location_provider()->is_permission_granted_);
   EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
   arbitrator_->OnPermissionGranted();
   EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
-  EXPECT_TRUE(GetSystemLocationProviderOverride()->is_permission_granted_);
+  EXPECT_TRUE(fake_delegate->mock_location_provider()->is_permission_granted_);
 }
 
 TEST_F(GeolocationLocationArbitratorTest,
        CustomSystemAndDefaultNetworkProviders) {
-  InitializeArbitrator(true /* use_fake_delegate */);
-  arbitrator_->GetFakeGeolocationDelegate()->set_use_network(true);
+  FakeGeolocationDelegate* fake_delegate = new FakeGeolocationDelegate;
+  fake_delegate->set_use_network(true);
+
+  std::unique_ptr<GeolocationDelegate> delegate(fake_delegate);
+  InitializeArbitrator(std::move(delegate));
   ASSERT_TRUE(arbitrator_);
 
   EXPECT_FALSE(cell());
@@ -317,9 +298,9 @@ TEST_F(GeolocationLocationArbitratorTest,
 
   ASSERT_TRUE(cell());
   EXPECT_FALSE(gps());
-  ASSERT_TRUE(GetSystemLocationProviderOverride());
+  ASSERT_TRUE(fake_delegate->mock_location_provider());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY,
-            GetSystemLocationProviderOverride()->state_);
+            fake_delegate->mock_location_provider()->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);
@@ -339,12 +320,11 @@ TEST_F(GeolocationLocationArbitratorTest,
 }
 
 TEST_F(GeolocationLocationArbitratorTest, SetObserverOptions) {
-  InitializeArbitrator(false /* use_fake_delegate */);
+  InitializeArbitrator(nullptr);
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, gps()->state_);
   SetReferencePosition(cell());
@@ -353,16 +333,14 @@ TEST_F(GeolocationLocationArbitratorTest, SetObserverOptions) {
   arbitrator_->StartProviders(true);
   EXPECT_EQ(MockLocationProvider::HIGH_ACCURACY, cell()->state_);
   EXPECT_EQ(MockLocationProvider::HIGH_ACCURACY, gps()->state_);
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
 }
 
 TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
-  InitializeArbitrator(false /* use_fake_delegate */);
+  InitializeArbitrator(nullptr);
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
 
   SetPositionFix(cell(), 1, 2, 150);
 
@@ -435,12 +413,11 @@ TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
 }
 
 TEST_F(GeolocationLocationArbitratorTest, TwoOneShotsIsNewPositionBetter) {
-  InitializeArbitrator(false /* use_fake_delegate */);
+  InitializeArbitrator(nullptr);
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
   ASSERT_TRUE(gps());
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
 
   // Set the initial position.
   SetPositionFix(cell(), 3, 139, 100);