Do not send stale position in GeolocationProvider::AddObserver.

Do not stale cached position in GeolocationProvider::AddObserver.

WebKit is expecting a fresh geolocation positions from the
GeolocationClient, so we should try to send a fresh geolocation
position.

TEST=
BUG=157244
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170806

[John Knottenbelt, 2012-11-13 14:36:14]

Hi Marcus, Joth,

Comments much appreciated. I'm wondering about tests for this, too. I think
perhaps something in geolocation_provider_unittest?

Could this change negatively impact sites that previously would have received an
instant cached position? 

John

[joth, 2012-11-13 18:53:21]

On 13 November 2012 14:36, <jknotten@chromium.org> wrote:

> Reviewers: joth, bulach,
>
> Message:
> Hi Marcus, Joth,
>
> Comments much appreciated. I'm wondering about tests for this, too. I think
> perhaps something in geolocation_provider_unittest?
>
> Could this change negatively impact sites that previously would have
> received an
> instant cached position?
>
> John
>
>
> Description:
> Do not send cached position in GeolocationProvider::**AddObserver.
>
> WebKit is expecting a fresh geolocation positions from the
> GeolocationClient, so we should try to send a fresh geolocation
> position.
>
> TEST=
> BUG=157244
>
>
> Please review this at
https://codereview.chromium.**org/11312210/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M content/browser/geolocation/**geolocation_provider.cc
>
>
> Index: content/browser/geolocation/**geolocation_provider.cc
> diff --git a/content/browser/geolocation/**geolocation_provider.cc
> b/content/browser/geolocation/**geolocation_provider.cc
> index 2b626d8db0c1ae01e953ae5c1ad6ee**faa6523c89..**
> c9789b44e44219ef869e312a993c97**75a3ee77af 100644
> --- a/content/browser/geolocation/**geolocation_provider.cc
> +++ b/content/browser/geolocation/**geolocation_provider.cc
> @@ -21,9 +21,13 @@ void
GeolocationProvider::**AddObserver(**GeolocationObserver*
> observer,
>    DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::IO)**);
>    observers_[observer] = update_options;
>    OnClientsChanged();
> -  if (position_.Validate() ||
> -      position_.error_code != Geoposition::ERROR_CODE_NONE)
> -    observer->OnLocationUpdate(**position_);
> +  if (ignore_location_updates_) {
> +    // Position has been overridden for testing. Tests expect the
> observers to
> +    // be called synchronously in this case.
>

This sounds bad: writing production code purely for the sake of tests means
the tests are actually covering the things we want them too?


> +    if (position_.Validate() ||
> +        position_.error_code != Geoposition::ERROR_CODE_NONE)
> +      observer->OnLocationUpdate(**position_);
> +  }
>  }
>
>  bool GeolocationProvider::**RemoveObserver(**GeolocationObserver*
> observer) {
>
>
>

[John Knottenbelt, 2012-11-13 18:58:17]

I agree. 

I would prefer not to have this section in there at all, and fix the tests. The
GeolocationPosition::OverrideLocationForTesting code is written for ChromeDriver
support (Selenium), but I want to get your opinion on removing the initial
send-of-cached position when AddObserver is called before persuing it.

[bulach, 2012-11-13 19:22:27]

hmm... if i remember the flow correctly, it's something like:

GeolocationProvider::AddObserver
GeolocationProvider::OnClientsChanged
GeolocationProvider::AddObserver
GeolocationProvider::StartProviders
LocationArbitrator::StartProviders
NetworkLocationProvider::StartProvider
^^^ at this point, if we've already done a wifi scan in the past, we'd just wait
for the next wifi poll, which may not happen for up to 10mins (see
wifi_data_provider_linux.cc)..

if the above flow is correct (don't trust me, please double check! :), this may
end up with a really bad latency for those cases...

perhaps we should change the logic both here and webkit so that:
- if we have a new request, try to poke the providers if the location is not
fresh enough...

- perhaps: send the last location to webkit and let it decide if that's good
enough.. I think deep down in WebCore/Modules/Geolocation.cpp it blindly passes
whatever it receives.. alternatively, send the age requirements all the way up
here so that this layer can decide whether to kick its providers or use the last
one...

wdyt?

[John Knottenbelt, 2012-11-14 15:56:26]

You're right. If I have a second tab with a geolocation watch, it causes the
first to take forever to answer getCurrentPosition. I think this is partly due
to poll frequency, and partly that the WifiScan results should be significantly
different before passing on the wifi scan to the network geolocation provider
for lookup.

[John Knottenbelt, 2012-11-18 16:03:07]

https://codereview.chromium.org/11312210/diff/1/content/browser/geolocation/g...
File content/browser/geolocation/geolocation_provider.cc (right):

https://codereview.chromium.org/11312210/diff/1/content/browser/geolocation/g...
content/browser/geolocation/geolocation_provider.cc:29:
observer->OnLocationUpdate(position_);
We don't want to send this if we're the first observer, as we'll want to take a
fresh fix. But if we're not the first then we want to send the most recent fresh
position we have. When the last observer unregisters, we should clear this
cached position.

[vadimt, 2012-11-21 23:25:23]

I’ve tried the patch, and found a strange behavior.

Repro:
I have Windows 7, with no Wifi.
I open 1 web page that has a button which makes getCurrentPosition requests.
maximumAge 1 sec. This works correctly, thanks to the fix, and the age of the
result is never higher than 1 sec.
Then I open another tab, which makes watchPosition request.
While watchPosition is active, I return to the getCurrentPosition page and hit
the button. Now pressing a button can result in old results, despite 1 sec age
limit, for example, I saw 485 sec.

To repro, you can go to this web site:
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_geolocation, and open
it in 2 different tabs.
Copy the scripts that I include below into 2 tabs, press “Submit Code” button on
the both, click “Try It” button on the watchPosition tab. Then, keep clicking
“Try It” on the getCurrentPosition tab.

Don’t forget to click “Submit Code” button on both before "Try It"!

*** getCurrentPosition script: ***
<!DOCTYPE html>
<html>
<body>
<p id="demo">Click the button to get your coordinates:</p>
<button onclick="getLocation()">Try It</button>
<script>
var x=document.getElementById("demo");
function getLocation()
  {
  if (navigator.geolocation)
    {
    navigator.geolocation.getCurrentPosition(showPosition, function (e) {
alert('Error! ' + JSON.stringify(e)); }, {maximumAge:1000});
    }
  else{x.innerHTML="Geolocation is not supported by this browser.";}
  }
function showPosition(position)
  {
  x.innerHTML="Latitude: " + position.coords.latitude + 
  "<br>Longitude: " + position.coords.longitude +
  "<br>Age: " + (new Date().getTime() - position.timestamp)/1000 +
  "<br>Whole: " + JSON.stringify(position);	
  }
</script>
</body>
</html>

*** watchPosition script: ***
<!DOCTYPE html>
<html>
<body>
<p id="demo">Click the button to get your coordinates:</p>
<button onclick="getLocation()">Try It</button>
<script>
var x=document.getElementById("demo");
function getLocation()
  {
  if (navigator.geolocation)
    {
    navigator.geolocation.watchPosition(showPosition, function (e) {
alert('Error! ' + JSON.stringify(e)); });
    }
  else{x.innerHTML="Geolocation is not supported by this browser.";}
  }
function showPosition(position)
  {
  x.innerHTML="Latitude: " + position.coords.latitude + 
  "<br>Longitude: " + position.coords.longitude +
  "<br>Age: " + (new Date().getTime() - position.timestamp)/1000 +
  "<br>Whole: " + JSON.stringify(position);	
  }
</script>
</body>
</html>

[John Knottenbelt, 2012-11-22 10:39:54]

Thanks for testing, Vadim! As you've observed, this patch isn't the whole story
regarding freshness of geopositions. 

In the case where there is a web page watching the geolocation position, the
geolocation system will spin up and keep the underlying system-specific provider
and ask for updates whenever the position changes. When a second geolocation
request comes in, it will yield whatever position it last received from the
underlying system. 

One reason that this isn't fresh is that the underlying system (in our case
network based geolocation) relies on the wifi data provider's polling strategy
to prompt new lookups. Since the wifi data isn't changing (as there is no wifi
on your system), we're only getting the initial geo-ip lookup.

There's definitely some room for improvement here. For example, when a request
demands a fresh position, we could poke the underlying location providers to
provide their freshest positions. This is a more complex change, however.

[John Knottenbelt, 2012-11-30 16:24:16]

This patch adds the test.

[bulach, 2012-11-30 16:49:56]

lgtm, thanks! I have one suggestion for the test, please let me know your
thoughts:

https://codereview.chromium.org/11312210/diff/20001/content/browser/geolocati...
File content/browser/geolocation/geolocation_provider_unittest.cc (right):

https://codereview.chromium.org/11312210/diff/20001/content/browser/geolocati...
content/browser/geolocation/geolocation_provider_unittest.cc:220:
MessageLoop::current()->Run();
it looks like this test would work even without the patch? :)
I mean, it will _also_ work with the patch, but maybe we could:

208:
- add a second observer.
- post a quit message
- MessageLoop::current()->Run()
- ensure the second observer did NOT get any update.

then keep 215 / 223 as is?

[John Knottenbelt, 2012-11-30 17:25:19]

Thanks for reviewing, Marcus!

I've made the suggested change, but used RunUntilIdle() as I believe that it's
equivalent to posting a quit and then running the loop.

[John Knottenbelt, 2012-11-30 17:26:47]

Publish draft comment, suggested change made in patch set 4.

https://codereview.chromium.org/11312210/diff/20001/content/browser/geolocati...
File content/browser/geolocation/geolocation_provider_unittest.cc (right):

https://codereview.chromium.org/11312210/diff/20001/content/browser/geolocati...
content/browser/geolocation/geolocation_provider_unittest.cc:220:
MessageLoop::current()->Run();
Actually, the test does indeed fail without the patch, because the geolocaiton
provider sends through the stale position synchronously with the AddObserver
call, which causes the EXPECT_CALL to mismatch (wrong position sent through).

But yes, I think that your suggestion is good. I'll try and work it in.

On 2012/11/30 16:49:56, bulach wrote:
> it looks like this test would work even without the patch? :)
> I mean, it will _also_ work with the patch, but maybe we could:
> 
> 208:
> - add a second observer.
> - post a quit message
> - MessageLoop::current()->Run()
> - ensure the second observer did NOT get any update.
> 
> then keep 215 / 223 as is?

[commit-bot: I haz the power, 2012-12-03 17:06:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/11312210/22006

[commit-bot: I haz the power, 2012-12-03 18:14:04]

Retried try job too often on linux_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 18:47:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/11312210/22006

[commit-bot: I haz the power, 2012-12-03 20:54:57]

Change committed as 170806