Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider return unique_ptr

Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider return unique_ptr

This CL changes the signature of GeolocationDelegate's
  virtual LocationProvider* OverrideSystemLocationProvider();
to
  virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider();

Most of the code changes are trivial except in the
unittests where the lifetime of the generated
LocationProvider needs to be taken into account.

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5
Cr-Commit-Position: refs/heads/master@{#404693}

[mcasas, 2016-07-06 22:59:54]

Description was changed from

==========
Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider
return unique_ptr

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
==========

to

==========
Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider
return unique_ptr


This CL changes the signature of GeolocationDelegate's
  virtual LocationProvider* OverrideSystemLocationProvider();
to
  virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider();

which was a cleanup-todo. 
There's no intention of adding new code except in 
the relevant unit tests infra.

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
==========

[mcasas, 2016-07-07 00:35:18]

mcasas@chromium.org changed reviewers:
+ lethalantidote@chromium.org, sl.ostapenko@samsung.com, wez@chromium.org

[mcasas, 2016-07-07 00:35:19]

lethalantidote@/wez@ PTAL

sl.ostapenko@samsung.com: FYI/PTAL -- note that this
CL changes a method signature, potentially breaking your
downstream build !!

[Wez, 2016-07-07 18:34:06]

nit: Re the CL description "which was a cleanup-todo" seems redundant :P

It's also not clear what "There's no intention of adding new code..." means. Are
you just saying that the CL only changes the signature, without any functional
changes?

[mcasas, 2016-07-07 18:37:50]

Description was changed from

==========
Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider
return unique_ptr


This CL changes the signature of GeolocationDelegate's
  virtual LocationProvider* OverrideSystemLocationProvider();
to
  virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider();

which was a cleanup-todo. 
There's no intention of adding new code except in 
the relevant unit tests infra.

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
==========

to

==========
Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider
return unique_ptr


This CL changes the signature of GeolocationDelegate's
  virtual LocationProvider* OverrideSystemLocationProvider();
to
  virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider();

Most of the code changes are trivial except in the 
unittests where the lifetime of the generated 
LocationProvider needs to be taken into account.

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
==========

[mcasas, 2016-07-07 18:38:38]

On 2016/07/07 18:34:06, Wez wrote:
> nit: Re the CL description "which was a cleanup-todo" seems redundant :P
> 
Done

> It's also not clear what "There's no intention of adding new code..." means.
Are
> you just saying that the CL only changes the signature, without any functional
> changes?

Yeah, not my best sentence. Rewritten. What I meant
is that all changes should be evident, except in the 
unit test(s) case.

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

https://codereview.chromium.org/2126893003/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_browser_client.cc (left):

https://codereview.chromium.org/2126893003/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.cc:31:
std::unique_ptr<BlimpLocationProvider> location_provider_;
You're changing the ownership semantics here - was the old code broken?

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:89:
LocationProvider* system_location_provider_ = nullptr;
nit: See below - do you actually need this bare-pointer and getter? I can't see
anything that actually needs it.

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:134: return
GetFakeGeolocationDelegate()->system_location_provider();
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()?

https://codereview.chromium.org/2126893003/diff/20001/content/public/browser/...
File content/public/browser/geolocation_delegate.h (right):

https://codereview.chromium.org/2126893003/diff/20001/content/public/browser/...
content/public/browser/geolocation_delegate.h:25: // clarify ownership,
https://crbug.com/623114.
nit: Consider cleaning this up while you're here. :P

nit: What's going on with the extra space indents on // Creates above, and //
Allows below?

nit: Can you also add a blank line after each virtual, before the comment
blocks, to make this header more readable, plz?

https://codereview.chromium.org/2126893003/diff/20001/content/public/browser/...
content/public/browser/geolocation_delegate.h:29: // Caller takes ownership of
the returned LocationProvider. FYI: Used by an
nit: No need for the ownership comment now.

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

https://codereview.chromium.org/2126893003/diff/20001/blimp/engine/app/blimp_...
File blimp/engine/app/blimp_content_browser_client.cc (left):

https://codereview.chromium.org/2126893003/diff/20001/blimp/engine/app/blimp_...
blimp/engine/app/blimp_content_browser_client.cc:31:
std::unique_ptr<BlimpLocationProvider> location_provider_;
On 2016/07/07 18:51:39, Wez wrote:
> You're changing the ownership semantics here - was the old code broken?

Probably - the result of OverrideSystemLocationProvider()
is base::WrapUnique() in [1] on ToT.


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

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:89:
LocationProvider* system_location_provider_ = nullptr;
On 2016/07/07 18:51:39, Wez wrote:
> nit: See below - do you actually need this bare-pointer and getter? I can't
see
> anything that actually needs it.

Acknowledged.

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:134: return
GetFakeGeolocationDelegate()->system_location_provider();
On 2016/07/07 18:51:39, Wez wrote:
> 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?
> 

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.

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

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...

https://codereview.chromium.org/2126893003/diff/20001/content/public/browser/...
File content/public/browser/geolocation_delegate.h (right):

https://codereview.chromium.org/2126893003/diff/20001/content/public/browser/...
content/public/browser/geolocation_delegate.h:25: // clarify ownership,
https://crbug.com/623114.
On 2016/07/07 18:51:39, Wez wrote:
> nit: Consider cleaning this up while you're here. :P
> 

Since this CL affects a downstream project, I just 
thought it easier to have two cleanups instead of 
one (moar cleanups!).

> nit: What's going on with the extra space indents on // Creates above, and //
> Allows below?

Corrected.

> 
> nit: Can you also add a blank line after each virtual, before the comment
> blocks, to make this header more readable, plz?

Done!

https://codereview.chromium.org/2126893003/diff/20001/content/public/browser/...
content/public/browser/geolocation_delegate.h:29: // Caller takes ownership of
the returned LocationProvider. FYI: Used by an
On 2016/07/07 18:51:39, Wez wrote:
> nit: No need for the ownership comment now.

Done.

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

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:134: return
GetFakeGeolocationDelegate()->system_location_provider();
On 2016/07/07 19:07:09, mcasas wrote:
> On 2016/07/07 18:51:39, Wez wrote:
> > 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?
> > 
> 
> 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.

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.

> > As far as I can tell, these tests never use the ability to refer to the
> > previously-returned system_location_provider()?
> 
> It's used in the CustomSystemProviderOnly and 
> CustomSystemAndDefaultNetworkProviders  test 
> cases: [1,2].

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?

https://codereview.chromium.org/2126893003/diff/20001/content/public/browser/...
File content/public/browser/geolocation_delegate.h (right):

https://codereview.chromium.org/2126893003/diff/20001/content/public/browser/...
content/public/browser/geolocation_delegate.h:25: // clarify ownership,
https://crbug.com/623114.
On 2016/07/07 19:07:09, mcasas wrote:
> On 2016/07/07 18:51:39, Wez wrote:
> > nit: Consider cleaning this up while you're here. :P
> > 
> 
> Since this CL affects a downstream project, I just 
> thought it easier to have two cleanups instead of 
> one (moar cleanups!).

Ah, so you're arguing that it's cleaner to fix it separately, since it'll
require updating the Chrome & Shell call-sites. Gotcha.

https://codereview.chromium.org/2126893003/diff/40001/content/public/browser/...
File content/public/browser/geolocation_delegate.h (right):

https://codereview.chromium.org/2126893003/diff/40001/content/public/browser/...
content/public/browser/geolocation_delegate.h:30: // Return nullptr to use the
default one for the platform to be created. FYI:
nit: Suggest starting a new line for the FYI: so it stands out to the reader.

[mcasas, 2016-07-07 23:37:38]

Description was changed from

==========
Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider
return unique_ptr


This CL changes the signature of GeolocationDelegate's
  virtual LocationProvider* OverrideSystemLocationProvider();
to
  virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider();

Most of the code changes are trivial except in the 
unittests where the lifetime of the generated 
LocationProvider needs to be taken into account.

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
==========

to

==========
Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider
return unique_ptr


This CL changes the signature of GeolocationDelegate's
  virtual LocationProvider* OverrideSystemLocationProvider();
to
  virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider();

Most of the code changes are trivial except in the 
unittests where the lifetime of the generated 
LocationProvider needs to be taken into account.

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[mcasas, 2016-07-07 23:40:14]

Patchset #4 (id:60001) has been deleted

[mcasas, 2016-07-07 23:58:42]

PTAL

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:134: return
GetFakeGeolocationDelegate()->system_location_provider();
On 2016/07/07 21:47:10, Wez wrote:
> On 2016/07/07 19:07:09, mcasas wrote:
> > On 2016/07/07 18:51:39, Wez wrote:
> > > 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?
> > > 
> > 
> > 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.
> 
> 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.

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 :)
> 
> > > As far as I can tell, these tests never use the ability to refer to the
> > > previously-returned system_location_provider()?
> > 
> > It's used in the CustomSystemProviderOnly and 
> > CustomSystemAndDefaultNetworkProviders  test 
> > cases: [1,2].
> 
> 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?

Why indeed - I moved the logic one level up in
the object hierarchy.

https://codereview.chromium.org/2126893003/diff/40001/content/public/browser/...
File content/public/browser/geolocation_delegate.h (right):

https://codereview.chromium.org/2126893003/diff/40001/content/public/browser/...
content/public/browser/geolocation_delegate.h:30: // Return nullptr to use the
default one for the platform to be created. FYI:
On 2016/07/07 21:47:10, Wez wrote:
> nit: Suggest starting a new line for the FYI: so it stands out to the reader.

Done.

[CJ, 2016-07-08 19:00:33]

lgtm

[mcasas, 2016-07-08 22:13:55]

mcasas@chromium.org changed reviewers:
+ nick@chromium.org

[mcasas, 2016-07-08 22:13:56]

nick@ can you please RS? (pending wez@s LGTM)

[ncarter (slow), 2016-07-08 22:20:06]

content lgtm

[Wez, 2016-07-08 23:12:58]

https://codereview.chromium.org/2126893003/diff/40001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/40001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:128:
DCHECK(is_fake_delegate_);
You don't want to remove this, otherwise someone will end up calling it when the
delegate isn't fake, surely?

See below, though, re InitializeArbitrator().

https://codereview.chromium.org/2126893003/diff/80001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/80001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:126: return
static_cast<MockLocationProvider*>(providers_.front().get());
|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?

https://codereview.chromium.org/2126893003/diff/80001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:152: void
InitializeArbitrator(bool use_fake_delegate) {
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.

https://codereview.chromium.org/2126893003/diff/80001/content/public/browser/...
File content/public/browser/geolocation_delegate.h (right):

https://codereview.chromium.org/2126893003/diff/80001/content/public/browser/...
content/public/browser/geolocation_delegate.h:18: class CONTENT_EXPORT
GeolocationDelegate {
This interface should have a virtual dtor declared in-line.

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

wez@ PTAL

https://codereview.chromium.org/2126893003/diff/80001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/80001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:126: return
static_cast<MockLocationProvider*>(providers_.front().get());
On 2016/07/08 23:12:57, Wez wrote:
> |providers_| is private, so I don't think you can access it here.
> 
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.

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

Removed in light of subsequent actions.

https://codereview.chromium.org/2126893003/diff/80001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:152: void
InitializeArbitrator(bool use_fake_delegate) {
On 2016/07/08 23:12:57, Wez wrote:
> 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.

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.

https://codereview.chromium.org/2126893003/diff/80001/content/public/browser/...
File content/public/browser/geolocation_delegate.h (right):

https://codereview.chromium.org/2126893003/diff/80001/content/public/browser/...
content/public/browser/geolocation_delegate.h:18: class CONTENT_EXPORT
GeolocationDelegate {
On 2016/07/08 23:12:58, Wez wrote:
> This interface should have a virtual dtor declared in-line.

Done.

[Wez, 2016-07-11 17:10:11]

lgtm

https://codereview.chromium.org/2126893003/diff/100001/content/browser/geoloc...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/100001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:78:
mock_location_provider_ = new MockLocationProvider;
Looks like you want to ASSERT/DCHECK(!mock_location_provider_) here.

[mcasas, 2016-07-11 17:18:19]

https://codereview.chromium.org/2126893003/diff/100001/content/browser/geoloc...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2126893003/diff/100001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:78:
mock_location_provider_ = new MockLocationProvider;
On 2016/07/11 17:10:10, Wez wrote:
> Looks like you want to ASSERT/DCHECK(!mock_location_provider_) here.

Done.

[mcasas, 2016-07-11 17:37:06]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-07-11 17:37:07]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
lethalantidote@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2126893003/#ps120001
(title: "wez@ nit")

[commit-bot: I haz the power, 2016-07-11 17:37:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-11 18:28:24]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-07-11 18:28:38]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-11 18:32:39]

Description was changed from

==========
Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider
return unique_ptr


This CL changes the signature of GeolocationDelegate's
  virtual LocationProvider* OverrideSystemLocationProvider();
to
  virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider();

Most of the code changes are trivial except in the 
unittests where the lifetime of the generated 
LocationProvider needs to be taken into account.

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Geolocation cleanup: make GeolocationDelegate::OverrideSystemLocationProvider
return unique_ptr

This CL changes the signature of GeolocationDelegate's
  virtual LocationProvider* OverrideSystemLocationProvider();
to
  virtual std::unique_ptr<LocationProvider> OverrideSystemLocationProvider();

Most of the code changes are trivial except in the
unittests where the lifetime of the generated
LocationProvider needs to be taken into account.

BUG=623132
TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5
Cr-Commit-Position: refs/heads/master@{#404693}
==========

[commit-bot: I haz the power, 2016-07-11 18:32:41]

Patchset 6 (id:??) landed as
https://crrev.com/f6da183fd9aa098f57e04f55c589db3eb31786f5
Cr-Commit-Position: refs/heads/master@{#404693}