Bug 34962 – Geolocation object requires a means to cancel a permission request

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

When a Geolocation object needs to obtain user permission to release the user's location to script, it makes a call to the chrome client. This call may be synchronous or asynchronous. If the request is asynchronous, but the Geolocation object later wishes to cancel it (such as when the Geolocation object is destroyed) there is no way for the chrome client to be notified.

We need to add a chrome client method to cancel an ongoing asynchronous Geolocation permission request. The chome client may wish, for example, to remove any UI currently being shown to the user to obtain permission.

[Steve Block]

Created attachment 48800 [details]
Patch

[Darin Adler]

Comment on attachment 48800 [details]
Patch

> +        No new tests, as this is for the purposes of browser UI only.

You're not hooking this up in WebKit, and doing the work only in WebCore.

That seems strange; maybe OK for Chromium, but not right for other platforms except the ones that aren't doing Geolocation at all.

I think this could be hooked up to testing machinery if you figured out what the WebKit API was for this on various platforms.

>          // This can be either a synchronous or asynchronous call. The ChromeClient can display UI asking the user for permission
> -        // to use Geolococation. The ChromeClient must call Geolocation::setShouldClearCache() appropriately.
> +        // to use Geolococation.

I think we should use the traditional "Geolocation" rather than "Geolococation" ;-)

>          virtual void requestGeolocationPermissionForFrame(Frame*, Geolocation*) = 0;
> +        virtual void cancelGeolocationPermissionRequestForFrame(Frame*) { }

Making this an empty function rather than pure virtual makes it easier to keep all the platforms compiling, but hides the work needed for platforms that currently have requestGeolocationPermissionForFrame functions defined. Why did we make the function above this pure virtual but not this one?

I'll say r=me, but this seems like a partial change that is not good for the WebKit/mac and WebKit/win implementations.

[Steve Block]

(In reply to comment #2)
> You're not hooking this up in WebKit, and doing the work only in WebCore.
>
> That seems strange; maybe OK for Chromium, but not right for other platforms
> except the ones that aren't doing Geolocation at all.
This patch was motivated by the needs of Android. WebKit/android isn't yet upstreamed, which is why there's no WebKit implementation.

> I think we should use the traditional "Geolocation" rather than "Geolococation"
Will fix

> Making this an empty function rather than pure virtual makes it easier to keep
> all the platforms compiling, but hides the work needed for platforms that
> currently have requestGeolocationPermissionForFrame functions defined. Why did
> we make the function above this pure virtual but not this one?
All platforms supporting Geolocation will need to implement requestGeolocationPermissionForFrame. However, I can imagine a situation where a platform may not need to know when an individual Geolocation object is canceling a request - for example, if permissions UI is only cleared when the page is navigated away. That's why cancelGeolocationPermissionRequestForFrame was empty.

However, I've just spoken to joth (cc'ed) who's doing Chromium Geolocation and they're likely to use such a callback too. So I'll change to a pure virtual method in ChromeClient. Note that even for Chromium the implementation will currently be empty as Chromium Geolocation is not yet complete.

[Steve Block]

Created attachment 48885 [details]
Patch

[Jonathan Dixon]

(In reply to comment #3)
> However, I've just spoken to joth (cc'ed) who's doing Chromium Geolocation and
> they're likely to use such a callback too. So I'll change to a pure virtual
> method in ChromeClient. Note that even for Chromium the implementation will
> currently be empty as Chromium Geolocation is not yet complete.

Quick update on this: the chromium implementation is being committed as we
speak https://bugs.webkit.org/show_bug.cgi?id=32068) so there maybe a merge
conflict when you come to land this one, sorry about that.
Thanks for adding the placeholder method for us.

[Steve Block]

Created attachment 48890 [details]
Patch

[Steve Block]

Darin, do you want to take a look at my updated patch, or are you happy for me to submit?

[Steve Block]

Landed manually as http://trac.webkit.org/changeset/55136

Closing bug as resolved fixed.