[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


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #137310|0                           |1
        is obsolete|                            |
 Attachment #137310|review?                     |
               Flag|                            |
 Attachment #138117|                            |review?
               Flag|                            |




--- Comment #16 from Mario Sanchez Prada <msanchez at igalia.com>  2012-04-20 10:38:18 PST ---
Created an attachment (id=138117)
 --> (https://bugs.webkit.org/attachment.cgi?id=138117&action=review)
Patch proposal + Unit test

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

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