[webkit-reviews] review granted: [Bug 34962] Geolocation object requires a means to cancel a permission request : [Attachment 48800] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Steve Block <steveblock at google.com>'s
request for review:
Bug 34962: Geolocation object requires a means to cancel a permission request
https://bugs.webkit.org/show_bug.cgi?id=34962

Attachment 48800: Patch
https://bugs.webkit.org/attachment.cgi?id=48800&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   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.