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..c2a616e586d8c4be2baad3ecdff9ea5ce5184830 100644
--- a/content/browser/geolocation/location_arbitrator_impl_unittest.cc
+++ b/content/browser/geolocation/location_arbitrator_impl_unittest.cc
@@ -74,19 +74,19 @@ 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 {
+    std::unique_ptr<LocationProvider> provider(new MockLocationProvider);
+    system_location_provider_ = provider.get();
+    return provider;
   }
 
   LocationProvider* system_location_provider() const {
-    return system_location_provider_.get();
+    return system_location_provider_;
   }
 
  private:
   bool use_network_ = true;
-  std::unique_ptr<LocationProvider> system_location_provider_;
+  LocationProvider* system_location_provider_ = nullptr;

[Wez, 2016/07/07 18:51:39]

nit: See below - do you actually need this bare-pointer and getter? I can't see
anything that actually needs it.

[mcasas, 2016/07/07 19:07:09]

Acknowledged.

 
   DISALLOW_COPY_AND_ASSIGN(FakeGeolocationDelegate);
 };
@@ -132,7 +132,9 @@ class TestingLocationArbitrator : public LocationArbitratorImpl {
   LocationProvider* GetLocationProvider() {
     if (is_fake_delegate_)
       return GetFakeGeolocationDelegate()->system_location_provider();

[Wez, 2016/07/07 18:51:39]

Why do you need this special-case for |is_fake_delegate_|?  It looks like the
GetDelegateFortesting() call immediately below will do exactly what you want?

As far as I can tell, these tests never use the ability to refer to the
previously-returned system_location_provider()?

[mcasas, 2016/07/07 19:07:09]

OverrideSystemLocationProvider() will always create
a new MockLocationProvider (l.78), whereas if we take this 
backdoor, we can check if the said function has been called,
and observe its state. It's a horrific unittest pattern, but didn't
want to touch it too much.
It's used in the CustomSystemProviderOnly and 
CustomSystemAndDefaultNetworkProviders  test 
cases: [1,2].

[1]
https://cs.chromium.org/chromium/src/content/browser/geolocation/location_arb...
[2]
https://cs.chromium.org/chromium/src/content/browser/geolocation/location_arb...

[Wez, 2016/07/07 21:47:10]

My point is that the GetLocationProvider() implementation is already doing the
horrible own-and-return-bare-pointer hack, so there is no need to duplicate that
hack in the Fake.
Are you sure? AFAICT those tests only call GetLocationProvider() (indirectly via
GetSystemLocationProviderOverride()), which is itself already doing the ugly
own-and-return-bare-pointer hack - so why do we need the Fake to *also* do that
hack?

[mcasas, 2016/07/07 23:58:41]

Ok, took your suggestion and deepened the cleanup :)

Removed the ...ForTesting() method in LocationArbitratorImpl,
made TestingLocationArbitrator a friend of it instead,
and retrieved the (Mock)LocationProvider out of the
place where it's stored, i.e. LocationArbitratorImpl::providers_

More changes, but the code is way cleaner :)
Why indeed - I moved the logic one level up in
the object hierarchy.

-    return GetDelegateForTesting()->OverrideSystemLocationProvider();
+    system_location_provider_ =
+        GetDelegateForTesting()->OverrideSystemLocationProvider();
+    return system_location_provider_.get();
   }
 
   const bool is_fake_delegate_;
@@ -141,11 +143,11 @@ class TestingLocationArbitrator : public LocationArbitratorImpl {
   // 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_;
 };