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..11578dc95bfe22fa38c086a586cf7feebf82f07f 100644
--- a/content/browser/geolocation/location_arbitrator_impl_unittest.cc
+++ b/content/browser/geolocation/location_arbitrator_impl_unittest.cc
@@ -74,32 +74,25 @@ 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();
-  }
-
-  LocationProvider* system_location_provider() const {
-    return system_location_provider_.get();
+  std::unique_ptr<LocationProvider> OverrideSystemLocationProvider() override {
+    return base::WrapUnique(new MockLocationProvider);
   }
 
  private:
   bool use_network_ = true;
-  std::unique_ptr<LocationProvider> system_location_provider_;
 
   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) {}
@@ -125,32 +118,26 @@ class TestingLocationArbitrator : public LocationArbitratorImpl {
   }
 
   FakeGeolocationDelegate* GetFakeGeolocationDelegate() {
-    DCHECK(is_fake_delegate_);
-    return static_cast<FakeGeolocationDelegate*>(GetDelegateForTesting());
+    return static_cast<FakeGeolocationDelegate*>(delegate_);
   }
 
-  LocationProvider* GetLocationProvider() {
-    if (is_fake_delegate_)
-      return GetFakeGeolocationDelegate()->system_location_provider();
-    return GetDelegateForTesting()->OverrideSystemLocationProvider();
+  MockLocationProvider* GetMockLocationProvider() {
+    // Return the first location provider, which is anyway the only one.
+    return static_cast<MockLocationProvider*>(providers_.front().get());

[Wez, 2016/07/08 23:12:57]

|providers_| is private, so I don't think you can access it here.

Isn't the whole point of the arbitrator to manage multiple providers, not just
one?

[mcasas, 2016/07/09 01:24:18]

I can, because I made TestingLocationArbitrator
friend of LocationArbitratorImpl, which allows me
to remove GetDelegateForTesting() method (accessing
|delegate_| directly instead, l.121), and 
|providers_|; since the whole point of this
class is to fake part of the behaviour of its
parent and observe its state, reading those member
variables seems OK to me.
Removed in light of subsequent actions.

   }
 
-  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
@@ -163,14 +150,14 @@ class GeolocationLocationArbitratorTest : public testing::Test {
   // the ones exercising whatever the embedder provides. Test cases call this
   // method to choose the appropriate one.
   void InitializeArbitrator(bool use_fake_delegate) {

[Wez, 2016/07/08 23:12:57]

Rather than use true/false at call-sites and document the meaning with comments,
could you just have

unique_ptr<...> delegate = new FakeGeolocationDelegate();
InitializeArbitrator(std:move(delegate));

so it's clearer what's going on?

This also has the advantage that tests can add a line in-between, to configure
the fake, or take a bare pointer to it, as necessary.

[mcasas, 2016/07/09 01:24:18]

Done. Made InitializeArbitrator() accept an
overriding std::unique_ptr<LocationProvider>
(which most test cases leave to nullptr,
but others initialize to a FakeGeolocationProvider
to which they hold on -weakly).
I also added a 
FakeGeolocationDelegate::mock_location_provider()
to have (weak) access to the MockLocationProvider
generated by the said FakeGD, in order to observe
and/or modify its state.

+    use_fake_delegate_ = use_fake_delegate;
     delegate_.reset(use_fake_delegate ? new FakeGeolocationDelegate()
                                       : new GeolocationDelegate);
     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
@@ -201,13 +188,15 @@ class GeolocationLocationArbitratorTest : public testing::Test {
   }
 
   MockLocationProvider* GetSystemLocationProviderOverride() {
-    return static_cast<MockLocationProvider*>(
-        arbitrator_->GetLocationProvider());
+    if (!use_fake_delegate_)
+      return nullptr;
+    return arbitrator_->GetMockLocationProvider();
   }
 
   scoped_refptr<FakeAccessTokenStore> access_token_store_;
   std::unique_ptr<MockLocationObserver> observer_;
   std::unique_ptr<TestingLocationArbitrator> arbitrator_;
+  bool use_fake_delegate_;
   std::unique_ptr<GeolocationDelegate> delegate_;
   base::MessageLoop loop_;
 };