Checks to see if the position is valid before OnLocationUpdate call.

Checks to see if the position is valid before OnLocationUpdate call.

A StopProvider call from the Engine was being received while a message
was being processed to go to the Engine from the Client. This caused
the LocationArbitrator to reset. When we came back from sending,
the callback tries to get the new location from the LocationArbitrator,
but since it was reset, it now has an invalid location. This adds a
check for this case.

BUG=645230
Committed: https://crrev.com/cb91640779ff557012cbf8bf728afd6166555a49
Cr-Commit-Position: refs/heads/master@{#419258}

[CJ, 2016-09-09 21:52:08]

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

[Wez, 2016-09-13 20:22:12]

lgtm

[Wez, 2016-09-13 20:26:08]

https://codereview.chromium.org/2331513002/diff/40001/blimp/client/core/geolo...
File blimp/client/core/geolocation/geolocation_feature.cc (right):

https://codereview.chromium.org/2331513002/diff/40001/blimp/client/core/geolo...
blimp/client/core/geolocation/geolocation_feature.cc:147: if
(need_to_send_message_ && new_position.Validate()) {
Hmmm, actually isn't Validate() a proxy for whether the LocationProvider is
started? Should we just track whether we started it, and use that, and then
DCHECK that the position is valid? Or are there other reasons it may become
invalid?

[CJ, 2016-09-13 22:45:22]

https://codereview.chromium.org/2331513002/diff/40001/blimp/client/core/geolo...
File blimp/client/core/geolocation/geolocation_feature.cc (right):

https://codereview.chromium.org/2331513002/diff/40001/blimp/client/core/geolo...
blimp/client/core/geolocation/geolocation_feature.cc:147: if
(need_to_send_message_ && new_position.Validate()) {
On 2016/09/13 20:26:08, Wez wrote:
> Hmmm, actually isn't Validate() a proxy for whether the LocationProvider is
> started? Should we just track whether we started it, and use that, and then
> DCHECK that the position is valid? Or are there other reasons it may become
> invalid?

It has to be this way because even if the LocationProvider is considered
started, it isnt until the first update that we can a non-valid new_position. So
there's an edge case where the LocationProvider is started and has not yet
returned a geoposition update.

[Wez, 2016-09-16 01:20:27]

lgtm

https://codereview.chromium.org/2331513002/diff/40001/blimp/client/core/geolo...
File blimp/client/core/geolocation/geolocation_feature.cc (right):

https://codereview.chromium.org/2331513002/diff/40001/blimp/client/core/geolo...
blimp/client/core/geolocation/geolocation_feature.cc:147: if
(need_to_send_message_ && new_position.Validate()) {
On 2016/09/13 22:45:22, CJ wrote:
> On 2016/09/13 20:26:08, Wez wrote:
> > Hmmm, actually isn't Validate() a proxy for whether the LocationProvider is
> > started? Should we just track whether we started it, and use that, and then
> > DCHECK that the position is valid? Or are there other reasons it may become
> > invalid?
> 
> It has to be this way because even if the LocationProvider is considered
> started, it isnt until the first update that we can a non-valid new_position.
So
> there's an edge case where the LocationProvider is started and has not yet
> returned a geoposition update.  

But will we see an OnSendComplete() in that case? If we're stopped then
presumably nothing is sending? Or is there a race between start/stop and message
send?

nit: I'd suggest turning the need_to_send_message_ check into an early-exit, so
that you remove the overhead of GetPosition() in the case where there's nothing
to send (though that's fairly minor :).

[CJ, 2016-09-16 18:27:12]

https://codereview.chromium.org/2331513002/diff/40001/blimp/client/core/geolo...
File blimp/client/core/geolocation/geolocation_feature.cc (right):

https://codereview.chromium.org/2331513002/diff/40001/blimp/client/core/geolo...
blimp/client/core/geolocation/geolocation_feature.cc:147: if
(need_to_send_message_ && new_position.Validate()) {
On 2016/09/16 01:20:27, Wez wrote:
> On 2016/09/13 22:45:22, CJ wrote:
> > On 2016/09/13 20:26:08, Wez wrote:
> > > Hmmm, actually isn't Validate() a proxy for whether the LocationProvider
is
> > > started? Should we just track whether we started it, and use that, and
then
> > > DCHECK that the position is valid? Or are there other reasons it may
become
> > > invalid?
> > 
> > It has to be this way because even if the LocationProvider is considered
> > started, it isnt until the first update that we can a non-valid
new_position.
> So
> > there's an edge case where the LocationProvider is started and has not yet
> > returned a geoposition update.  
> 
> But will we see an OnSendComplete() in that case? If we're stopped then
> presumably nothing is sending? Or is there a race between start/stop and
message
> send?
> 
> nit: I'd suggest turning the need_to_send_message_ check into an early-exit,
so
> that you remove the overhead of GetPosition() in the case where there's
nothing
> to send (though that's fairly minor :).

There's that race case. A Stop message gets received on the Client while a
Coordinate message is in flight. Since the Stop message was just a Stop call and
not the result of something getting destructed, the Coordinate message gets sent
just fine, but the Stop call already resets the LocationArbitrator, causing the
cache location to be invalid.

[CJ, 2016-09-16 18:27:19]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-09-16 18:27:20]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2331513002/#ps60001
(title: "Addresses Wez's #6 comments.")

[commit-bot: I haz the power, 2016-09-16 18:27:49]

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

[CJ, 2016-09-16 18:53:57]

The CQ bit was checked by lethalantidote@chromium.org

[CJ, 2016-09-16 18:53:57]

The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org
Link to the patchset: https://codereview.chromium.org/2331513002/#ps100001
(title: "Tidy up.")

[commit-bot: I haz the power, 2016-09-16 18:54:25]

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-16 19:55:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-16 19:57:32]

Description was changed from

==========
Checks to see if the position is valid before OnLocationUpdate call.

A StopProvider call from the Engine was being received while a message
was being processed to go to the Engine from the Client. This caused
the LocationArbitrator to reset. When we came back from sending,
the callback tries to get the new location from the LocationArbitrator,
but since it was reset, it now has an invalid location. This adds a
check for this case.

BUG=645230
==========

to

==========
Checks to see if the position is valid before OnLocationUpdate call.

A StopProvider call from the Engine was being received while a message
was being processed to go to the Engine from the Client. This caused
the LocationArbitrator to reset. When we came back from sending,
the callback tries to get the new location from the LocationArbitrator,
but since it was reset, it now has an invalid location. This adds a
check for this case.

BUG=645230

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

[commit-bot: I haz the power, 2016-09-16 19:57:33]

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