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

[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 144313: Patch
https://bugs.webkit.org/attachment.cgi?id=144313&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Attaching new patch addressing the issues raised here.

(In reply to comment #18)
> (From update of attachment 144313 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=144313&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:42
> > +	 WKRetainPtr<WKGeolocationPermissionRequestRef> request;
> 
> We usually use wk prefix for C API variables, in this case wkRequest

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:68
> > +	 // Only one decision at a time.
> > +	 if (priv->madeDecision)
> > +	     return;
> 
> This actually means that the request can only be used once, no?

Yes. I don't think it makes sense to allow making more than one decision _per
request_.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:105

> > +WebKitGeolocationPermissionRequest*
webkitGeolocationPermissionRequestCreate(WKGeolocationPermissionRequestRef
request)
> 
> request -> wkRequest

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:148
> > +static void decidePolicyForGeolocationPermissionRequest(WKPageRef,
WKFrameRef, WKSecurityOriginRef, WKGeolocationPermissionRequestRef request,
const void* clientInfo)
> 
> request -> wkRequest

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:482
> > +	 // Test denying a permission request.
> > +	 test->m_allowPermissionRequests = false;
> > +	 test->loadHtml(geolocationRequestHTML, 0);
> > +	 test->waitUntilMainLoopFinishes();
> > +
> > +	 // Test allowing a permission request.
> > +	 test->m_allowPermissionRequests = true;
> > +	 test->loadHtml(geolocationRequestHTML, 0);
> > +	 test->waitUntilMainLoopFinishes();
> 
> These only check whether the signal is emitted or not, but I wonder if it's
possible to 
> check also that the request was actually allowed/denied

I've implemented a better test in the current patch, by putting checking the
actual result after calling to getCurrentPosition() both after denying and
allowing the request. However, in order to work fine it needs the patch for bug
83877 in place, so that's why I'll be marking this bug as dependent on that
other one right after uploading this new patch

> > Tools/MiniBrowser/gtk/BrowserWindow.c:216
> > +static void geolocationRequestDialogCallback(GtkDialog* dialog, gint
response, WebKitPermissionRequest* request)
> 
> The * is incorrectly placed here

Fixed.