Bug 27256 - Geolocation timeout parameter is not correctly applied
Summary: Geolocation timeout parameter is not correctly applied
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on: 28264 29027
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-14 05:08 PDT by Steve Block
Modified: 2009-09-10 04:33 PDT (History)
7 users (show)

See Also:


Attachments
Patch 1 for bug 27256 (11.04 KB, patch)
2009-08-17 08:55 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch 2 for bug 27256 (11.06 KB, patch)
2009-08-21 08:38 PDT, Steve Block
abarth: review-
Details | Formatted Diff | Diff
Patch 3 for bug 27256 (18.32 KB, patch)
2009-09-09 04:25 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch 4 for bug 27256 (18.29 KB, patch)
2009-09-09 12:45 PDT, Steve Block
abarth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2009-07-14 05:08:46 PDT
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.
Comment 1 Steve Block 2009-08-17 08:55:26 PDT
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?
Comment 2 Greg Bolsinga 2009-08-17 10:26:23 PDT
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.
Comment 3 Steve Block 2009-08-21 08:38:45 PDT
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.
Comment 4 Greg Bolsinga 2009-08-21 14:05:52 PDT
CC'ing Sam. He was the original Geolocation reviewer.
Comment 5 Adam Barth 2009-09-01 16:01:45 PDT
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.
Comment 6 Steve Block 2009-09-09 04:25:25 PDT
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.
Comment 7 Eric Seidel (no email) 2009-09-09 11:29:17 PDT
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.
Comment 8 Steve Block 2009-09-09 12:45:06 PDT
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.
Comment 9 Adam Barth 2009-09-09 22:01:10 PDT
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.
Comment 10 WebKit Commit Bot 2009-09-09 22:25:30 PDT
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.
Comment 11 Ben Murdoch 2009-09-10 04:33:29 PDT
Landed as r48249.