Geolocation: extract ContentBrowserClient methods specific to Geolocation into a class

Geolocation: extract ContentBrowserClient methods specific to Geolocation into a class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider()
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a (weak)
GeolocationProvider::Delegate* on ctor; this object
encapsulates the 3 previously mentioned  methods, that are
(partially) overriden in each Client.

This CL also changes all those implementations and
the unittests (which is quite simplified) to follow the
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

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

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

[mcasas, 2016-06-23 16:51:48]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation

BUG=
==========

to

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation

As a preliminary step to move Geolocation to //device/.

For the record, the methods are:
bool UseNetworkLocationProvider() 
 - overriden in {blimp,content}_browser_client
LocationProvider* OverrideSystemLocationProvider()
 - overriden in (a) downstream project(s)
AccessTokenStore* CreateAccessTokenStore()
 - override in {aw,cast,chrome,content,shell}_browser_client.

BUG=612334
==========

[mcasas, 2016-06-23 17:12:26]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation

As a preliminary step to move Geolocation to //device/.

For the record, the methods are:
bool UseNetworkLocationProvider() 
 - overriden in {blimp,content}_browser_client
LocationProvider* OverrideSystemLocationProvider()
 - overriden in (a) downstream project(s)
AccessTokenStore* CreateAccessTokenStore()
 - override in {aw,cast,chrome,content,shell}_browser_client.

BUG=612334
==========

to

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation

As a preliminary step to move Geolocation to //device/.

For the record, the methods are:
bool UseNetworkLocationProvider() 
 - overriden in {blimp,content}_browser_client
LocationProvider* OverrideSystemLocationProvider()
 - overriden in (a) downstream project(s)
AccessTokenStore* CreateAccessTokenStore()
 - overriden in {aw,cast,chrome,content,shell}_browser_client.

BUG=612334
==========

[mcasas, 2016-06-23 17:39:38]

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

[mcasas, 2016-06-23 17:39:38]

mvanouwerkerk@ can you plz take a first -rough- look?
Caveats:
 - name of class GeolocationProvider::ProviderOfStuff is 
 obviously TBD, can you recommend?
 - making ContentBrowserClient derive from the 
 previous class doesn't seem too good an idea, any 
 suggestions?

[mcasas, 2016-06-23 23:40:43]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation

As a preliminary step to move Geolocation to //device/.

For the record, the methods are:
bool UseNetworkLocationProvider() 
 - overriden in {blimp,content}_browser_client
LocationProvider* OverrideSystemLocationProvider()
 - overriden in (a) downstream project(s)
AccessTokenStore* CreateAccessTokenStore()
 - overriden in {aw,cast,chrome,content,shell}_browser_client.

BUG=612334
==========

to

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into
class

As a preliminary step to move Geolocation to //device/.

ContentBrowserClient defines three methods:
bool UseNetworkLocationProvider() 
 - overriden in {blimp,content}_browser_client
LocationProvider* OverrideSystemLocationProvider()
 - overriden in (a) downstream project(s) and
   blimp_content_browser_client.
AccessTokenStore* CreateAccessTokenStore()
 - overriden in {aw,cast,chrome,content,shell}_browser_client.

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a
GeolocationProviderImpl::ServiceOverrides on ctor;
this object encapsulates the 3 previous methods,
that are (partially) overriden per embedder mentioned
before. This CL also changes all those implementations
and the unittests (which is quite simplified).

BUG=612334
==========

[mcasas, 2016-06-23 23:43:06]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into
class

As a preliminary step to move Geolocation to //device/.

ContentBrowserClient defines three methods:
bool UseNetworkLocationProvider() 
 - overriden in {blimp,content}_browser_client
LocationProvider* OverrideSystemLocationProvider()
 - overriden in (a) downstream project(s) and
   blimp_content_browser_client.
AccessTokenStore* CreateAccessTokenStore()
 - overriden in {aw,cast,chrome,content,shell}_browser_client.

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a
GeolocationProviderImpl::ServiceOverrides on ctor;
this object encapsulates the 3 previous methods,
that are (partially) overriden per embedder mentioned
before. This CL also changes all those implementations
and the unittests (which is quite simplified).

BUG=612334
==========

to

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into
class

ToT ContentBrowserClient defines three methods:
bool UseNetworkLocationProvider() 
 - overriden in {blimp,content}_browser_client
LocationProvider* OverrideSystemLocationProvider()
 - overriden in (a) downstream project(s) and
   blimp_content_browser_client.
AccessTokenStore* CreateAccessTokenStore()
 - overriden in {aw,cast,chrome,content,shell}_browser_client.

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().


This CL changes LocationArbitratorImpl to get a
GeolocationProviderImpl::ServiceOverrides on ctor;
this object encapsulates the 3 previously mentioned 
methods, that are (partially) overriden in each Client.
This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

BUG=612334
TEST=all relevant unittests and browser_tests working.
==========

[mcasas, 2016-06-23 23:44:28]

On 2016/06/23 17:39:38, mcasas wrote:
> mvanouwerkerk@ can you plz take a first -rough- look?
> Caveats:
>  - name of class GeolocationProvider::ProviderOfStuff is 
>  obviously TBD, can you recommend?
>  - making ContentBrowserClient derive from the 
>  previous class doesn't seem too good an idea, any 
>  suggestions?

Update: After talking to reillyg@ I decided to bite the
bullet and do the deep refacroring, so the caveats do
not apply, but still please suggest name for the
GeolocationProvider::ServiceOverrides class.

[mcasas, 2016-06-23 23:45:21]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into
class

ToT ContentBrowserClient defines three methods:
bool UseNetworkLocationProvider() 
 - overriden in {blimp,content}_browser_client
LocationProvider* OverrideSystemLocationProvider()
 - overriden in (a) downstream project(s) and
   blimp_content_browser_client.
AccessTokenStore* CreateAccessTokenStore()
 - overriden in {aw,cast,chrome,content,shell}_browser_client.

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().


This CL changes LocationArbitratorImpl to get a
GeolocationProviderImpl::ServiceOverrides on ctor;
this object encapsulates the 3 previously mentioned 
methods, that are (partially) overriden in each Client.
This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

BUG=612334
TEST=all relevant unittests and browser_tests working.
==========

to

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider() 
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a
GeolocationProvider::ServiceOverrides on ctor;
this object encapsulates the 3 previously mentioned 
methods, that are (partially) overriden in each Client.
This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

BUG=612334
TEST=all relevant unittests and browser_tests working.
==========

[Michael van Ouwerkerk, 2016-06-24 13:57:19]

https://codereview.chromium.org/2098553002/diff/20001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/20001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:442: return new
AwGeolocationServiceOverrides();
For a getter, I would expect the client to own the instance, and instantiate it
lazily. Otherwise this method should be called
"CreateGeolocationServiceOverrides" but I wouldn't recommend going in that
direction.

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

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
content/browser/geolocation/geolocation_provider_impl.cc:17: #include
"content/public/browser/browser_thread.h"
nit: delete this duplicated include

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.h:45: // Takes ownership of
|service_overrides|.
I think that a) taking ownership should be expressed by using unique_ptr, and b)
it seems strange for the arbitrator to own it, though it does not define it.
Maybe it should be owned by the creator?

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

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:91:
DISALLOW_COPY_AND_ASSIGN(GeolocationContentBrowserClient);
Why delete this?

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

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:69: class
MockServiceOverrides : public GeolocationProvider::ServiceOverrides {
This is not really a mock (using gmock), but a fake.

https://codereview.chromium.org/2098553002/diff/20001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2098553002/diff/20001/content/content_browser...
content/content_browser.gypi:192: 'public/browser/geolocation_provider.cc',
Is this file suitable for the public content api?
https://www.chromium.org/developers/content-module/content-api

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

https://codereview.chromium.org/2098553002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:535: virtual
GeolocationProvider::ServiceOverrides*
This method must be documented, like the others in this file.

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

https://codereview.chromium.org/2098553002/diff/20001/content/public/browser/...
content/public/browser/geolocation_provider.h:33: class CONTENT_EXPORT
ServiceOverrides {
I'd suggest calling it GeolocationProvider::Delegate

https://codereview.chromium.org/2098553002/diff/20001/content/public/browser/...
content/public/browser/geolocation_provider.h:37: virtual bool
UseNetworkLocationProviders();
The first rule of the content API [1] is "content/public should contain only
interfaces, structs and (rarely) static functions" so this should be pure
virtual. When in doubt, ask the content api owners.

[1] https://www.chromium.org/developers/content-module/content-api

https://codereview.chromium.org/2098553002/diff/20001/content/public/browser/...
content/public/browser/geolocation_provider.h:40: virtual AccessTokenStore*
CreateAccessTokenStore();
If you're here anyway, consider whether this should return a unique_ptr for
ownership clarity. You could leave a TODO for doing this as a future change.

[Michael van Ouwerkerk, 2016-06-24 16:11:36]

Adding CJ and Wez to CC.

[mcasas, 2016-06-24 17:16:56]

Patchset #3 (id:40001) has been deleted

[mcasas, 2016-06-24 18:38:18]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider() 
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a
GeolocationProvider::ServiceOverrides on ctor;
this object encapsulates the 3 previously mentioned 
methods, that are (partially) overriden in each Client.
This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

BUG=612334
TEST=all relevant unittests and browser_tests working.
==========

to

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider() 
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a (weak) 
GeolocationProvider::Delegate* on ctor; this object 
encapsulates the 3 previously mentioned  methods, that are 
(partially) overriden in each Client.

This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

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

[Wez, 2016-06-24 18:40:26]

Looks like we should also CC sl.ostapenko@samsung.com since they're the one
using the original override API?

Since we're moving the override APIs anyway, can we take the opportunity to
clean up the methods e.g. LocationProvider* OverrideSystemLocationProvider() ->
unique_ptr<> GetCustomSystemLocationProvider(), for example?

[mcasas, 2016-06-24 18:55:36]

Patchset #3 (id:60001) has been deleted

[mcasas, 2016-06-24 19:08:22]

Patchset #3 (id:80001) has been deleted

[mcasas, 2016-06-24 19:22:12]

Patchset #3 (id:100001) has been deleted

[mcasas, 2016-06-24 19:25:53]

PTAL

https://codereview.chromium.org/2098553002/diff/20001/android_webview/browser...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/20001/android_webview/browser...
android_webview/browser/aw_content_browser_client.cc:442: return new
AwGeolocationServiceOverrides();
On 2016/06/24 13:57:18, Michael van Ouwerkerk wrote:
> For a getter, I would expect the client to own the instance, and instantiate
it
> lazily. Otherwise this method should be called
> "CreateGeolocationServiceOverrides" but I wouldn't recommend going in that
> direction.

The easiest way code-wise would be to change it to
CreateGeolocationServiceOverride() and hold on to
it inside GeolocationProviderImpl (taking into account
your other comment about not holding on to it inside
LocationArbitratorImpl).  Embedders already create 
instances of AccessTokenStore and/or LocationProvider 
and return them via |new Bla|, so they don't expect to
hold on to those objects... So, renamed.

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

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
content/browser/geolocation/geolocation_provider_impl.cc:17: #include
"content/public/browser/browser_thread.h"
On 2016/06/24 13:57:18, Michael van Ouwerkerk wrote:
> nit: delete this duplicated include

Done.

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.h:45: // Takes ownership of
|service_overrides|.
On 2016/06/24 13:57:19, Michael van Ouwerkerk wrote:
> I think that a) taking ownership should be expressed by using unique_ptr, and 
Yes, I was just lazy -- and I thought I'd be getting a 
coarse first review :)  
> b) it seems strange for the arbitrator to own it, though it does not define
it.
> Maybe it should be owned by the creator?

Yeah, I moved its ownership to GeolocationProviderImpl
and used from here via *.

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

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:91:
DISALLOW_COPY_AND_ASSIGN(GeolocationContentBrowserClient);
On 2016/06/24 13:57:19, Michael van Ouwerkerk wrote:
> Why delete this?

Unintended, restated.

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

https://codereview.chromium.org/2098553002/diff/20001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:69: class
MockServiceOverrides : public GeolocationProvider::ServiceOverrides {
On 2016/06/24 13:57:19, Michael van Ouwerkerk wrote:
> This is not really a mock (using gmock), but a fake.

Done.

https://codereview.chromium.org/2098553002/diff/20001/content/content_browser...
File content/content_browser.gypi (right):

https://codereview.chromium.org/2098553002/diff/20001/content/content_browser...
content/content_browser.gypi:192: 'public/browser/geolocation_provider.cc',
On 2016/06/24 13:57:19, Michael van Ouwerkerk wrote:
> Is this file suitable for the public content api?
> https://www.chromium.org/developers/content-module/content-api

Please see my reply to your comment in the file itself.

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

https://codereview.chromium.org/2098553002/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:535: virtual
GeolocationProvider::ServiceOverrides*
On 2016/06/24 13:57:19, Michael van Ouwerkerk wrote:
> This method must be documented, like the others in this file.

Done.

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

https://codereview.chromium.org/2098553002/diff/20001/content/public/browser/...
content/public/browser/geolocation_provider.h:33: class CONTENT_EXPORT
ServiceOverrides {
On 2016/06/24 13:57:19, Michael van Ouwerkerk wrote:
> I'd suggest calling it GeolocationProvider::Delegate

Done.

https://codereview.chromium.org/2098553002/diff/20001/content/public/browser/...
content/public/browser/geolocation_provider.h:37: virtual bool
UseNetworkLocationProviders();
On 2016/06/24 13:57:19, Michael van Ouwerkerk wrote:
> The first rule of the content API [1] is "content/public should contain only
> interfaces, structs and (rarely) static functions" so this should be pure
> virtual. When in doubt, ask the content api owners.
> 
> [1] https://www.chromium.org/developers/content-module/content-api

I didn't know that rule, but currently there are
52 .cc files in content/public/browser, and some
of them (e.g.  download_manager_delegate.cc) has
class implementation. That said, I'm willing to comply
and will send this CL to content/ OWNERS before 
landing, of course, and we'll see where to put this
file then, SG?

https://codereview.chromium.org/2098553002/diff/20001/content/public/browser/...
content/public/browser/geolocation_provider.h:40: virtual AccessTokenStore*
CreateAccessTokenStore();
On 2016/06/24 13:57:19, Michael van Ouwerkerk wrote:
> If you're here anyway, consider whether this should return a unique_ptr for
> ownership clarity. You could leave a TODO for doing this as a future change.

Added a TODO and a bug. Smaller CLs are easier
to review :)

[mcasas, 2016-06-24 21:28:18]

Patchset #3 (id:120001) has been deleted

[mcasas, 2016-06-27 17:43:46]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider() 
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a (weak) 
GeolocationProvider::Delegate* on ctor; this object 
encapsulates the 3 previously mentioned  methods, that are 
(partially) overriden in each Client.

This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

BUG=612334
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: extract ContentBrowserClient methods specific to Geolocation into a
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider() 
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a (weak) 
GeolocationProvider::Delegate* on ctor; this object 
encapsulates the 3 previously mentioned  methods, that are 
(partially) overriden in each Client.

This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

BUG=612334
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-06-27 17:44:00]

ping

[CJ, 2016-06-27 18:42:55]

lethalantidote@chromium.org changed reviewers:
+ lethalantidote@chromium.org

[CJ, 2016-06-27 18:42:56]

https://codereview.chromium.org/2098553002/diff/140001/content/public/browser...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/2098553002/diff/140001/content/public/browser...
content/public/browser/geolocation_provider.h:33: class CONTENT_EXPORT Delegate
{
Would it be possible to give this a more informative name? It is a little
confusing to me, since "GeolocationProvider::Delegate" makes me think it it is
the call back to receive a location. Perhaps something like
"GeolocationProvider::ContentBrowserClientDelegate" or some shortened form of
that?

[mcasas, 2016-06-27 21:52:36]

Patchset #4 (id:160001) has been deleted

[mcasas, 2016-06-28 04:42:20]

Patchset #4 (id:180001) has been deleted

[Michael van Ouwerkerk, 2016-06-28 11:17:08]

lgtm with nits, thanks Miguel!

https://codereview.chromium.org/2098553002/diff/140001/content/public/browser...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/2098553002/diff/140001/content/public/browser...
content/public/browser/geolocation_provider.h:33: class CONTENT_EXPORT Delegate
{
On 2016/06/27 18:42:56, CJ wrote:
> Would it be possible to give this a more informative name? It is a little
> confusing to me, since "GeolocationProvider::Delegate" makes me think it it is
> the call back to receive a location. Perhaps something like
> "GeolocationProvider::ContentBrowserClientDelegate" or some shortened form of
> that? 

I think generally in our codebase a delegate is used to perform actions on your
behalf, while say an observer is used to receive event driven data. So, the name
doesn't seem so wrong, but I agree it can look a bit generic in some places. I'm
not sure how to improve it, so I'll leave the decision to the CL author :-)

https://codereview.chromium.org/2098553002/diff/200001/android_webview/browse...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/200001/android_webview/browse...
android_webview/browser/aw_content_browser_client.cc:164: class
AwGeolocationDelegate : public content::GeolocationProvider::Delegate {
nit: add DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2098553002/diff/200001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/200001/blimp/engine/app/blimp...
blimp/engine/app/blimp_content_browser_client.cc:17: class
BlimpGeolocationDelegate : public content::GeolocationProvider::Delegate {
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2098553002/diff/200001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/200001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:631: class
ChromeGeolocationDelegate
nit: DISALLOW_COPY_AND_ASSIGN

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

https://codereview.chromium.org/2098553002/diff/200001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:81:
LocationProvider* location_provider() const {
nit: blank line above this one

https://codereview.chromium.org/2098553002/diff/200001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:87:
std::unique_ptr<LocationProvider> location_provider_;
nit: consider renaming to system_location_provider_ for clarity.

https://codereview.chromium.org/2098553002/diff/200001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:136: const bool
is_fake_delegate_;
nit: blank line below this one

https://codereview.chromium.org/2098553002/diff/200001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2098553002/diff/200001/content/public/browser...
content/public/browser/content_browser_client.h:534: // Allows the Embedder to
provide a Delegate for Geolocation to override some
nit: s/Embedder/embedder/

https://codereview.chromium.org/2098553002/diff/200001/content/public/browser...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/2098553002/diff/200001/content/public/browser...
content/public/browser/geolocation_provider.h:31: // An Embedder of Geolocation
may override these class' methods to provide
nit: s/Embedder/embedder/

https://codereview.chromium.org/2098553002/diff/200001/content/shell/browser/...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/200001/content/shell/browser/...
content/shell/browser/shell_content_browser_client.cc:122: class
ShellGeolocationDelegate : public content::GeolocationProvider::Delegate {
nit: DISALLOW_COPY_AND_ASSIGN

[mcasas, 2016-06-28 16:21:45]

Patchset #5 (id:220001) has been deleted

[mcasas, 2016-06-28 16:51:26]

https://codereview.chromium.org/2098553002/diff/140001/content/public/browser...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/2098553002/diff/140001/content/public/browser...
content/public/browser/geolocation_provider.h:33: class CONTENT_EXPORT Delegate
{
On 2016/06/28 11:17:07, Michael van Ouwerkerk wrote:
> On 2016/06/27 18:42:56, CJ wrote:
> > Would it be possible to give this a more informative name? It is a little
> > confusing to me, since "GeolocationProvider::Delegate" makes me think it it
is
> > the call back to receive a location. Perhaps something like
> > "GeolocationProvider::ContentBrowserClientDelegate" or some shortened form
of
> > that? 
> 
> I think generally in our codebase a delegate is used to perform actions on
your
> behalf, while say an observer is used to receive event driven data. So, the
name
> doesn't seem so wrong, but I agree it can look a bit generic in some places.
I'm
> not sure how to improve it, so I'll leave the decision to the CL author :-)

I agree with Michael, ::Delegate is a class doing something on
our behalf, and, since it has always a GeolocationProvider::
resolution, it's somehow clear what's all about. 
PS1 called it ServiceOverrides, and I consider calling it
ServiceFactory, or PlatformServices or the like, but
UseNetworkLocationProvider() doesn't look like a 
factory method :) Finally, since the embedder doesn't 
necessarily need to be a ContentBrowserClient, adding it
to the name might be too restrictive. So Delegate seems
to be understandable for Chrome developers. If other
suggestions  come up, let's make a bug and follow up,
since we're moving around all this code.

https://codereview.chromium.org/2098553002/diff/200001/android_webview/browse...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/200001/android_webview/browse...
android_webview/browser/aw_content_browser_client.cc:164: class
AwGeolocationDelegate : public content::GeolocationProvider::Delegate {
On 2016/06/28 11:17:07, Michael van Ouwerkerk wrote:
> nit: add DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2098553002/diff/200001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/200001/blimp/engine/app/blimp...
blimp/engine/app/blimp_content_browser_client.cc:17: class
BlimpGeolocationDelegate : public content::GeolocationProvider::Delegate {
On 2016/06/28 11:17:07, Michael van Ouwerkerk wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2098553002/diff/200001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/200001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:631: class
ChromeGeolocationDelegate
On 2016/06/28 11:17:07, Michael van Ouwerkerk wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

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

https://codereview.chromium.org/2098553002/diff/200001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:81:
LocationProvider* location_provider() const {
On 2016/06/28 11:17:07, Michael van Ouwerkerk wrote:
> nit: blank line above this one

Done.

https://codereview.chromium.org/2098553002/diff/200001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:87:
std::unique_ptr<LocationProvider> location_provider_;
On 2016/06/28 11:17:08, Michael van Ouwerkerk wrote:
> nit: consider renaming to system_location_provider_ for clarity.

Done.

https://codereview.chromium.org/2098553002/diff/200001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:136: const bool
is_fake_delegate_;
On 2016/06/28 11:17:07, Michael van Ouwerkerk wrote:
> nit: blank line below this one

Done.

https://codereview.chromium.org/2098553002/diff/200001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2098553002/diff/200001/content/public/browser...
content/public/browser/content_browser_client.h:534: // Allows the Embedder to
provide a Delegate for Geolocation to override some
On 2016/06/28 11:17:08, Michael van Ouwerkerk wrote:
> nit: s/Embedder/embedder/

Done.

https://codereview.chromium.org/2098553002/diff/200001/content/public/browser...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/2098553002/diff/200001/content/public/browser...
content/public/browser/geolocation_provider.h:31: // An Embedder of Geolocation
may override these class' methods to provide
On 2016/06/28 11:17:08, Michael van Ouwerkerk wrote:
> nit: s/Embedder/embedder/

Done.

https://codereview.chromium.org/2098553002/diff/200001/content/shell/browser/...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/2098553002/diff/200001/content/shell/browser/...
content/shell/browser/shell_content_browser_client.cc:122: class
ShellGeolocationDelegate : public content::GeolocationProvider::Delegate {
On 2016/06/28 11:17:08, Michael van Ouwerkerk wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

[CJ, 2016-06-28 17:00:35]

lgtm

https://codereview.chromium.org/2098553002/diff/140001/content/public/browser...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/2098553002/diff/140001/content/public/browser...
content/public/browser/geolocation_provider.h:33: class CONTENT_EXPORT Delegate
{
On 2016/06/28 16:51:25, mcasas wrote:
> On 2016/06/28 11:17:07, Michael van Ouwerkerk wrote:
> > On 2016/06/27 18:42:56, CJ wrote:
> > > Would it be possible to give this a more informative name? It is a little
> > > confusing to me, since "GeolocationProvider::Delegate" makes me think it
it
> is
> > > the call back to receive a location. Perhaps something like
> > > "GeolocationProvider::ContentBrowserClientDelegate" or some shortened form
> of
> > > that? 
> > 
> > I think generally in our codebase a delegate is used to perform actions on
> your
> > behalf, while say an observer is used to receive event driven data. So, the
> name
> > doesn't seem so wrong, but I agree it can look a bit generic in some places.
> I'm
> > not sure how to improve it, so I'll leave the decision to the CL author :-)
> 
> I agree with Michael, ::Delegate is a class doing something on
> our behalf, and, since it has always a GeolocationProvider::
> resolution, it's somehow clear what's all about. 
> PS1 called it ServiceOverrides, and I consider calling it
> ServiceFactory, or PlatformServices or the like, but
> UseNetworkLocationProvider() doesn't look like a 
> factory method :) Finally, since the embedder doesn't 
> necessarily need to be a ContentBrowserClient, adding it
> to the name might be too restrictive. So Delegate seems
> to be understandable for Chrome developers. If other
> suggestions  come up, let's make a bug and follow up,
> since we're moving around all this code.

Acknowledged.

[CJ, 2016-06-28 17:00:40]

lgtm

lgtm

[CJ, 2016-06-28 17:00:44]

lgtm

[mcasas, 2016-06-28 17:02:05]

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

[mcasas, 2016-06-28 17:02:06]

jam@ RS LGTM please.

[jam, 2016-06-28 18:08:21]

lgtm

https://codereview.chromium.org/2098553002/diff/240001/content/public/browser...
File content/public/browser/geolocation_provider.h (right):

https://codereview.chromium.org/2098553002/diff/240001/content/public/browser...
content/public/browser/geolocation_provider.h:33: class CONTENT_EXPORT Delegate
{
nit: I realize this is moving to device/. I would encourage you to make this to
be a separate header (i.e. geolocation_delegate.) for two reasons:
1) it can be overridden and forward declared without including the this header
2) once you move this to device/geolocation, there's no more need for an
interface/impl split of GeolocationProvider since it's out of content. so more
stuff will move to this parent class.

[mcasas, 2016-06-28 18:25:51]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-06-28 18:25:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, jam@chromium.org, lethalantidote@chromium.org
Link to the patchset: https://codereview.chromium.org/2098553002/#ps260001
(title: "Added a TODO")

[commit-bot: I haz the power, 2016-06-28 18:26:46]

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-06-28 20:24:56]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into a
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider() 
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a (weak) 
GeolocationProvider::Delegate* on ctor; this object 
encapsulates the 3 previously mentioned  methods, that are 
(partially) overriden in each Client.

This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

BUG=612334
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: extract ContentBrowserClient methods specific to Geolocation into a
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider() 
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a (weak) 
GeolocationProvider::Delegate* on ctor; this object 
encapsulates the 3 previously mentioned  methods, that are 
(partially) overriden in each Client.

This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

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

[commit-bot: I haz the power, 2016-06-28 20:24:57]

Committed patchset #6 (id:260001)

[commit-bot: I haz the power, 2016-06-28 20:27:38]

Description was changed from

==========
Geolocation: extract ContentBrowserClient methods specific to Geolocation into a
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider() 
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a (weak) 
GeolocationProvider::Delegate* on ctor; this object 
encapsulates the 3 previously mentioned  methods, that are 
(partially) overriden in each Client.

This CL also changes all those implementations and 
the unittests (which is quite simplified) to follow the 
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

BUG=612334
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: extract ContentBrowserClient methods specific to Geolocation into a
class

ToT ContentBrowserClient defines three methods:
+ bool UseNetworkLocationProvider()
  (overriden in {blimp,content}_browser_client)
+ LocationProvider* OverrideSystemLocationProvider()
 (overriden in (a) downstream project(s) and
   blimp_content_browser_client.)
+ AccessTokenStore* CreateAccessTokenStore()
 (overriden in {aw,cast,chrome,content,shell}_browser_client.)

which are used from LocationArbitratorImpl, after
accessing the embedder via GetContentClient()->browser().

This CL changes LocationArbitratorImpl to get a (weak)
GeolocationProvider::Delegate* on ctor; this object
encapsulates the 3 previously mentioned  methods, that are
(partially) overriden in each Client.

This CL also changes all those implementations and
the unittests (which is quite simplified) to follow the
object-getter pattern.

This CL is a preliminary step to move Geolocation to //device/
and is a refactoring.

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

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

[commit-bot: I haz the power, 2016-06-28 20:27:40]

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