Bug 27256 – Geolocation timeout parameter is not correctly applied

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

Currently, the Geolocation timeout specified by PositionOptions.timeout is started only when permissions are granted in Geolocation::setIsAllowed(). This means that when a call to getCurrentPosition() or watchPosition() is made ...
- If permissions have already been granted, the timeout is not applied
- If permissions have not yet been granted, the timeout is started after a location has been acquired (and permissions granted).

The intent of the PositionOptions.timeout parameter is to limit the time taken for the implementation to obtain a location. See http://www.w3.org/TR/geolocation-API/#position-options. This means that the timeout should be started as soon as the implementation starts the process of obtaining the position fix.

Note that the timeout should not include any time taken to obtain permissions from the user.  It is this fact that allows the parameter to offer functionality that can not be provided with a JavaScript timer.

[Steve Block]

Created attachment 34970 [details]
Patch 1 for bug 27256

Correctly implements Geolocation timeout parameter.

Greg, you mentioned that on iPhone, the call to GeolocationService::startUpdating may trigger UI to obtain system permissions for location. Hence it's impossible to apply the timeout parameter in a meaningful way through the current GeolocationService interface. Currently, the timeout parameter is not applied at all, so perhaps a reasonable solution is to ignore timeouts on iPhone by skipping the call to maybeStartTimer? Or we could skip the call unless we know that system permissions have already been granted? What do you think?

[Greg Bolsinga]

There's going to be lots of iPhone work to get the recent GeoLocation patches back to iPhone. I'm not sure when I'd be able to get to it to work, nor have I been able to fully track all of the recent changes made. Basically I trust your judgement.

[Steve Block]

Created attachment 38373 [details]
Patch 2 for bug 27256

> Why are tests impossible?  (I'm not suggesting they aren't, I just need more
> info as to 'why?')
Writing test for this (and all of the other Geolocation features) requires a mock Geolocation service so that the behavior of the API is predictable for testing. This is being tracked in Bug 28264 (which also needs review!).

> Is it possible that the handlers could do nasty things?
> like start a new request or something?  Anything that could potentially crash
> us when we return back to executing c++ after the JS callback?
Yes, a handler could start a new request, but this shouldn't be a problem. I'll add tests for this once we have a mock Geolocation service.

> We only name arguments when they add clarity:
Fixed.

> Are
> there other webkit reviewers who know something about geolocation?
Not that I knew of. Greg Bolsinga wrote the original code, but he's not a reviewer.

> Who uses
> this code?  Andriod?  Does iPhone use this or do they have their own thing?
Yes, this is used by Android, iPhone and Chrome.

[Greg Bolsinga]

CC'ing Sam. He was the original Geolocation reviewer.

[Adam Barth]

Comment on attachment 38373 [details]
Patch 2 for bug 27256

We need to be able to write test (e.g., fix Bug 28264) before we can move forward with this patch.

[Steve Block]

Created attachment 39262 [details]
Patch 3 for bug 27256

Adds LayoutTests.

Note that refactoring to avoid the duplication in getcurrentPosition/watchPosition is included as part of the fix for Bug 27944.

[Eric Seidel (no email)]

Did you modify this patch by hand?  PrettyPatch has trouble parsing it:
https://bugs.webkit.org/attachment.cgi?id=39262&action=prettypatch
Note the Geolocation.cpp header is missing.

[Steve Block]

Created attachment 39294 [details]
Patch 4 for bug 27256

> Did you modify this patch by hand?
Yes - I have to manually patch in and out a couple of changes that let me build and run the Geolocation tests on mac.

> Note the Geolocation.cpp header is missing.
Fixed.

[Adam Barth]

Comment on attachment 39294 [details]
Patch 4 for bug 27256

I might be slightly over-stepping my knowledge, but no one else seems to be reviewing geolocation patches.  If you'd like me to be more conservative in this regard, please let me know.

As far as I can tell, this patch looks correct.

[WebKit Commit Bot]

Comment on attachment 39294 [details]
Patch 4 for bug 27256

Rejecting patch 39294 from commit-queue.

This patch will require manual commit. Patch https://bugs.webkit.org/attachment.cgi?id=39294 from bug 27256 failed to download and apply.

[Ben Murdoch]

Landed as r48249.