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();
-  }
-
-  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) {}
 
   base::Time GetTimeNow() const override { return GetTimeNowForTest(); }
 
   AccessTokenStore* NewAccessTokenStore() override {
     return access_token_store_.get();
   }
 
   std::unique_ptr<LocationProvider> NewNetworkLocationProvider(
       AccessTokenStore* access_token_store,
       net::URLRequestContextGetter* context,
       const GURL& url,
       const base::string16& access_token) override {
     cell_ = new MockLocationProvider;
     return base::WrapUnique(cell_);
   }
 
   std::unique_ptr<LocationProvider> NewSystemLocationProvider() override {
     gps_ = new MockLocationProvider;
     return base::WrapUnique(gps_);
   }
 
   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
   void SetUp() override {
     access_token_store_ = new NiceMock<FakeAccessTokenStore>;
     observer_.reset(new MockLocationObserver);
   }
 
   // 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) {

[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
   void TearDown() override {}
 
   void CheckLastPositionInfo(double latitude,
                              double longitude,
                              double accuracy) {
     Geoposition geoposition = observer_->last_position_;
     EXPECT_TRUE(geoposition.Validate());
@@ skipping 10 matching lines @@
 
   MockLocationProvider* cell() {
     return arbitrator_->cell_;
   }
 
   MockLocationProvider* gps() {
     return arbitrator_->gps_;
   }
 
   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_;
 };
 
 TEST_F(GeolocationLocationArbitratorTest, CreateDestroy) {
   EXPECT_TRUE(access_token_store_.get());
   InitializeArbitrator(true /* use_fake_delegate */);
   EXPECT_TRUE(arbitrator_);
   arbitrator_.reset();
   SUCCEED();
@@ skipping 238 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