[Webkit-unassigned] [Bug 83879] [GTK][WK2] Implement API for Geolocation permission requests in the GTK port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=83879





--- Comment #11 from Mario Sanchez Prada <msanchez at igalia.com>  2012-04-16 01:48:42 PST ---
(From update of attachment 137092)
View in context: https://bugs.webkit.org/attachment.cgi?id=137092&action=review

Thanks for the review. Will be addressing those issues in a follow-up patch

>> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:62
>> +        iface->allow(request);
> 
> I guess this should be a required method, so we should enforce to be implemented, no?

I just followed the pattern I could see in other places in WebCore:
  - WebCore/bindings/gobject/WebKitDOMEventTarget.cpp 
  - WebKit/gtk/webkit/webkitspellchecker.cpp 

After all, it's an interface, you will be either completely implementing it or not, but not doing things half-way :-)

>> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:77
>> +        iface->deny(request);
> 
> Ditto.

Ditto too.

>> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.h:36
>> +#define WEBKIT_PERMISSION_REQUEST_GET_IFACE(obj) (G_TYPE_INSTANCE_GET_INTERFACE((obj), WEBKIT_TYPE_PERMISSION_REQUEST, WebKitPermissionRequestIface))
> 
> Don't add CLASS macros for interfaces, just WEBKIT_TYPE_PERMISSION_REQUEST, WEBKIT_PERMISSION_REQUEST, WEBKIT_IS_PERMISSION_REQUEST and WEBKIT_PERMISSION_REQUEST_GET_IFACE

Ok (we should fix WebCore/bindings/gobject/WebKitDOMEventTarget.h too)

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:323
>> +    webViewClass->permission_request = webkitWebViewDecidePermissionRequest;
> 
> We use the same name as the signal, so remove 'Decide'.

Ok

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:652
>> +     * @request_type: a #WebKitPermissionRequestType denoting the type of @request
> 
> WebKitPermissionRequest is an interface, so request is an instance of an object that implements the interface. We usually use FOOT_IS_BAR macros to know the concrete type, so we don't need an enum with all types. I'm not opposed to use an enum though, if you think it's clearer and consistent with policy client API.

Just left it because my patch clearly came from when I was thinking of integrating things with the policy decision framework

But I think you're right and that FOO_IS_BAR macro should be more than enough, and it makes the patches cleaner.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:660
>> +     * returning %TRUE to block the default signal handler.
> 
> I think this is something that depends on the concrete request object, unless we make clear that implementations of WebKitPermissionRequest should allow it.

For the case of geolocation requests it's true, but perhaps we should remove this paragraph at this early stage, before clearly knowing whether we should ask implementors of WebKitPermissionRequest for it or not.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:665
>> +     * simply call webkit_permission_request_deny().
> 
> Ditto, this will be implemented in the finalize of every request implementation.

Ok.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.