[webkit-reviews] review canceled: [Bug 83879] [GTK][WK2] Implement API for Geolocation permission requests in the GTK port : [Attachment 137310] Patch proposal

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has canceled Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 83879: [GTK][WK2] Implement API for Geolocation permission requests in the
GTK port
https://bugs.webkit.org/show_bug.cgi?id=83879

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

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Attaching a new patch, this time with the new unit test needed to check this
new API for permission requests.

Some comments below...

(In reply to comment #15)
> [...]
> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:25
> > +#include "WebKitPrivate.h"
> 
> This is already included by WebKitGeolocationPermissionRequestPrivate.h
> 

Fixed

> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:52
> > +	 WKGeolocationPermissionRequestAllow(priv->request.get());
> 
> I wonder whether it's valid to call this twice, maybe we should return early
if madeDecission is true
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:61
> > +	 WKGeolocationPermissionRequestDeny(priv->request.get());
> 
> Ditto.

I think you are right: it is not possible (and doesn't make much sense either)
to deny a request after allowing it, or viceversa.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:75
> > +	 request->priv->madeDecision = false;
> 
> Glib already initialize to 0 the instance struct when it's allocated, so this
is already false.
> 

Fixed.

> > Tools/MiniBrowser/gtk/BrowserWindow.c:298
> > +	 GtkWidget *dialog = gtk_dialog_new_with_buttons("Geolocation request",

> > +							 GTK_WINDOW(window),
> > +							 GTK_DIALOG_MODAL |
GTK_DIALOG_DESTROY_WITH_PARENT,
> > +							 GTK_STOCK_OK,
> > +							 GTK_RESPONSE_ACCEPT,
> > +							 GTK_STOCK_CANCEL,
> > +							 GTK_RESPONSE_REJECT,
> > +							 NULL);
> > +
> > +	 GtkWidget *contentArea =
gtk_dialog_get_content_area(GTK_DIALOG(dialog));
> > +	 GtkWidget *label = gtk_label_new("Allow geolocation request?");
> > +	 gtk_box_pack_start(GTK_BOX(contentArea), label, TRUE, TRUE, 6);
> > +	 gtk_widget_show(label);
> 
> This would probably be easier using gtk_message_dialog.

Agreed. Fixed