Geolocation: extract ContentBrowserClient methods specific to Geolocation into a class

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 b5429c86d47a650add0b6250771865b3b026ee70..61547ccde1c1823efa167926866909430685b62b 100644
--- a/content/browser/geolocation/location_arbitrator_impl_unittest.cc
+++ b/content/browser/geolocation/location_arbitrator_impl_unittest.cc
@@ -66,37 +66,38 @@ void SetReferencePosition(MockLocationProvider* provider) {
 
 namespace {
 
-class GeolocationContentBrowserClient : public TestContentBrowserClient {
+class FakeDelegate : public GeolocationProvider::Delegate {
  public:
-  GeolocationContentBrowserClient() {}
+  FakeDelegate() = default;
 
+  bool UseNetworkLocationProviders() override { return use_network_; }
   void set_use_network(bool use_network) { use_network_ = use_network; }
 
   LocationProvider* OverrideSystemLocationProvider() override {
-    provider_ = new MockLocationProvider;
-    return provider_;
+    if (!location_provider_)
+      location_provider_ = base::WrapUnique(new MockLocationProvider);
+    return location_provider_.get();
+  }
+  LocationProvider* location_provider() const {

[Michael van Ouwerkerk, 2016/06/28 11:17:07]

nit: blank line above this one

[mcasas, 2016/06/28 16:51:25]

Done.

+    return location_provider_.get();
   }
-
-  bool UseNetworkLocationProviders() override { return use_network_; }
-
-  // This provider does not own the object. It is returned by
-  // GeolocationLocationAribtratorTest::GetSystemLocationProviderOverride().
-  // The caller takes ownership. This is just a reference we can use for
-  // mocking purposes.
-  MockLocationProvider* provider_ = nullptr;
 
  private:
   bool use_network_ = true;
+  std::unique_ptr<LocationProvider> location_provider_;

[Michael van Ouwerkerk, 2016/06/28 11:17:08]

nit: consider renaming to system_location_provider_ for clarity.

[mcasas, 2016/06/28 16:51:25]

Done.

 
-  DISALLOW_COPY_AND_ASSIGN(GeolocationContentBrowserClient);
+  DISALLOW_COPY_AND_ASSIGN(FakeDelegate);
 };
 
 class TestingLocationArbitrator : public LocationArbitratorImpl {
  public:
   TestingLocationArbitrator(
       const LocationArbitratorImpl::LocationUpdateCallback& callback,
-      AccessTokenStore* access_token_store)
-      : LocationArbitratorImpl(callback),
+      AccessTokenStore* access_token_store,
+      GeolocationProvider::Delegate* delegate,
+      bool is_fake_delegate)
+      : LocationArbitratorImpl(callback, delegate),
+        is_fake_delegate_(is_fake_delegate),
         cell_(nullptr),
         gps_(nullptr),
         access_token_store_(access_token_store) {}
@@ -121,6 +122,18 @@ class TestingLocationArbitrator : public LocationArbitratorImpl {
     return base::WrapUnique(gps_);
   }
 
+  FakeDelegate* GetFakeDelegate() {
+    DCHECK(is_fake_delegate_);
+    return static_cast<FakeDelegate*>(GetDelegateForTesting());
+  }
+
+  LocationProvider* GetLocationProvider() {
+    if (is_fake_delegate_)
+      return GetFakeDelegate()->location_provider();
+    return GetDelegateForTesting()->OverrideSystemLocationProvider();
+  }
+
+  const bool is_fake_delegate_;

[Michael van Ouwerkerk, 2016/06/28 11:17:07]

nit: blank line below this one

[mcasas, 2016/06/28 16:51:25]

Done.

   // 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
@@ -141,12 +154,20 @@ class GeolocationLocationArbitratorTest : public testing::Test {
   void SetUp() override {
     access_token_store_ = new NiceMock<FakeAccessTokenStore>;
     observer_.reset(new MockLocationObserver);
-    LocationArbitratorImpl::LocationUpdateCallback callback =
+  }
+
+  // There are two types of test cases: those using FakeDelegate 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 FakeDelegate()
+                                      : new GeolocationProvider::Delegate);
+    const 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 GeolocationContentBrowserClient);
+    arbitrator_.reset(
+        new TestingLocationArbitrator(callback, access_token_store_.get(),
+                                      delegate_.get(), use_fake_delegate));
   }
 
   // testing::Test
@@ -177,25 +198,27 @@ class GeolocationLocationArbitratorTest : public testing::Test {
   }
 
   MockLocationProvider* GetSystemLocationProviderOverride() {
-    return override_content_browser_client_->provider_;
+    return static_cast<MockLocationProvider*>(
+        arbitrator_->GetLocationProvider());
   }
 
   scoped_refptr<FakeAccessTokenStore> access_token_store_;
   std::unique_ptr<MockLocationObserver> observer_;
   std::unique_ptr<TestingLocationArbitrator> arbitrator_;
+  std::unique_ptr<GeolocationProvider::Delegate> delegate_;
   base::MessageLoop loop_;
-  std::unique_ptr<GeolocationContentBrowserClient>
-      override_content_browser_client_;
 };
 
 TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) {
   EXPECT_TRUE(access_token_store_.get());
+  InitializeArbitrator(true /* use_fake_delegate */);
   EXPECT_TRUE(arbitrator_);
   arbitrator_.reset();
   SUCCEED();
 }
 
 TEST_F(GeolocationLocationArbitratorTest, OnPermissionGranted) {
+  InitializeArbitrator(false /* use_fake_delegate */);
   EXPECT_FALSE(arbitrator_->HasPermissionBeenGranted());
   arbitrator_->OnPermissionGranted();
   EXPECT_TRUE(arbitrator_->HasPermissionBeenGranted());
@@ -207,6 +230,7 @@ TEST_F(GeolocationLocationArbitratorTest, OnPermissionGranted) {
 }
 
 TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
+  InitializeArbitrator(false /* use_fake_delegate */);
   ASSERT_TRUE(access_token_store_.get());
   ASSERT_TRUE(arbitrator_);
 
@@ -243,18 +267,17 @@ TEST_F(GeolocationLocationArbitratorTest, NormalUsage) {
 }
 
 TEST_F(GeolocationLocationArbitratorTest, CustomSystemProviderOnly) {
-  override_content_browser_client_->set_use_network(false);
-  SetBrowserClientForTesting(override_content_browser_client_.get());
+  InitializeArbitrator(true /* use_fake_delegate */);
+  arbitrator_->GetFakeDelegate()->set_use_network(false);
   ASSERT_TRUE(arbitrator_);
 
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
   arbitrator_->StartProviders(false);
 
   ASSERT_FALSE(cell());
   EXPECT_FALSE(gps());
-  EXPECT_TRUE(GetSystemLocationProviderOverride());
+  ASSERT_TRUE(GetSystemLocationProviderOverride());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY,
             GetSystemLocationProviderOverride()->state_);
   EXPECT_FALSE(observer_->last_position_.Validate());
@@ -277,13 +300,12 @@ TEST_F(GeolocationLocationArbitratorTest, CustomSystemProviderOnly) {
 
 TEST_F(GeolocationLocationArbitratorTest,
        CustomSystemAndDefaultNetworkProviders) {
-  override_content_browser_client_->set_use_network(true);
-  content::SetBrowserClientForTesting(override_content_browser_client_.get());
+  InitializeArbitrator(true /* use_fake_delegate */);
+  arbitrator_->GetFakeDelegate()->set_use_network(true);
   ASSERT_TRUE(arbitrator_);
 
   EXPECT_FALSE(cell());
   EXPECT_FALSE(gps());
-  EXPECT_FALSE(GetSystemLocationProviderOverride());
   arbitrator_->StartProviders(false);
 
   EXPECT_TRUE(access_token_store_->access_token_map_.empty());
@@ -292,7 +314,7 @@ TEST_F(GeolocationLocationArbitratorTest,
 
   ASSERT_TRUE(cell());
   EXPECT_FALSE(gps());
-  EXPECT_TRUE(GetSystemLocationProviderOverride());
+  ASSERT_TRUE(GetSystemLocationProviderOverride());
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY,
             GetSystemLocationProviderOverride()->state_);
   EXPECT_EQ(MockLocationProvider::LOW_ACCURACY, cell()->state_);
@@ -314,6 +336,7 @@ TEST_F(GeolocationLocationArbitratorTest,
 }
 
 TEST_F(GeolocationLocationArbitratorTest, SetObserverOptions) {
+  InitializeArbitrator(false /* use_fake_delegate */);
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
@@ -331,6 +354,7 @@ TEST_F(GeolocationLocationArbitratorTest, SetObserverOptions) {
 }
 
 TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
+  InitializeArbitrator(false /* use_fake_delegate */);
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());
@@ -408,6 +432,7 @@ TEST_F(GeolocationLocationArbitratorTest, Arbitration) {
 }
 
 TEST_F(GeolocationLocationArbitratorTest, TwoOneShotsIsNewPositionBetter) {
+  InitializeArbitrator(false /* use_fake_delegate */);
   arbitrator_->StartProviders(false);
   access_token_store_->NotifyDelegateTokensLoaded();
   ASSERT_TRUE(cell());