Bug 25149 – Geolocation should start timer after permission is granted

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Greg Bolsinga]

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>

[Greg Bolsinga]

Created attachment 29422 [details]
implements the fix

[Greg Bolsinga]

Created attachment 29423 [details]
updated patch to compile

this compiles. I brought the change from iPhone, which is slightly different.

[Sam Weinig]

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.

[Greg Bolsinga]

(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.

[Greg Bolsinga]

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.

[Sam Weinig]

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.

[Greg Bolsinga]

startTimer uses GeoNotifier, which is private to Geolocation. If I remove the declaration from the class, I get errors when using GeoNotifier.

[Greg Bolsinga]

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.