Hooks together Geolocation Feature in the Client and Engine.

Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate with the LocationArbitrator, and makes the necessary changes to permissions.

Additional Changes:
* Renames LocationArbitratorImpl -> LocationArbitrator

BUG=614486, 641071
Committed: https://crrev.com/8e0b2a7306378c41550a589dfbb8cefa0ebabca0
Cr-Commit-Position: refs/heads/master@{#416190}

[CJ, 2016-08-17 18:29:45]

TODO: Not sure about the semantics of SetUpdateCallback for LocationArbitrator.
Seems misleading and not useful. 
TODO: Move client feature geolocation into own folder.

[CJ, 2016-08-17 18:43:30]

Description was changed from

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming.

BUG=614486
==========

to

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming. This to be handled in another CL: Delegate
implementation. Update Scheduling. 

BUG=614486
==========

[CJ, 2016-08-17 22:17:50]

lethalantidote@chromium.org changed reviewers:
+ kmarshall@chromium.org, nyquist@chromium.org, timvolodine@chromium.org,
wez@chromium.org

[CJ, 2016-08-17 22:21:13]

Plz take a look.

[CJ, 2016-08-17 22:21:14]

Plz take a look.

[CJ, 2016-08-17 23:02:49]

Description was changed from

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming. This to be handled in another CL: Delegate
implementation. Update Scheduling. 

BUG=614486
==========

to

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming. This to be handled in another CL: Delegate
implementation. Update Scheduling. 

This CL had to have a CL currently in-flight merged into it to make things work
(well, actually two different CL's changes needed to exist for this CL to work,
but the first I set upstream to.) Because of such there are some extra files.
Please in particular look at:

blimp/client/BUILD.gn
blimp/client/feature/geolocation/blimp_location_provider.cc
blimp/client/feature/geolocation/blimp_location_provider.h
blimp/client/session/blimp_client_session.cc
blimp/client/session/blimp_client_session.h
device/geolocation/location_arbitrator_impl.h

To be noted, client feature geolocation related files were given their own
directory in this CL.




BUG=614486
==========

[CJ, 2016-08-18 21:11:42]

Description was changed from

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming. This to be handled in another CL: Delegate
implementation. Update Scheduling. 

This CL had to have a CL currently in-flight merged into it to make things work
(well, actually two different CL's changes needed to exist for this CL to work,
but the first I set upstream to.) Because of such there are some extra files.
Please in particular look at:

blimp/client/BUILD.gn
blimp/client/feature/geolocation/blimp_location_provider.cc
blimp/client/feature/geolocation/blimp_location_provider.h
blimp/client/session/blimp_client_session.cc
blimp/client/session/blimp_client_session.h
device/geolocation/location_arbitrator_impl.h

To be noted, client feature geolocation related files were given their own
directory in this CL.




BUG=614486
==========

to

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming. This to be handled in another CL: Delegate
implementation. Update Scheduling. 


BUG=614486
==========

[CJ, 2016-08-18 21:13:06]

On 2016/08/17 22:21:14, CJ wrote:
> Plz take a look.

Hi everyone. Things have been reorganized so that this CL is now directly
dependent on the Geolocation Feature CL
https://codereview.chromium.org/2161223003/ . Hopefully things are now clearer!

[nyquist, 2016-08-18 22:17:17]

https://codereview.chromium.org/2249283003/diff/40001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/2249283003/diff/40001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.h:107:
std::unique_ptr<GeolocationFeature> geolocation_feature_;
Drive-by: I think you want this for BlimpClientContextImpl as well. In fact,
that's where we'll use it going forward, as the plan is to remove this class at
some point.

[CJ, 2016-08-18 23:00:19]

On 2016/08/18 22:17:17, nyquist wrote:
>
https://codereview.chromium.org/2249283003/diff/40001/blimp/client/session/bl...
> File blimp/client/session/blimp_client_session.h (right):
> 
>
https://codereview.chromium.org/2249283003/diff/40001/blimp/client/session/bl...
> blimp/client/session/blimp_client_session.h:107:
> std::unique_ptr<GeolocationFeature> geolocation_feature_;
> Drive-by: I think you want this for BlimpClientContextImpl as well. In fact,
> that's where we'll use it going forward, as the plan is to remove this class
at
> some point.

Done. Will add tests after initial review.

[CJ, 2016-08-18 23:38:40]

Description was changed from

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming. This to be handled in another CL: Delegate
implementation. Update Scheduling. 


BUG=614486
==========

to

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming. To be handled in another CL: Delegate
implementation. Update Scheduling. 


BUG=614486
==========

[Kevin M, 2016-08-19 17:51:37]

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:54: geolocation_feature_(new
GeolocationFeature(
How come this is stored in both the ClientContext and the ClientSession?

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:132: // Register features'
message senders and receivers.
Move this comment to the .h file

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
File blimp/client/core/geolocation/blimp_location_provider.h (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
blimp/client/core/geolocation/blimp_location_provider.h:18: class
BlimpLocationProvider : public device::LocationProvider {
Perhaps we could provide it with a more descriptive name, e.g.
RateLimitingLocationProviderProxy? Something like that... the "Blimp" class
prefix is superfluous for several reasons (namespace, dir structure)

* "handles frequency" isn't descriptive, either.
* Add TODO indicating that the scheduling aspect still needs to be implemented.

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
blimp/client/core/geolocation/blimp_location_provider.h:32: void
OnLocationUpdate(const device::Geoposition& new_position);
Comment this method

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:42: geolocation_feature_(
This is also contained in the ClientContext? O_o

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
File device/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
device/geolocation/location_arbitrator_impl.h:37: class
DEVICE_GEOLOCATION_EXPORT LocationArbitratorImpl
This should probably be LocationArbitrator, not LocationArbitratorImpl, since
the LocationArbitrator interface is no more.

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
device/geolocation/location_arbitrator_impl.h:55: // SetUpdateCallback in this
case will indicate what callback to pass to each
I don't think this is the case? Shouldn't the arbitrator be passing its own
internal callback to |providers_|, aggregating the result into a single value,
and then returning that value via |callback|?

[CJ, 2016-08-19 20:44:01]

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:54: geolocation_feature_(new
GeolocationFeature(
On 2016/08/19 17:51:37, Kevin M wrote:
> How come this is stored in both the ClientContext and the ClientSession?

nyquist's suggestion. Based on what was discussed offline, it will be removed
for now.

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:132: // Register features'
message senders and receivers.
On 2016/08/19 17:51:37, Kevin M wrote:
> Move this comment to the .h file

Done.

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
File blimp/client/core/geolocation/blimp_location_provider.h (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
blimp/client/core/geolocation/blimp_location_provider.h:18: class
BlimpLocationProvider : public device::LocationProvider {
On 2016/08/19 17:51:37, Kevin M wrote:
> Perhaps we could provide it with a more descriptive name, e.g.
> RateLimitingLocationProviderProxy? Something like that... the "Blimp" class
> prefix is superfluous for several reasons (namespace, dir structure)
> 
> * "handles frequency" isn't descriptive, either.
> * Add TODO indicating that the scheduling aspect still needs to be
implemented.

Done.

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
blimp/client/core/geolocation/blimp_location_provider.h:32: void
OnLocationUpdate(const device::Geoposition& new_position);
On 2016/08/19 17:51:37, Kevin M wrote:
> Comment this method

Done.

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:42: geolocation_feature_(
On 2016/08/19 17:51:37, Kevin M wrote:
> This is also contained in the ClientContext? O_o

Acknowledged.

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
File device/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
device/geolocation/location_arbitrator_impl.h:37: class
DEVICE_GEOLOCATION_EXPORT LocationArbitratorImpl
On 2016/08/19 17:51:37, Kevin M wrote:
> This should probably be LocationArbitrator, not LocationArbitratorImpl, since
> the LocationArbitrator interface is no more.

Done.

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
device/geolocation/location_arbitrator_impl.h:55: // SetUpdateCallback in this
case will indicate what callback to pass to each
On 2016/08/19 17:51:37, Kevin M wrote:
> I don't think this is the case? Shouldn't the arbitrator be passing its own
> internal callback to |providers_|, aggregating the result into a single value,
> and then returning that value via |callback|?

It is doing that. However, by making LocationArbitrator a LocationProvider, it
gained this function. It's kinda of a case of okay, now that we have this
function what to do with it. Maybe it would best if it was left unimplemented
for LocationArbitrator.

[Wez, 2016-08-19 23:12:11]

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:69: // registered.
nit: While you're here can you correct this typo, plz? :P

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
File blimp/client/core/geolocation/blimp_location_provider.cc (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
blimp/client/core/geolocation/blimp_location_provider.cc:27: new
BlimpGeolocationDelegate())) {}
Why not pass in a default GeolocationDelegate here?

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
File blimp/client/core/geolocation/blimp_location_provider.h (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
blimp/client/core/geolocation/blimp_location_provider.h:18: class
BlimpLocationProvider : public device::LocationProvider {
This class is a pure LocationProvider proxy - it doesn't implement any behaviour
- so why not just use the LocationArbitratorImpl directly for now?

Even when we do make this a rate-limiting proxy, it doesn't seem that that will
be particularly Blimp-specific, so it may make more sense to have a
RateLimitedLocationProvider in device/gelocation/

WDYT?

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/fake...
File device/geolocation/fake_location_provider.h (right):

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/fake...
device/geolocation/fake_location_provider.h:26: bool IsProviderStarted() const;
nit: Why is this moved in this CL?

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
File device/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
device/geolocation/location_arbitrator_impl.h:56: // LocationProvider contained
in |providers_|.
I'm not sure what this comment means - are you saying that |callback| gets pass
as-is to every provider, or do we still wrap it in some way?

Fundamentally though that's an implementation detail - it's not important to the
caller what happens internally so long as they get the behaviour they expect.

[CJ, 2016-08-22 20:28:08]

Ignored rate_limiting_location_provider_proxy_unittest.cc. Still in progress.

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/blimp...
blimp/client/core/blimp_client_context_impl.cc:69: // registered.
On 2016/08/19 23:12:10, Wez wrote:
> nit: While you're here can you correct this typo, plz? :P

Done.

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
File blimp/client/core/geolocation/blimp_location_provider.cc (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
blimp/client/core/geolocation/blimp_location_provider.cc:27: new
BlimpGeolocationDelegate())) {}
On 2016/08/19 23:12:10, Wez wrote:
> Why not pass in a default GeolocationDelegate here?

I thought there would be some issue with there not being a AccessTokenStore on
that one, but I looked at it again, and I guess there really isn't much of a
reason to worry about that, since for android it doesnt seem to be used.

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
File blimp/client/core/geolocation/blimp_location_provider.h (right):

https://codereview.chromium.org/2249283003/diff/60001/blimp/client/core/geolo...
blimp/client/core/geolocation/blimp_location_provider.h:18: class
BlimpLocationProvider : public device::LocationProvider {
On 2016/08/19 23:12:10, Wez wrote:
> This class is a pure LocationProvider proxy - it doesn't implement any
behaviour
> - so why not just use the LocationArbitratorImpl directly for now?
> 
> Even when we do make this a rate-limiting proxy, it doesn't seem that that
will
> be particularly Blimp-specific, so it may make more sense to have a
> RateLimitedLocationProvider in device/gelocation/
> 
> WDYT?

I agree with moving it over to device/geolocation.

I'm trying to make CL's a bit smaller, so that's why it doesn't have much going
for it yet. Basic tests can be made for the class, and will make the next
checkin hopefully smoother.

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/fake...
File device/geolocation/fake_location_provider.h (right):

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/fake...
device/geolocation/fake_location_provider.h:26: bool IsProviderStarted() const;
On 2016/08/19 23:12:10, Wez wrote:
> nit: Why is this moved in this CL?

Not sure. It's not used anymore I think though, so it's gone now.

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
File device/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2249283003/diff/60001/device/geolocation/loca...
device/geolocation/location_arbitrator_impl.h:56: // LocationProvider contained
in |providers_|.
On 2016/08/19 23:12:10, Wez wrote:
> I'm not sure what this comment means - are you saying that |callback| gets
pass
> as-is to every provider, or do we still wrap it in some way?
> 
> Fundamentally though that's an implementation detail - it's not important to
the
> caller what happens internally so long as they get the behaviour they expect.

The comment on this function no longer applies with the changes that occurred in
GeolocationFeature CL. The behavior is now as expected. Before it was a little
funky, but it got fixed.

[Kevin M, 2016-08-22 22:09:35]

lgtm

https://codereview.chromium.org/2249283003/diff/100001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2249283003/diff/100001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:63: // Register features' message
senders and receivers.
Function no longer exists in the .cc; remove this line

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/loc...
File device/geolocation/location_arbitrator.h (right):

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/loc...
device/geolocation/location_arbitrator.h:44: explicit
LocationArbitrator(GeolocationDelegate* delegate);
Take a std::unique_ptr here

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy.cc (right):

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.cc:14: :
location_provider_(new LocationArbitrator(new GeolocationDelegate())) {
nit: remove () from new GeolocationDelegate()

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy_unittest.cc
(right):

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy_unittest.cc:5: #include
"device/geolocation/rate_limiting_location_provider_proxy.h"
#include <memory>

[CJ, 2016-08-23 18:07:13]

https://codereview.chromium.org/2249283003/diff/100001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2249283003/diff/100001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:63: // Register features' message
senders and receivers.
On 2016/08/22 22:09:34, Kevin M wrote:
> Function no longer exists in the .cc; remove this line

Done.

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/loc...
File device/geolocation/location_arbitrator.h (right):

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/loc...
device/geolocation/location_arbitrator.h:44: explicit
LocationArbitrator(GeolocationDelegate* delegate);
On 2016/08/22 22:09:34, Kevin M wrote:
> Take a std::unique_ptr here

I don't think we want to do that. GeolocationProviderImpl owns the object and
passes the delegate more as an interface.

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy.cc (right):

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.cc:14: :
location_provider_(new LocationArbitrator(new GeolocationDelegate())) {
On 2016/08/22 22:09:34, Kevin M wrote:
> nit: remove () from new GeolocationDelegate()

Done.

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy_unittest.cc
(right):

https://codereview.chromium.org/2249283003/diff/100001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy_unittest.cc:5: #include
"device/geolocation/rate_limiting_location_provider_proxy.h"
On 2016/08/22 22:09:34, Kevin M wrote:
> #include <memory>

Done.

[Wez, 2016-08-24 23:00:11]

Looks like the CL description is out-of-date?

[Wez, 2016-08-24 23:07:19]

https://codereview.chromium.org/2249283003/diff/140001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2249283003/diff/140001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:79: // register it.
Not clear what this means - why not do that now? Or, if it can't happen now, why
do we need a TODO rather than just a bug filed for that work?

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy.cc (right):

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.cc:14: :
location_provider_(new LocationArbitrator(new GeolocationDelegate)) {
This class is just supposed to be a proxy - i.e. a thing that sits in-between
the caller and the underlying location provider. If you directly instantiate the
LocationArbitrator here then this class is really a
RateLimitingLocationArbitrator, isn't it, not a proxy?

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy.h (right):

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:17: //
TODO(lethalantidote): Schedule aspect still to be implemented. Dummy for now.
What is the point of having a "dummy" that doesn't implement the actual
functionality?

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:25:
std::unique_ptr<LocationProvider> location_provider);
If this is only for test then how does a caller provide the underlying
LocationProvider they want to use in production code?

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:36: // Limits the
rate in which location updates from |location_provider_| will
nit: s/in/at

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:42:
LocationProvider::LocationProviderUpdateCallback feature_update_callback_;
What is "feature" in this context?

[CJ, 2016-08-25 18:29:48]

Description was changed from

==========
Lays down foundation for Client-side Blimp Location Provider.

This CL contains a dummy implementation of Blimp Location Provider, which
passes along messages to and from the LocationArbitratorImpl to the
GeolocationFeature. This CL also registers the GeolocationFeature with the
BlimpClientSession.

Basic unittests forthcoming. To be handled in another CL: Delegate
implementation. Update Scheduling. 


BUG=614486
==========

to

==========
Lays down foundation for RateLimitingLocationProviderProxy.

This CL contains a dummy implementation of RateLimitingLocationProviderProxy. It
is intended that the proxy will limit updates coming from the
LocationArbitrator, as to decrease network congestion in the Blimp use-case.
This CL also registers the GeolocationFeature with the
BlimpClientSession.

To be handled in another CL:
Update Scheduling. 


BUG=614486
==========

[CJ, 2016-08-25 19:08:22]

https://codereview.chromium.org/2249283003/diff/140001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2249283003/diff/140001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:79: // register it.
On 2016/08/24 23:07:18, Wez wrote:
> Not clear what this means - why not do that now? Or, if it can't happen now,
why
> do we need a TODO rather than just a bug filed for that work?

I can file a bug, sure. Kmarshall suggested that we wait until after this file
stabilizes more, since it seems that we are in transition from using
blimp_client_session to using blimp_client_context_impl.

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy.cc (right):

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.cc:14: :
location_provider_(new LocationArbitrator(new GeolocationDelegate)) {
On 2016/08/24 23:07:18, Wez wrote:
> This class is just supposed to be a proxy - i.e. a thing that sits in-between
> the caller and the underlying location provider. If you directly instantiate
the
> LocationArbitrator here then this class is really a
> RateLimitingLocationArbitrator, isn't it, not a proxy?

Changed to take in a LocationProvider. Should have done that in the first place.

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy.h (right):

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:17: //
TODO(lethalantidote): Schedule aspect still to be implemented. Dummy for now.
On 2016/08/24 23:07:18, Wez wrote:
> What is the point of having a "dummy" that doesn't implement the actual
> functionality?

I can make a baseline to see if the techniques I proposed actually work. I can
also see if functionality is the same between blimp and normal geolocation
without rate limiting before I apply things. Also smaller CL.

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:25:
std::unique_ptr<LocationProvider> location_provider);
On 2016/08/24 23:07:18, Wez wrote:
> If this is only for test then how does a caller provide the underlying
> LocationProvider they want to use in production code?

Good point, its gone, constructor now handles it.

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:36: // Limits the
rate in which location updates from |location_provider_| will
On 2016/08/24 23:07:18, Wez wrote:
> nit: s/in/at

Done.

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:42:
LocationProvider::LocationProviderUpdateCallback feature_update_callback_;
On 2016/08/24 23:07:18, Wez wrote:
> What is "feature" in this context?

Good point.

[CJ, 2016-08-26 20:31:30]

On 2016/08/25 19:08:22, CJ wrote:
>
https://codereview.chromium.org/2249283003/diff/140001/blimp/client/core/blim...
> File blimp/client/core/blimp_client_context_impl.h (right):
> 
>
https://codereview.chromium.org/2249283003/diff/140001/blimp/client/core/blim...
> blimp/client/core/blimp_client_context_impl.h:79: // register it.
> On 2016/08/24 23:07:18, Wez wrote:
> > Not clear what this means - why not do that now? Or, if it can't happen now,
> why
> > do we need a TODO rather than just a bug filed for that work?
> 
> I can file a bug, sure. Kmarshall suggested that we wait until after this file
> stabilizes more, since it seems that we are in transition from using
> blimp_client_session to using blimp_client_context_impl.
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> File device/geolocation/rate_limiting_location_provider_proxy.cc (right):
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> device/geolocation/rate_limiting_location_provider_proxy.cc:14: :
> location_provider_(new LocationArbitrator(new GeolocationDelegate)) {
> On 2016/08/24 23:07:18, Wez wrote:
> > This class is just supposed to be a proxy - i.e. a thing that sits
in-between
> > the caller and the underlying location provider. If you directly instantiate
> the
> > LocationArbitrator here then this class is really a
> > RateLimitingLocationArbitrator, isn't it, not a proxy?
> 
> Changed to take in a LocationProvider. Should have done that in the first
place.
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> File device/geolocation/rate_limiting_location_provider_proxy.h (right):
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> device/geolocation/rate_limiting_location_provider_proxy.h:17: //
> TODO(lethalantidote): Schedule aspect still to be implemented. Dummy for now.
> On 2016/08/24 23:07:18, Wez wrote:
> > What is the point of having a "dummy" that doesn't implement the actual
> > functionality?
> 
> I can make a baseline to see if the techniques I proposed actually work. I can
> also see if functionality is the same between blimp and normal geolocation
> without rate limiting before I apply things. Also smaller CL.
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> device/geolocation/rate_limiting_location_provider_proxy.h:25:
> std::unique_ptr<LocationProvider> location_provider);
> On 2016/08/24 23:07:18, Wez wrote:
> > If this is only for test then how does a caller provide the underlying
> > LocationProvider they want to use in production code?
> 
> Good point, its gone, constructor now handles it.
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> device/geolocation/rate_limiting_location_provider_proxy.h:36: // Limits the
> rate in which location updates from |location_provider_| will
> On 2016/08/24 23:07:18, Wez wrote:
> > nit: s/in/at
> 
> Done.
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> device/geolocation/rate_limiting_location_provider_proxy.h:42:
> LocationProvider::LocationProviderUpdateCallback feature_update_callback_;
> On 2016/08/24 23:07:18, Wez wrote:
> > What is "feature" in this context?
> 
> Good point.

Ping nyquist, timvolodine

[Wez, 2016-08-30 00:03:58]

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy.h (right):

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:17: //
TODO(lethalantidote): Schedule aspect still to be implemented. Dummy for now.
On 2016/08/25 19:08:22, CJ wrote:
> On 2016/08/24 23:07:18, Wez wrote:
> > What is the point of having a "dummy" that doesn't implement the actual
> > functionality?
> 
> I can make a baseline to see if the techniques I proposed actually work. I can
> also see if functionality is the same between blimp and normal geolocation
> without rate limiting before I apply things. Also smaller CL. 

Right, but you don't need the RateLimitingLocationProviderProxy for a baseline -
you can just pass the LocationArbitrator directly to the GeolocationProviderImpl
to use, rather than passing the rate-limiter.

It would be reasonable to rename this to LocationProviderProxy and keep it to
use as a base-class for RateLimitingLocationProvider, but that could still
happen in a separate CL - this do-nothing proxy doesn't seem to be relevant to
hooking the GeolocationFeature up in the client.

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.cc:7: #include <string>
nit: No need for this here, since the .h already includes it.

(If some other random .h included it then we'd want to include string
explicitly, still, but since it's this .cc file's corresponding header, we don't
need to duplicate the include here)

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:47: new
device::GeolocationDelegate)))),
You're allocating a default GelocationDelegate here and passing it to the
LocationArbitrator, but the LocationArbitrator doesn't actually take ownership -
I think you want to add a geolocation_delegate_ member to this class, to hold
the default Delegate.

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:138: geolocation_feature_.get()));
nit: Can we be consistent in the placement of this relative to all the other
features - e.g. when we declare the member varliable we put it immediately
before tab_control_feature_.

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:16: #include
"blimp/client/core/geolocation/geolocation_feature.h"
No need for this include here - just forward-declare like we do for the other
features, below.

https://codereview.chromium.org/2249283003/diff/180001/device/geolocation/net...
File device/geolocation/network_location_provider.h (right):

https://codereview.chromium.org/2249283003/diff/180001/device/geolocation/net...
device/geolocation/network_location_provider.h:81: void RequestRefresh();
We are we adding this to the header but not to the .cc?

[CJ, 2016-08-31 23:22:17]

Description was changed from

==========
Lays down foundation for RateLimitingLocationProviderProxy.

This CL contains a dummy implementation of RateLimitingLocationProviderProxy. It
is intended that the proxy will limit updates coming from the
LocationArbitrator, as to decrease network congestion in the Blimp use-case.
This CL also registers the GeolocationFeature with the
BlimpClientSession.

To be handled in another CL:
Update Scheduling. 


BUG=614486
==========

to

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

BUG=614486
==========

[CJ, 2016-08-31 23:22:17]

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

[CJ, 2016-08-31 23:22:57]

+mcasas for device/geolocation related items.

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
File device/geolocation/rate_limiting_location_provider_proxy.h (right):

https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
device/geolocation/rate_limiting_location_provider_proxy.h:17: //
TODO(lethalantidote): Schedule aspect still to be implemented. Dummy for now.
On 2016/08/30 00:03:58, Wez wrote:
> On 2016/08/25 19:08:22, CJ wrote:
> > On 2016/08/24 23:07:18, Wez wrote:
> > > What is the point of having a "dummy" that doesn't implement the actual
> > > functionality?
> > 
> > I can make a baseline to see if the techniques I proposed actually work. I
can
> > also see if functionality is the same between blimp and normal geolocation
> > without rate limiting before I apply things. Also smaller CL. 
> 
> Right, but you don't need the RateLimitingLocationProviderProxy for a baseline
-
> you can just pass the LocationArbitrator directly to the
GeolocationProviderImpl
> to use, rather than passing the rate-limiter.
> 
> It would be reasonable to rename this to LocationProviderProxy and keep it to
> use as a base-class for RateLimitingLocationProvider, but that could still
> happen in a separate CL - this do-nothing proxy doesn't seem to be relevant to
> hooking the GeolocationFeature up in the client.

Done.

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.cc:7: #include <string>
On 2016/08/30 00:03:58, Wez wrote:
> nit: No need for this here, since the .h already includes it.
> 
> (If some other random .h included it then we'd want to include string
> explicitly, still, but since it's this .cc file's corresponding header, we
don't
> need to duplicate the include here)

Done.

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:47: new
device::GeolocationDelegate)))),
On 2016/08/30 00:03:58, Wez wrote:
> You're allocating a default GelocationDelegate here and passing it to the
> LocationArbitrator, but the LocationArbitrator doesn't actually take ownership
-
> I think you want to add a geolocation_delegate_ member to this class, to hold
> the default Delegate.

Done.

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:138: geolocation_feature_.get()));
On 2016/08/30 00:03:58, Wez wrote:
> nit: Can we be consistent in the placement of this relative to all the other
> features - e.g. when we declare the member varliable we put it immediately
> before tab_control_feature_.

Done.

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:16: #include
"blimp/client/core/geolocation/geolocation_feature.h"
On 2016/08/30 00:03:58, Wez wrote:
> No need for this include here - just forward-declare like we do for the other
> features, below.

Done.

https://codereview.chromium.org/2249283003/diff/180001/device/geolocation/net...
File device/geolocation/network_location_provider.h (right):

https://codereview.chromium.org/2249283003/diff/180001/device/geolocation/net...
device/geolocation/network_location_provider.h:81: void RequestRefresh();
On 2016/08/30 00:03:58, Wez wrote:
> We are we adding this to the header but not to the .cc?

Mistake. Gone.

[CJ, 2016-08-31 23:23:07]

The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 23:23:56]

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

[CJ, 2016-08-31 23:27:03]

Description was changed from

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

BUG=614486
==========

to

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

Additional Changes:
* Renames LocationArbitratorImpl -> LocationArbitrator

BUG=614486
==========

[commit-bot: I haz the power, 2016-08-31 23:27:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 23:27:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[nyquist, 2016-08-31 23:29:11]

basically //blimp/client/session is going away very, very soon, so I think you'd
want to move thing over.
Also, I think there might be a few changes that you want to rebase on top of /
merge in in BlimpClientSession.

https://codereview.chromium.org/2249283003/diff/220001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/2249283003/diff/220001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:108:
std::unique_ptr<device::GeolocationDelegate> geolocation_delegate_;
This class is going away veru shortly. Could you please add the feature and the
feature registration to BlimpClientContextImpl as well as here?

[mcasas, 2016-08-31 23:29:45]

On 2016/08/31 23:22:57, CJ wrote:
> +mcasas for device/geolocation related items.
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> File device/geolocation/rate_limiting_location_provider_proxy.h (right):
> 
>
https://codereview.chromium.org/2249283003/diff/140001/device/geolocation/rat...
> device/geolocation/rate_limiting_location_provider_proxy.h:17: //
> TODO(lethalantidote): Schedule aspect still to be implemented. Dummy for now.
> On 2016/08/30 00:03:58, Wez wrote:
> > On 2016/08/25 19:08:22, CJ wrote:
> > > On 2016/08/24 23:07:18, Wez wrote:
> > > > What is the point of having a "dummy" that doesn't implement the actual
> > > > functionality?
> > > 
> > > I can make a baseline to see if the techniques I proposed actually work. I
> can
> > > also see if functionality is the same between blimp and normal geolocation
> > > without rate limiting before I apply things. Also smaller CL. 
> > 
> > Right, but you don't need the RateLimitingLocationProviderProxy for a
baseline
> -
> > you can just pass the LocationArbitrator directly to the
> GeolocationProviderImpl
> > to use, rather than passing the rate-limiter.
> > 
> > It would be reasonable to rename this to LocationProviderProxy and keep it
to
> > use as a base-class for RateLimitingLocationProvider, but that could still
> > happen in a separate CL - this do-nothing proxy doesn't seem to be relevant
to
> > hooking the GeolocationFeature up in the client.
> 
> Done.
> 
>
https://codereview.chromium.org/2249283003/diff/180001/blimp/client/core/blim...
> File blimp/client/core/blimp_client_context_impl.cc (right):
> 
>
https://codereview.chromium.org/2249283003/diff/180001/blimp/client/core/blim...
> blimp/client/core/blimp_client_context_impl.cc:7: #include <string>
> On 2016/08/30 00:03:58, Wez wrote:
> > nit: No need for this here, since the .h already includes it.
> > 
> > (If some other random .h included it then we'd want to include string
> > explicitly, still, but since it's this .cc file's corresponding header, we
> don't
> > need to duplicate the include here)
> 
> Done.
> 
>
https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
> File blimp/client/session/blimp_client_session.cc (right):
> 
>
https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
> blimp/client/session/blimp_client_session.cc:47: new
> device::GeolocationDelegate)))),
> On 2016/08/30 00:03:58, Wez wrote:
> > You're allocating a default GelocationDelegate here and passing it to the
> > LocationArbitrator, but the LocationArbitrator doesn't actually take
ownership
> -
> > I think you want to add a geolocation_delegate_ member to this class, to
hold
> > the default Delegate.
> 
> Done.
> 
>
https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
> blimp/client/session/blimp_client_session.cc:138:
geolocation_feature_.get()));
> On 2016/08/30 00:03:58, Wez wrote:
> > nit: Can we be consistent in the placement of this relative to all the other
> > features - e.g. when we declare the member varliable we put it immediately
> > before tab_control_feature_.
> 
> Done.
> 
>
https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
> File blimp/client/session/blimp_client_session.h (right):
> 
>
https://codereview.chromium.org/2249283003/diff/180001/blimp/client/session/b...
> blimp/client/session/blimp_client_session.h:16: #include
> "blimp/client/core/geolocation/geolocation_feature.h"
> On 2016/08/30 00:03:58, Wez wrote:
> > No need for this include here - just forward-declare like we do for the
other
> > features, below.
> 
> Done.
> 
>
https://codereview.chromium.org/2249283003/diff/180001/device/geolocation/net...
> File device/geolocation/network_location_provider.h (right):
> 
>
https://codereview.chromium.org/2249283003/diff/180001/device/geolocation/net...
> device/geolocation/network_location_provider.h:81: void RequestRefresh();
> On 2016/08/30 00:03:58, Wez wrote:
> > We are we adding this to the header but not to the .cc?
> 
> Mistake. Gone.

Insofar as the geolocation/ changes are essentially
s/LocationArbitratorImpl/LocationArbitrator/, lgtm those.

[CJ, 2016-08-31 23:49:37]

https://codereview.chromium.org/2249283003/diff/220001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/2249283003/diff/220001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:108:
std::unique_ptr<device::GeolocationDelegate> geolocation_delegate_;
On 2016/08/31 23:29:11, nyquist wrote:
> This class is going away veru shortly. Could you please add the feature and
the
> feature registration to BlimpClientContextImpl as well as here?

Done.

[CJ, 2016-08-31 23:49:45]

The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 23:50:13]

Dry run: 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-08-31 23:54:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 23:54:52]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[CJ, 2016-09-01 00:08:34]

The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 00:09:28]

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

[CJ, 2016-09-01 00:10:00]

Description was changed from

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

Additional Changes:
* Renames LocationArbitratorImpl -> LocationArbitrator

BUG=614486
==========

to

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

Additional Changes:
* Renames LocationArbitratorImpl -> LocationArbitrator

BUG=614486, 641071
==========

[commit-bot: I haz the power, 2016-09-01 00:14:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 00:14:02]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[CJ, 2016-09-01 00:19:16]

The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 00:20:01]

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

[nyquist, 2016-09-01 00:34:30]

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.cc:73: geolocation_delegate_(new
device::GeolocationDelegate),
Nit: base::MakeUnique here and below

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:9: #include <string>
thanks!

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:23: #include
"device/geolocation/geolocation_delegate.h"
can this be forward declared?

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:105:
std::unique_ptr<device::GeolocationDelegate> geolocation_delegate_;
Could you have a quick comment here as to what this delegate is for?

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:43: geolocation_delegate_(new
device::GeolocationDelegate),
Nit: base::MakeUnique here and below too

[CJ, 2016-09-01 00:45:40]

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.cc (right):

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.cc:73: geolocation_delegate_(new
device::GeolocationDelegate),
On 2016/09/01 00:34:30, nyquist wrote:
> Nit: base::MakeUnique here and below

Done. Why is it that we do MakeUnique here but not for the rest of the features?

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:23: #include
"device/geolocation/geolocation_delegate.h"
On 2016/09/01 00:34:30, nyquist wrote:
> can this be forward declared?

Done.

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:105:
std::unique_ptr<device::GeolocationDelegate> geolocation_delegate_;
On 2016/09/01 00:34:30, nyquist wrote:
> Could you have a quick comment here as to what this delegate is for?

Done.

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/2249283003/diff/260001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:43: geolocation_delegate_(new
device::GeolocationDelegate),
On 2016/09/01 00:34:30, nyquist wrote:
> Nit: base::MakeUnique here and below too

Done.

[Wez, 2016-09-01 02:45:43]

LGTM once the issues below are resolved.

nit for future CLs: It would have been preferable to keep the
LocationArbitratorImpl renaming separate from the Blimp-specific changes, IMO,
since the two are unrelated. :)

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_permission_manager.cc (right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/app/blimp...
blimp/engine/app/blimp_permission_manager.cc:26:
callback.Run(blink::mojom::PermissionStatus::GRANTED);
Let's not grant *all* permissions - for now we want to grant Geolocation
permissions but lets continue to deny everything else.

Let's also add a VLOG() to indicate that a permission was requested, so that
with verbose logging we have clear visibility into that (e.g. when you were
hooking up Geolocation, verbose logging would have highlighted the permission
issue!)

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.cc:162: task_runner_
= std::move(runner);
Why can't we just assign task_runner_ directly in SetUpdateCallback?

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature.h (right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.h:47: void
SetTaskRunner(scoped_refptr<base::SingleThreadTaskRunner> runner);
This method isn't part of the Delegate interface, so shouldn't be grouped with
the other methods.

Why do we need it at all, though?  Can't we just set the TaskRunner directly, in
SetUpdateCallback()?

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc
(right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:124: //
Runs tasks posted during the processing of incoming messages.
nit: "Used to run tasks..."

It's really the RunLoop().RunUntilIdle() calls that run the tasks.

[CJ, 2016-09-01 18:17:05]

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_permission_manager.cc (right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/app/blimp...
blimp/engine/app/blimp_permission_manager.cc:26:
callback.Run(blink::mojom::PermissionStatus::GRANTED);
On 2016/09/01 02:45:43, Wez wrote:
> Let's not grant *all* permissions - for now we want to grant Geolocation
> permissions but lets continue to deny everything else.
> 
> Let's also add a VLOG() to indicate that a permission was requested, so that
> with verbose logging we have clear visibility into that (e.g. when you were
> hooking up Geolocation, verbose logging would have highlighted the permission
> issue!)

Done.

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.cc:162: task_runner_
= std::move(runner);
On 2016/09/01 02:45:43, Wez wrote:
> Why can't we just assign task_runner_ directly in SetUpdateCallback?

It's because ThreadTaskRunnerHandle::Get() returns a const task runner, and cant
be assigned to the non-const |task_runner_|. Is there a better way to handle
this?

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc
(right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:124: //
Runs tasks posted during the processing of incoming messages.
On 2016/09/01 02:45:43, Wez wrote:
> nit: "Used to run tasks..."
> 
> It's really the RunLoop().RunUntilIdle() calls that run the tasks.

Done.

[CJ, 2016-09-01 18:17:05]

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_permission_manager.cc (right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/app/blimp...
blimp/engine/app/blimp_permission_manager.cc:26:
callback.Run(blink::mojom::PermissionStatus::GRANTED);
On 2016/09/01 02:45:43, Wez wrote:
> Let's not grant *all* permissions - for now we want to grant Geolocation
> permissions but lets continue to deny everything else.
> 
> Let's also add a VLOG() to indicate that a permission was requested, so that
> with verbose logging we have clear visibility into that (e.g. when you were
> hooking up Geolocation, verbose logging would have highlighted the permission
> issue!)

Done.

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.cc:162: task_runner_
= std::move(runner);
On 2016/09/01 02:45:43, Wez wrote:
> Why can't we just assign task_runner_ directly in SetUpdateCallback?

It's because ThreadTaskRunnerHandle::Get() returns a const task runner, and cant
be assigned to the non-const |task_runner_|. Is there a better way to handle
this?

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc
(right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:124: //
Runs tasks posted during the processing of incoming messages.
On 2016/09/01 02:45:43, Wez wrote:
> nit: "Used to run tasks..."
> 
> It's really the RunLoop().RunUntilIdle() calls that run the tasks.

Done.

[Kevin M, 2016-09-01 20:01:07]

https://codereview.chromium.org/2249283003/diff/340001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:23: 
Remove this newline

https://codereview.chromium.org/2249283003/diff/340001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:44:
geolocation_feature_(base::MakeUnique<GeolocationFeature>(
Can we move ownership of the GeolocationDelegate object to the Feature? I don't
see any need to have the pointer held here. Life's much easier when objects have
control over the lifetime of their dependencies.

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_permission_manager.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/app/blimp...
blimp/engine/app/blimp_permission_manager.cc:27: VLOG(1) << "Permission
requested and granted.";
"Geolocation permission granted"

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/app/blimp...
blimp/engine/app/blimp_permission_manager.cc:30: VLOG(1) << "Permission
requested and denied.";
Log |permission|?

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.cc:157:
SetTaskRunner(base::ThreadTaskRunnerHandle::Get());
This probably doesn't need a helper function - maybe just split it into a
separate block w/a blank line and a little comment at the top.

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.cc:160: void
EngineGeolocationFeature::SetTaskRunner(
Recommend renaming |task_runner_| to |callback_task_runner_|, to make it crystal
clear which operations are being posted to the TaskRunner.

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc
(right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:124: //
Provides RunLoop().RunUntilIdle() to run tasks posted during the
Nit: the MessageLoop doesn't actually provide that functionality, that's the
RunLoop's doing. :)

How about this: "Processes tasks that were posted as a result of processing
incoming messages." ?

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
File device/geolocation/location_arbitrator_unittest.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
device/geolocation/location_arbitrator_unittest.cc:25: // Need a vtable for
GMock.
This class really has nothing to do with GMock. I has a confus.

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
device/geolocation/location_arbitrator_unittest.cc:32: // Delegate
This comment doesn't make any sense?

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
device/geolocation/location_arbitrator_unittest.cc:38: Geoposition
last_position_;
Member fields shouldn't be directly public gettable - needs to be mediated by a
getter instead.

[Wez, 2016-09-01 20:08:18]

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right):

https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.cc:162: task_runner_
= std::move(runner);
On 2016/09/01 18:17:05, CJ wrote:
> On 2016/09/01 02:45:43, Wez wrote:
> > Why can't we just assign task_runner_ directly in SetUpdateCallback?
> 
> It's because ThreadTaskRunnerHandle::Get() returns a const task runner, and
cant
> be assigned to the non-const |task_runner_|. Is there a better way to handle
> this?

You can just assigned the result of Get() directly into a
scoped_refptr<TaskRunner> - that's effectively what you're doing by passing it
to SetTaskRunner(), since that takes a scoped_refptr<>.

[CJ, 2016-09-01 22:49:37]

On 2016/09/01 20:08:18, Wez wrote:
>
https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
> File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right):
> 
>
https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
> blimp/engine/feature/geolocation/engine_geolocation_feature.cc:162:
task_runner_
> = std::move(runner);
> On 2016/09/01 18:17:05, CJ wrote:
> > On 2016/09/01 02:45:43, Wez wrote:
> > > Why can't we just assign task_runner_ directly in SetUpdateCallback?
> > 
> > It's because ThreadTaskRunnerHandle::Get() returns a const task runner, and
> cant
> > be assigned to the non-const |task_runner_|. Is there a better way to handle
> > this?
> 
> You can just assigned the result of Get() directly into a
> scoped_refptr<TaskRunner> - that's effectively what you're doing by passing it
> to SetTaskRunner(), since that takes a scoped_refptr<>.

The compiler blows up if i do that cause the value is const.

[CJ, 2016-09-01 22:59:28]

On 2016/09/01 22:49:37, CJ wrote:
> On 2016/09/01 20:08:18, Wez wrote:
> >
>
https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
> > File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right):
> > 
> >
>
https://codereview.chromium.org/2249283003/diff/320001/blimp/engine/feature/g...
> > blimp/engine/feature/geolocation/engine_geolocation_feature.cc:162:
> task_runner_
> > = std::move(runner);
> > On 2016/09/01 18:17:05, CJ wrote:
> > > On 2016/09/01 02:45:43, Wez wrote:
> > > > Why can't we just assign task_runner_ directly in SetUpdateCallback?
> > > 
> > > It's because ThreadTaskRunnerHandle::Get() returns a const task runner,
and
> > cant
> > > be assigned to the non-const |task_runner_|. Is there a better way to
handle
> > > this?
> > 
> > You can just assigned the result of Get() directly into a
> > scoped_refptr<TaskRunner> - that's effectively what you're doing by passing
it
> > to SetTaskRunner(), since that takes a scoped_refptr<>.
> 
> The compiler blows up if i do that cause the value is const.

Nevermind. Apparently the error disappeared. Confused, but pleased.

[CJ, 2016-09-01 22:59:43]

https://codereview.chromium.org/2249283003/diff/340001/blimp/client/core/blim...
File blimp/client/core/blimp_client_context_impl.h (right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/client/core/blim...
blimp/client/core/blimp_client_context_impl.h:23: 
On 2016/09/01 20:01:06, Kevin M wrote:
> Remove this newline

Done.

https://codereview.chromium.org/2249283003/diff/340001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:44:
geolocation_feature_(base::MakeUnique<GeolocationFeature>(
On 2016/09/01 20:01:06, Kevin M wrote:
> Can we move ownership of the GeolocationDelegate object to the Feature? I
don't
> see any need to have the pointer held here. Life's much easier when objects
have
> control over the lifetime of their dependencies.

Gave it to LocationArbitrator, since it is the one to use it anyway.

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/app/blimp...
File blimp/engine/app/blimp_permission_manager.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/app/blimp...
blimp/engine/app/blimp_permission_manager.cc:27: VLOG(1) << "Permission
requested and granted.";
On 2016/09/01 20:01:07, Kevin M wrote:
> "Geolocation permission granted"

Done.

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/app/blimp...
blimp/engine/app/blimp_permission_manager.cc:30: VLOG(1) << "Permission
requested and denied.";
On 2016/09/01 20:01:06, Kevin M wrote:
> Log |permission|?

It's an enum class.

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.cc:157:
SetTaskRunner(base::ThreadTaskRunnerHandle::Get());
On 2016/09/01 20:01:07, Kevin M wrote:
> This probably doesn't need a helper function - maybe just split it into a
> separate block w/a blank line and a little comment at the top.

Done.

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature.cc:160: void
EngineGeolocationFeature::SetTaskRunner(
On 2016/09/01 20:01:07, Kevin M wrote:
> Recommend renaming |task_runner_| to |callback_task_runner_|, to make it
crystal
> clear which operations are being posted to the TaskRunner.

Done.

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
File blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc
(right):

https://codereview.chromium.org/2249283003/diff/340001/blimp/engine/feature/g...
blimp/engine/feature/geolocation/engine_geolocation_feature_unittest.cc:124: //
Provides RunLoop().RunUntilIdle() to run tasks posted during the
On 2016/09/01 20:01:07, Kevin M wrote:
> Nit: the MessageLoop doesn't actually provide that functionality, that's the
> RunLoop's doing. :)
> 
> How about this: "Processes tasks that were posted as a result of processing
> incoming messages." ?

Done.

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
File device/geolocation/location_arbitrator_unittest.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
device/geolocation/location_arbitrator_unittest.cc:25: // Need a vtable for
GMock.
On 2016/09/01 20:01:07, Kevin M wrote:
> This class really has nothing to do with GMock. I has a confus.

Maybe they are referring to how to get rid of the FakeLocationProviders and use
actual MockLocationProviders. I didn't see that comment, and I'll probably go
ping whoever wrote it to see if they have any idea's of how to approach, because
the problem with using MockLocationProviders is that all the LocationProvider's
LocationArbitrator uses are internal to itself, and doesn't lend itself well to
true mocking. Either way, getting rid of the comment.

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
device/geolocation/location_arbitrator_unittest.cc:32: // Delegate
On 2016/09/01 20:01:07, Kevin M wrote:
> This comment doesn't make any sense?

yeah idk.

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
device/geolocation/location_arbitrator_unittest.cc:38: Geoposition
last_position_;
On 2016/09/01 20:01:07, Kevin M wrote:
> Member fields shouldn't be directly public gettable - needs to be mediated by
a
> getter instead.

Done.

[Wez, 2016-09-01 23:08:28]

For this in the cheap-seats, we think the issue was that the breaking code
was:

task_runner_ = std::move(ThreadTaskRunnerHandle::Get());

which won't compile since the result of Get() is a const scoped_refptr&,
i.e. std::move() can't move the pointer out of it, only copy it.

On 1 September 2016 at 15:49, <lethalantidote@chromium.org> wrote:

> On 2016/09/01 20:08:18, Wez wrote:
> >
> https://codereview.chromium.org/2249283003/diff/320001/
> blimp/engine/feature/geolocation/engine_geolocation_feature.cc
> > File blimp/engine/feature/geolocation/engine_geolocation_feature.cc
> (right):
> >
> >
> https://codereview.chromium.org/2249283003/diff/320001/
> blimp/engine/feature/geolocation/engine_geolocation_feature.cc#newcode162
> > blimp/engine/feature/geolocation/engine_geolocation_feature.cc:162:
> task_runner_
> > = std::move(runner);
> > On 2016/09/01 18:17:05, CJ wrote:
> > > On 2016/09/01 02:45:43, Wez wrote:
> > > > Why can't we just assign task_runner_ directly in SetUpdateCallback?
> > >
> > > It's because ThreadTaskRunnerHandle::Get() returns a const task
> runner, and
> > cant
> > > be assigned to the non-const |task_runner_|. Is there a better way to
> handle
> > > this?
> >
> > You can just assigned the result of Get() directly into a
> > scoped_refptr<TaskRunner> - that's effectively what you're doing by
> passing it
> > to SetTaskRunner(), since that takes a scoped_refptr<>.
>
> The compiler blows up if i do that cause the value is const.
>
> https://codereview.chromium.org/2249283003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2016-09-01 23:11:55]

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
File device/geolocation/location_arbitrator_unittest.cc (right):

https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
device/geolocation/location_arbitrator_unittest.cc:25: // Need a vtable for
GMock.
On 2016/09/01 22:59:43, CJ wrote:
> On 2016/09/01 20:01:07, Kevin M wrote:
> > This class really has nothing to do with GMock. I has a confus.
> 
> Maybe they are referring to how to get rid of the FakeLocationProviders and
use
> actual MockLocationProviders. I didn't see that comment, and I'll probably go
> ping whoever wrote it to see if they have any idea's of how to approach,
because
> the problem with using MockLocationProviders is that all the
LocationProvider's
> LocationArbitrator uses are internal to itself, and doesn't lend itself well
to
> true mocking. Either way, getting rid of the comment.

This was all code in the original unit-test, and you're just renaming it, right,
CJ?

Fundamentally this class is not a mock at all, is it?

P.S. The class and all the incidental bits below (GetTimeNowForTest etc) should
be in an anonymous namespace.

[CJ, 2016-09-01 23:28:53]

On 2016/09/01 23:11:55, Wez wrote:
>
https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
> File device/geolocation/location_arbitrator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2249283003/diff/340001/device/geolocation/loc...
> device/geolocation/location_arbitrator_unittest.cc:25: // Need a vtable for
> GMock.
> On 2016/09/01 22:59:43, CJ wrote:
> > On 2016/09/01 20:01:07, Kevin M wrote:
> > > This class really has nothing to do with GMock. I has a confus.
> > 
> > Maybe they are referring to how to get rid of the FakeLocationProviders and
> use
> > actual MockLocationProviders. I didn't see that comment, and I'll probably
go
> > ping whoever wrote it to see if they have any idea's of how to approach,
> because
> > the problem with using MockLocationProviders is that all the
> LocationProvider's
> > LocationArbitrator uses are internal to itself, and doesn't lend itself well
> to
> > true mocking. Either way, getting rid of the comment.
> 
> This was all code in the original unit-test, and you're just renaming it,
right,
> CJ?
> 
> Fundamentally this class is not a mock at all, is it?
> 
> P.S. The class and all the incidental bits below (GetTimeNowForTest etc)
should
> be in an anonymous namespace.

Right and right. Putting stuff into an anonymous namespace while Im here I
guess.

[CJ, 2016-09-01 23:37:31]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-09-01 23:37:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmarshall@chromium.org, mcasas@chromium.org, wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2249283003/#ps410001
(title: "Merge branch 'master' into blp")

[commit-bot: I haz the power, 2016-09-01 23:37:51]

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

[nyquist, 2016-09-02 00:24:04]

lgtm

[commit-bot: I haz the power, 2016-09-02 00:39:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 00:39:16]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[CJ, 2016-09-02 03:44:52]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-09-02 03:44:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, mcasas@chromium.org, wez@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/2249283003/#ps430001
(title: "Put TestingLocationArbitrator out of anonymous namespace.")

[commit-bot: I haz the power, 2016-09-02 03:45:08]

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-09-02 04:59:43]

Description was changed from

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

Additional Changes:
* Renames LocationArbitratorImpl -> LocationArbitrator

BUG=614486, 641071
==========

to

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

Additional Changes:
* Renames LocationArbitratorImpl -> LocationArbitrator

BUG=614486, 641071
==========

[commit-bot: I haz the power, 2016-09-02 04:59:46]

Committed patchset #23 (id:430001)

[commit-bot: I haz the power, 2016-09-02 05:01:45]

Description was changed from

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

Additional Changes:
* Renames LocationArbitratorImpl -> LocationArbitrator

BUG=614486, 641071
==========

to

==========
Hooks together Geolocation Feature in the Client and Engine.

This CL registers the GeolocationFeature with the
BlimpClientSession, allows the Client-side Geolocation Feature to communicate
with the LocationArbitrator, and makes the necessary changes to permissions.

Additional Changes:
* Renames LocationArbitratorImpl -> LocationArbitrator

BUG=614486, 641071

Committed: https://crrev.com/8e0b2a7306378c41550a589dfbb8cefa0ebabca0
Cr-Commit-Position: refs/heads/master@{#416190}
==========

[commit-bot: I haz the power, 2016-09-02 05:01:47]

Patchset 23 (id:??) landed as
https://crrev.com/8e0b2a7306378c41550a589dfbb8cefa0ebabca0
Cr-Commit-Position: refs/heads/master@{#416190}