Bug 25149 - Geolocation should start timer after permission is granted
Summary: Geolocation should start timer after permission is granted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Greg Bolsinga
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-04-12 12:54 PDT by Greg Bolsinga
Modified: 2009-04-13 12:06 PDT (History)
1 user (show)

See Also:


Attachments
implements the fix (8.56 KB, patch)
2009-04-12 13:12 PDT, Greg Bolsinga
no flags Details | Formatted Diff | Diff
updated patch to compile (9.05 KB, patch)
2009-04-12 13:38 PDT, Greg Bolsinga
sam: review-
Details | Formatted Diff | Diff
Addresses Sam's review comments. (9.10 KB, patch)
2009-04-12 16:28 PDT, Greg Bolsinga
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Bolsinga 2009-04-12 12:54:22 PDT
http://lists.w3.org/Archives/Public/public-geolocation/2009Mar/0068.html

In the current implementation, the timer starts right at the first request, and may fire before the user has granted permission.

<rdar://problem/6672275>
Comment 1 Greg Bolsinga 2009-04-12 13:12:22 PDT
Created attachment 29422 [details]
implements the fix
Comment 2 Greg Bolsinga 2009-04-12 13:38:23 PDT
Created attachment 29423 [details]
updated patch to compile

this compiles. I brought the change from iPhone, which is slightly different.
Comment 3 Sam Weinig 2009-04-12 14:22:55 PDT
Comment on attachment 29423 [details]
updated patch to compile

r-.  Some concerns below.

> +        * page/Geolocation.cpp: Add a RefPtr<PositionOptions> so it can be used
> +	later when the timer is started. Change PositionOptions* to PassRefPtr<PositionOptions>
> +	where needed.

Please remove the tabs.


> -void Geolocation::getCurrentPosition(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PositionOptions* options)
> +void Geolocation::getCurrentPosition(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PassRefPtr<PositionOptions> options)
>  {
>      RefPtr<GeoNotifier> notifier = GeoNotifier::create(successCallback, errorCallback, options);
>  
> -    if (!m_service->startUpdating(options)) {
> +    if (!m_service->startUpdating(options.get())) {

I am not sure it is safe to use the PRP options like this after passing it to GeoNotifier::create.  Instead, you should use notifier->m_options as is one for notifier->m_errorCallback.

>          if (notifier->m_errorCallback) {

> +int Geolocation::watchPosition(PassRefPtr<PositionCallback> successCallback, PassRefPtr<PositionErrorCallback> errorCallback, PassRefPtr<PositionOptions> options)
>  {
>      RefPtr<GeoNotifier> notifier = GeoNotifier::create(successCallback, errorCallback, options);
>  
> -    if (!m_service->startUpdating(options)) {
> +    if (!m_service->startUpdating(options.get())) {

Same here.

> +void Geolocation::startTimer(Vector<RefPtr<GeoNotifier> >& notifiers)

Can this function be a static function instead of a member function?

> +
> +void Geolocation::startTimersForOneShots()
> +{
> +    Vector<RefPtr<GeoNotifier> > copy;
> +    copyToVector(m_oneShots, copy);

If starting the timer cannot alter m_oneShot, I don't think there is any reason to copy the set into a vector. 

> +void Geolocation::startTimersForWatchers()
> +{
> +    Vector<RefPtr<GeoNotifier> > copy;
> +    copyValuesToVector(m_watchers, copy);

Same comment.
Comment 4 Greg Bolsinga 2009-04-12 15:42:19 PDT
(In reply to comment #3)
> (From update of attachment 29423 [details] [review])
> r-.  Some concerns below.

> > +
> > +void Geolocation::startTimersForOneShots()
> > +{
> > +    Vector<RefPtr<GeoNotifier> > copy;
> > +    copyToVector(m_oneShots, copy);
> 
> If starting the timer cannot alter m_oneShot, I don't think there is any reason
> to copy the set into a vector. 
> 
> > +void Geolocation::startTimersForWatchers()
> > +{
> > +    Vector<RefPtr<GeoNotifier> > copy;
> > +    copyValuesToVector(m_watchers, copy);
> 
> Same comment.
> 

Each of these are not Vectors, and this allows me to have a common startTimers call between the two.

I'm addressing your other comments, and will have a new patch shortly.
Comment 5 Greg Bolsinga 2009-04-12 16:28:26 PDT
Created attachment 29425 [details]
Addresses Sam's review comments.

It still gets the Vector for the oneShots and watchers when it starts the timers. This is so there is a common method between the two. One is a HashMap, the other a HashSet.
Comment 6 Sam Weinig 2009-04-13 10:28:38 PDT
Comment on attachment 29425 [details]
Addresses Sam's review comments.

>      
> +    static void startTimer(Vector<RefPtr<GeoNotifier> >&);

This doesn't need to be in the header.  You can just declare it static in the cpp file.

Otherwise, r=me.
Comment 7 Greg Bolsinga 2009-04-13 12:03:57 PDT
startTimer uses GeoNotifier, which is private to Geolocation. If I remove the declaration from the class, I get errors when using GeoNotifier.
Comment 8 Greg Bolsinga 2009-04-13 12:06:13 PDT
bolsinga:WebKit bolsinga$ svn commit
Sending        WebCore/ChangeLog
Sending        WebCore/bindings/js/JSGeolocationCustom.cpp
Sending        WebCore/page/Geolocation.cpp
Sending        WebCore/page/Geolocation.h
Transmitting file data ....
Committed revision 42445.