Bug 34962 - Geolocation object requires a means to cancel a permission request
: Geolocation object requires a means to cancel a permission request
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 35031 35809
  Show dependency treegraph
 
Reported: 2010-02-15 15:03 PST by
Modified: 2010-03-30 04:01 PST (History)


Attachments
Patch (3.78 KB, patch)
2010-02-16 03:08 PST, Steve Block
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.51 KB, patch)
2010-02-17 03:48 PST, Steve Block
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.57 KB, patch)
2010-02-17 04:21 PST, Steve Block
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-02-15 15:03:07 PST
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.
------- Comment #1 From 2010-02-16 03:08:38 PST -------
Created an attachment (id=48800) [details]
Patch
------- Comment #2 From 2010-02-16 13:43:48 PST -------
(From update of attachment 48800 [details])
> +        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.
------- Comment #3 From 2010-02-17 03:30:02 PST -------
(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.
------- Comment #4 From 2010-02-17 03:48:26 PST -------
Created an attachment (id=48885) [details]
Patch
------- Comment #5 From 2010-02-17 03:53:24 PST -------
(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.
------- Comment #6 From 2010-02-17 04:21:02 PST -------
Created an attachment (id=48890) [details]
Patch
------- Comment #7 From 2010-02-22 16:52:09 PST -------
Darin, do you want to take a look at my updated patch, or are you happy for me to submit?
------- Comment #8 From 2010-02-23 02:36:19 PST -------
Landed manually as http://trac.webkit.org/changeset/55136

Closing bug as resolved fixed.