Clear Reference Location State When Providers are Stopped

Clear Reference Location State When Providers are Stopped

The location arbitrator relies on pointer comparisons to identify
which location provider provided a reported location.

Now, we clear this state when the providers are shut down.

This prevents a bug where a stale location prevented the arbitrator
from updating its clients with a newer location.

BUG=240956
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203924

[robliao, 2013-05-31 21:11:59]

Here's a fix for 240956

[vadimt, 2013-05-31 21:19:58]

There is no need for 2 "BUG=" in the CL description.

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
File content/browser/geolocation/location_arbitrator_impl.cc (right):

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
content/browser/geolocation/location_arbitrator_impl.cc:74: // use fresh
locations from the newly constructed providers.
This comment applies more to the second assignment.
NULLing position_provider_ is simply for avoiding strange pointer to garbage.

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
content/browser/geolocation/location_arbitrator_impl.cc:75: position_provider_ =
nullptr;
position_provider_ was initialized with NULL, so it makes sense to use NULL here
too.

[robliao, 2013-05-31 21:26:25]

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
File content/browser/geolocation/location_arbitrator_impl.cc (right):

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
content/browser/geolocation/location_arbitrator_impl.cc:74: // use fresh
locations from the newly constructed providers.
The position as well as the provider in this class are treated as pairs. It does
not make sense in this class to have a position without a provider.
On 2013/05/31 21:19:58, vadimt wrote:
> This comment applies more to the second assignment.
> NULLing position_provider_ is simply for avoiding strange pointer to garbage.

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
content/browser/geolocation/location_arbitrator_impl.cc:75: position_provider_ =
nullptr;
Changed position_provider_ to use nullptr.
On 2013/05/31 21:19:58, vadimt wrote:
> position_provider_ was initialized with NULL, so it makes sense to use NULL
here
> too.

[vadimt, 2013-05-31 21:38:12]

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
File content/browser/geolocation/location_arbitrator_impl.cc (right):

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
content/browser/geolocation/location_arbitrator_impl.cc:75: position_provider_ =
nullptr;
On 2013/05/31 21:26:25, Robert Liao wrote:
> Changed position_provider_ to use nullptr.
> On 2013/05/31 21:19:58, vadimt wrote:
> > position_provider_ was initialized with NULL, so it makes sense to use NULL
> here
> > too.
> 

That's cool, but you are the first one to use nullptr in src\content. Could you
also change the remaining 3386 NULLs in this folders to nullptr :) ?

[robliao, 2013-05-31 22:24:28]

On 2013/05/31 21:38:12, vadimt wrote:
>
https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
> File content/browser/geolocation/location_arbitrator_impl.cc (right):
> 
>
https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
> content/browser/geolocation/location_arbitrator_impl.cc:75: position_provider_
=
> nullptr;
> On 2013/05/31 21:26:25, Robert Liao wrote:
> > Changed position_provider_ to use nullptr.
> > On 2013/05/31 21:19:58, vadimt wrote:
> > > position_provider_ was initialized with NULL, so it makes sense to use
NULL
> > here
> > > too.
> > 
> 
> That's cool, but you are the first one to use nullptr in src\content. Could
you
> also change the remaining 3386 NULLs in this folders to nullptr :) ?

That's probably the right thing to do, and we should do that in a separate
changelist.

[vadimt, 2013-05-31 22:46:02]

On 2013/05/31 22:24:28, Robert Liao wrote:
> On 2013/05/31 21:38:12, vadimt wrote:
> >
>
https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
> > File content/browser/geolocation/location_arbitrator_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
> > content/browser/geolocation/location_arbitrator_impl.cc:75:
position_provider_
> =
> > nullptr;
> > On 2013/05/31 21:26:25, Robert Liao wrote:
> > > Changed position_provider_ to use nullptr.
> > > On 2013/05/31 21:19:58, vadimt wrote:
> > > > position_provider_ was initialized with NULL, so it makes sense to use
> NULL
> > > here
> > > > too.
> > > 
> > 
> > That's cool, but you are the first one to use nullptr in src\content. Could
> you
> > also change the remaining 3386 NULLs in this folders to nullptr :) ?
> 
> That's probably the right thing to do, and we should do that in a separate
> changelist.

May be.
Please use NULLs for this CL.
It also makes sense to start with talking to file owners for "#define NULL 0".
May be, they already plane something...
Please also make sure that that work doesn't conflict with fixing location bugs
priorities. We need to get our product of of the door in time :)

[vadimt, 2013-05-31 22:48:51]

lgtm

LGTM.

Please request OWNER's approval from bulach@. He already knows that we've
volunteered to fix geolocation bugs.

For future: Please don't leave unanswered comments in code; mark them with
"Done".

[robliao, 2013-06-01 00:03:24]

bulach: I request owner approval for this CL.

Thanks!

Robert

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
File content/browser/geolocation/location_arbitrator_impl.cc (right):

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
content/browser/geolocation/location_arbitrator_impl.cc:74: // use fresh
locations from the newly constructed providers.
On 2013/05/31 21:26:25, Robert Liao wrote:
> The position as well as the provider in this class are treated as pairs. It
does
> not make sense in this class to have a position without a provider.
> On 2013/05/31 21:19:58, vadimt wrote:
> > This comment applies more to the second assignment.
> > NULLing position_provider_ is simply for avoiding strange pointer to
garbage.
> 

Done.

https://codereview.chromium.org/15968009/diff/1/content/browser/geolocation/l...
content/browser/geolocation/location_arbitrator_impl.cc:75: position_provider_ =
nullptr;
On 2013/05/31 21:26:25, Robert Liao wrote:
> Changed position_provider_ to use nullptr.
> On 2013/05/31 21:19:58, vadimt wrote:
> > position_provider_ was initialized with NULL, so it makes sense to use NULL
> here
> > too.
> 

Done.

[bulach, 2013-06-03 10:50:54]

lgtm, thanks!

any chance of making an explicit test for the comment you added?
http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/geolocation/l...

[robliao, 2013-06-03 18:25:39]

Added a unit test to test 240956

[vadimt, 2013-06-03 18:58:04]

lgtm

Still LGTM

[commit-bot: I haz the power, 2013-06-03 21:56:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/15968009/23004

[commit-bot: I haz the power, 2013-06-04 11:02:23]

Change committed as 203924