[webkit-reviews] review denied: [Bug 110397] Allow to override previously declined or accepted Geolocation permission : [Attachment 192934] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Dominic Battre
<battre at chromium.org>'s request for review:
Bug 110397: Allow to override previously declined or accepted Geolocation
permission
https://bugs.webkit.org/show_bug.cgi?id=110397

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=192934&action=review


We'll also need some tests to show how this API interacts with the geolocation
permission state machine.

>>>> Source/WebKit/chromium/public/WebGeolocationController.h:47
>>>> +	  WEBKIT_EXPORT static void setIsAllowed(WebFrame*, bool allowed);
>>> 
>>> nit. we usually prefer enums over bools
>> 
>> Do you also prefer this if it is inconsistent with the other setIsAllowed
functions (which take only one parameter, though)? See
https://code.google.com/p/chromium/codesearch#search/&q=file:Geolocation%20file
:%5C.h$%20setIsAllowed&sq=package:chromium&type=cs
> 
> For a function like setIsAllowed, it's pretty obvious what the bool means,
which means an enum isn't really needed for readability.

I guess what I don't understand about this patch is why this function is both
static and takes a WebFrame as a context object.  Instead, can't we make this a
non-static function of an appropriate object and skip the context argument?