Bug 84018 - [GTK][WK2] Implement API for generic permission requests
: [GTK][WK2] Implement API for generic permission requests
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
: Gtk
Depends on:
Blocks: 83879
  Show dependency treegraph
 
Reported: 2012-04-16 03:37 PDT by Mario Sanchez Prada
Modified: 2012-05-25 06:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch Proposal (19.72 KB, patch)
2012-04-16 03:44 PDT, Mario Sanchez Prada
pnormand: commit‑queue-
Details | Formatted Diff | Diff
Patch Proposal (19.53 KB, patch)
2012-04-16 04:05 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch Proposal (20.06 KB, patch)
2012-04-20 10:33 PDT, Mario Sanchez Prada
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2012-04-16 03:37:55 PDT
This will be useful for implementing things like geolocation or fullscreen permission requests
Comment 1 Mario Sanchez Prada 2012-04-16 03:44:39 PDT
Created attachment 137309 [details]
Patch Proposal
Comment 2 WebKit Review Bot 2012-04-16 03:47:23 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Philippe Normand 2012-04-16 03:50:01 PDT
Comment on attachment 137309 [details]
Patch Proposal

Attachment 137309 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12416004
Comment 4 Mario Sanchez Prada 2012-04-16 04:05:30 PDT
Created attachment 137312 [details]
Patch Proposal
Comment 5 Carlos Garcia Campos 2012-04-17 00:34:29 PDT
Comment on attachment 137312 [details]
Patch Proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=137312&action=review

Looks good.

> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:29
> + * There are situations where a web browser would need to ask the user

Not all embedders are web browsers

> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:33
> + * #WebKitWebVice::permission-request signal with a

WebKitWebVice? :-P

> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:40
> + * If the signal handler does nothing, WebKit will act as if
> + * webkit_permission_request_deny() was called as soon as signal
> + * handling completes. To handle a permission request in an
> + * asynchronous way, simply increment the reference count of the
> + * #WebKitPermissionRequest object.

This depends on the implementation, or it should be enforced by the interface.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:656
> +     *

You should mention here that if this signal is not hanlded, the default behaviour is to deny the request

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:661
> +     *

A code snippet here would help to understand how to use this signal

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:146
> +    gboolean   (* permission_request)    (WebKitWebView              *web_view,
> +                                          WebKitPermissionRequest    *permission_request);

There's an extra space before parameter names.
Comment 6 Mario Sanchez Prada 2012-04-20 10:33:02 PDT
Created attachment 138114 [details]
Patch Proposal

Thanks for the review. Attached new patch addressing all the issues.

See some comments below...

(In reply to comment #5)
> [...]
> Not all embedders are web browsers
>
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:33
> > + * #WebKitWebVice::permission-request signal with a
> 
> WebKitWebVice? :-P

You have to admit it was a great name to have in the API, a bit useless though.

Fixed.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:40
> > + * If the signal handler does nothing, WebKit will act as if
> > + * webkit_permission_request_deny() was called as soon as signal
> > + * handling completes. To handle a permission request in an
> > + * asynchronous way, simply increment the reference count of the
> > + * #WebKitPermissionRequest object.
> 
> This depends on the implementation, or it should be enforced by the interface.

Fixed (just removed)

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:656
> > +     *
> 
> You should mention here that if this signal is not hanlded, the default behaviour is to deny the request
>
Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:661
> > +     *
> 
> A code snippet here would help to understand how to use this signal
>

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:146
> > +    gboolean   (* permission_request)    (WebKitWebView              *web_view,
> > +                                          WebKitPermissionRequest    *permission_request);
> 
> There's an extra space before parameter names.

Fixed.
Comment 7 Martin Robinson 2012-04-26 09:01:59 PDT
Comment on attachment 138114 [details]
Patch Proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=138114&action=review

Looks good. In the future, some requests show allow asynchronous handling. Some probably won't allow that. The documentation in WebKitWebView should explain the situation and we should add some API to make this discoverable.

> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:30
> + * for permission on certain type of operations, such as switching to

"to ask the user for permission on certain type" -> "to ask the user for permission to do certain types"

> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:31
> + * full screen mode or reporting the user's location through the

Nit: you use "full screen" here but "fullscreen" below.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:198
> +static gboolean webkitWebViewPermissionRequest(WebKitWebView*, WebKitPermissionRequest* decision)
> +{
> +    webkit_permission_request_deny(decision);
> +    return TRUE;
> +}

Installing a default handler like this means that the request cannot be answered asynchronously. Perhaps it's better to do this in the finalize method for the concrete types.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:655
> +     * decide about a permission request, such as allowing the browser
> +     * switch to fullscreen mode, sharing its location or similar.

"allowing the browser to switch to fullscreen"

"or similar" -> "or similar operations."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:658
> +     * A possible way to use this signal could be through a dialog to
> +     * allow the user decide what to do with the request:

"through a dialog to allow" -> "through a dialog allowing"
Comment 8 Mario Sanchez Prada 2012-05-25 06:45:44 PDT
Committed r118522: <http://trac.webkit.org/changeset/118522>