Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider return unique_ptr

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 "base/memory/ptr_util.h"
@@ skipping 56 matching lines @@
 
 namespace {
 
 class FakeGeolocationDelegate : public GeolocationDelegate {
  public:
   FakeGeolocationDelegate() = default;
 
   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);
 };
 
 class TestingLocationArbitrator : public LocationArbitratorImpl {
  public:
   TestingLocationArbitrator(
       const LocationArbitratorImpl::LocationUpdateCallback& callback,
       AccessTokenStore* access_token_store,
       GeolocationDelegate* delegate,
@@ skipping 24 matching lines @@
     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();

[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_;
 
   // 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>;
@@ skipping 300 matching lines @@
 
   // 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);
 }
 
 }  // namespace content