Bug 83879 – [GTK][WK2] Implement API for Geolocation permission requests in the GTK port

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

Implement a 'permission request' API in WebKit2GTK+ for the enabling user to allow or deny certain requests, and provide an specific implementation for Geolocation permission requests.

The 'permission request' API should be usable for more situations in the future (e.g. requests to switch to full screen mode)

[Mario Sanchez Prada]

Created attachment 137081 [details]
1. Add new 'permission-request' signal to WebKitWebView

 Added new 'permission-request' signal to WebKitWebView, to be fired when WebKit needs confirmation from the user on whether to allow or deny certain operations, such as sharing the user's location with web site through the Geolocation API.

Also, provided a new WebKitPermissionRequest interface, providing allow() and deny() operations, to be called over the objects implementing it when emitted along with the new 'permission-request' signal.

Perhaps I should move the later to a different bug. Not 100% sure since it's tighly related to this one and thus to the next patch I'll be attaching in 2 minutes...

[Mario Sanchez Prada]

Created attachment 137082 [details]
2. Added a new geolocation permission request

Added a new kind of permission request for supporting the Geolocation API in WebKit2GTK+.

Added a new WebKitGeolocationPermissionRequest class, implementing the WebKitPermissionRequest interface, to enabling client applications  to allow or deny geolocation permission requests.

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

Comment on attachment 137081 [details]
1. Add new 'permission-request' signal to WebKitWebView

Attachment 137081 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12393864

[Mario Sanchez Prada]

Created attachment 137088 [details]
1. Add new 'permission-request' signal to WebKitWebView

Stupid me, looks like I screwed the patch while checking it locally.

Here's the new one.

[Gustavo Noronha (kov)]

Comment on attachment 137088 [details]
1. Add new 'permission-request' signal to WebKitWebView

Attachment 137088 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12399340

[Mario Sanchez Prada]

Created attachment 137092 [details]
1. Add new 'permission-request' signal to WebKitWebView

Strike 3!

[Gustavo Noronha (kov)]

Comment on attachment 137092 [details]
1. Add new 'permission-request' signal to WebKitWebView

Attachment 137092 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12403146

[Carlos Garcia Campos]

Comment on attachment 137092 [details]
1. Add new 'permission-request' signal to WebKitWebView

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

> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:62
> +    WebKitPermissionRequestIface* iface = WEBKIT_PERMISSION_REQUEST_GET_IFACE(request);
> +    if (iface->allow)
> +        iface->allow(request);

I guess this should be a required method, so we should enforce to be implemented, no?

> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.cpp:77
> +    WebKitPermissionRequestIface* iface = WEBKIT_PERMISSION_REQUEST_GET_IFACE(request);
> +    if (iface->deny)
> +        iface->deny(request);

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitPermissionRequest.h:36
> +#define WEBKIT_TYPE_PERMISSION_REQUEST           (webkit_permission_request_get_type())
> +#define WEBKIT_PERMISSION_REQUEST(obj)           (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_PERMISSION_REQUEST, WebKitPermissionRequest))
> +#define WEBKIT_PERMISSION_REQUEST_CLASS(obj)     (G_TYPE_CHECK_CLASS_CAST ((obj), WEBKIT_TYPE_PERMISSION_REQUEST, WebKitPermissionRequestIface))
> +#define WEBKIT_IS_PERMISSION_REQUEST(obj)        (G_TYPE_CHECK_INSTANCE_TYPE((obj), WEBKIT_TYPE_PERMISSION_REQUEST))
> +#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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:323
> +    webViewClass->permission_request = webkitWebViewDecidePermissionRequest;

We use the same name as the signal, so remove 'Decide'.

> 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.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:660
> +     * It is possible to handle permission requests asynchronously, by
> +     * simply calling g_object_ref() on the @request argument and
> +     * 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.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:665
> +     * If the last reference is removed on a #WebKitPermissionRequest
> +     * and no decision has been made, webkit_permission_request_deny()
> +     * will be the default decision. The default signal handler will
> +     * simply call webkit_permission_request_deny().

Ditto, this will be implemented in the finalize of every request implementation.

[Carlos Garcia Campos]

Comment on attachment 137082 [details]
2. Added a new geolocation permission request

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

Please, merge both patches or file separate bug reports.

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:63
> +static void
> +webkit_permission_request_interface_init(WebKitPermissionRequestIface *iface)

This should be a single line and the * is placed wrongly.

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:78
> +    WEBKIT_GEOLOCATION_PERMISSION_REQUEST(object)->priv->~WebKitGeolocationPermissionRequestPrivate();
> +    G_OBJECT_CLASS(webkit_geolocation_permission_request_parent_class)->finalize(object);

You should check here whether request has been made or not, and call deny if it wasn't made. That's what WebKitWebView::permission-request signal documentation says.

> Tools/ChangeLog:9
> +        Make minibrowser connect to the new 'permission requests' signal
> +        to allow users handle the Geolocation permission requests.

Please add unit tests, and maybe leave minibrowser impl for a following patch.

> Tools/MiniBrowser/gtk/BrowserWindow.c:293
> +    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);

What do you think about using GtkInfoBar instead of a popup dialog?

[Mario Sanchez Prada]

Comment on attachment 137092 [details]
1. Add new 'permission-request' signal to WebKitWebView

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.

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 137092 [details])
> 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 :-)

Exactly, so it doesn't make sense to create a request object that doesn't provide implementation for allow and deny, no?

> >> 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)

No, that's already public API, so it can be removed. It's harmless in any case.

[Mario Sanchez Prada]

Comment on attachment 137082 [details]
2. Added a new geolocation permission request

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:78
>> +    G_OBJECT_CLASS(webkit_geolocation_permission_request_parent_class)->finalize(object);
> 
> You should check here whether request has been made or not, and call deny if it wasn't made. That's what WebKitWebView::permission-request signal documentation says.

True.

>> Tools/ChangeLog:9
>> +        to allow users handle the Geolocation permission requests.
> 
> Please add unit tests, and maybe leave minibrowser impl for a following patch.

Will add later, of course. This is just a preliminar version of the patch.

>> Tools/MiniBrowser/gtk/BrowserWindow.c:293
>> +                                                    NULL);
> 
> What do you think about using GtkInfoBar instead of a popup dialog?

I think that for a test app like MiniBrowser a GtkDialog is more than enough, and simpler to implement.

If you strongly think we should use an info bar I will change it, though.

[Mario Sanchez Prada]

Created attachment 137310 [details]
Patch proposal

Attaching a new patch addressing the issues pointed out by Carlos, with the exception of unit tests, that will be added later.

(In reply to comment #10)
> (From update of attachment 137082 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137082&action=review
> 
> Please, merge both patches or file separate bug reports.

Filed a new bug for the part of adding a generic API for permission requests: bug 84018

[Carlos Garcia Campos]

Comment on attachment 137310 [details]
Patch proposal

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

Looks great!

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:25
> +#include "WebKitPrivate.h"

This is already included by WebKitGeolocationPermissionRequestPrivate.h

> 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.

> 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.

> 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.

[Mario Sanchez Prada]

Created attachment 138117 [details]
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

[Mario Sanchez Prada]

Created attachment 144313 [details]
Patch

New updated patch, as the old one didn't apply anymore

[Carlos Garcia Campos]

Comment on attachment 144313 [details]
Patch

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

> 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?

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:105
> +WebKitGeolocationPermissionRequest* webkitGeolocationPermissionRequestCreate(WKGeolocationPermissionRequestRef request)

request -> wkRequest

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

request -> wkRequest

> 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

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

The * is incorrectly placed here

[Mario Sanchez Prada]

Created attachment 144930 [details]
Patch proposal + Unit test

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.

[Mario Sanchez Prada]

We need to have bug 83877 fixed in order to properly implement unit tests here, so setting the dependency.

[Mario Sanchez Prada]

Created attachment 145058 [details]
Patch proposal + Unit test

New patch fixing a mess with the makefile (some files that should have been added here were not)

[Carlos Garcia Campos]

Comment on attachment 145058 [details]
Patch proposal + Unit test

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

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:48
> +    g_return_if_fail(WEBKIT_IS_GEOLOCATION_PERMISSION_REQUEST(request));

I think this is impossible to fail, so remove it or use an assert instead.

> Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:62
> +    g_return_if_fail(WEBKIT_IS_GEOLOCATION_PERMISSION_REQUEST(request));

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:488
> +    // According to the Geolocation API specification, '1' is the
> +    // error code returned for the PERMISSION_DENIED error.
> +    // http://dev.w3.org/geo/api/spec-source.html#position_error_interface
> +    g_assert_cmpstr(valueString.get(), ==, "1");

But it also says it's a numeric value, not, could you use WebViewTest::javascriptResultToNumber() and g_assert_cmpint()? You could a constant to give it a name.

[Mario Sanchez Prada]

Created attachment 145630 [details]
Patch proposal + Unit test

New patch attached. See my comments below...

(In reply to comment #22)
> (From update of attachment 145058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145058&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:48
> > +    g_return_if_fail(WEBKIT_IS_GEOLOCATION_PERMISSION_REQUEST(request));
> 
> I think this is impossible to fail, so remove it or use an assert instead.

True. Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitGeolocationPermissionRequest.cpp:62
> > +    g_return_if_fail(WEBKIT_IS_GEOLOCATION_PERMISSION_REQUEST(request));
> 
> Ditto.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:488
> > +    // According to the Geolocation API specification, '1' is the
> > +    // error code returned for the PERMISSION_DENIED error.
> > +    // http://dev.w3.org/geo/api/spec-source.html#position_error_interface
> > +    g_assert_cmpstr(valueString.get(), ==, "1");
> 
> But it also says it's a numeric value, not, could you use
> WebViewTest::javascriptResultToNumber() and g_assert_cmpint()?

Well, in this case I think it's easier to check the string "1" since I avoid doing the string -> double parsing in javascriptResultToNumber() and because, after all, I think we can agree on that "1" string represents a numeric value :-)

Not changing that yet.

[Carlos Garcia Campos]

(In reply to comment #23)

> > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:488
> > > +    // According to the Geolocation API specification, '1' is the
> > > +    // error code returned for the PERMISSION_DENIED error.
> > > +    // http://dev.w3.org/geo/api/spec-source.html#position_error_interface
> > > +    g_assert_cmpstr(valueString.get(), ==, "1");
> > 
> > But it also says it's a numeric value, not, could you use
> > WebViewTest::javascriptResultToNumber() and g_assert_cmpint()?
> 
> Well, in this case I think it's easier to check the string "1" since I avoid doing the string -> double parsing in javascriptResultToNumber() and because, after all, I think we can agree on that "1" string represents a numeric value :-)

If the value is a number it looks more natural to get the number from the js result (assuming the value is indeed a number, JSValueIsNumber() return true). I'm not proposing to convert the result string to a number, but getting the number from the js result, which is what javascriptResultToNumber() does, and it's definely easier than getting the string. 

> Not changing that yet.

If the result is a string (JSValueIsNumber() returns false), it's ok to me then

[Mario Sanchez Prada]

(In reply to comment #24)
> [...]
> > Not changing that yet.
> 
> If the result is a string (JSValueIsNumber() returns false), it's ok to me then

That's the case here:

 The following fails:
    JSGlobalContextRef context = webkit_javascript_result_get_global_context(javascriptResult);
    JSValueRef js_value = webkit_javascript_result_get_value(javascriptResult);
    g_assert(JSValueIsNumber(context, js_value));

 While this is OK:
    JSGlobalContextRef context = webkit_javascript_result_get_global_context(javascriptResult);
    JSValueRef js_value = webkit_javascript_result_get_value(javascriptResult);
    g_assert(JSValueIsString(context, js_value));

[Carlos Garcia Campos]

Comment on attachment 145630 [details]
Patch proposal + Unit test

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

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:483
> +        "<html>"
> +        "  <script>"
> +        "  function runTest()"
> +        "  {"
> +        "    navigator.geolocation.getCurrentPosition(function(p) { document.title = \"OK\" },"
> +        "                                             function(e) { document.title = e.code });"
> +        "  }"
> +        "  </script>"
> +        "  <body onload='runTest();'></body>"
> +        "</html>";

Ah, I see now, the value is a string because we are using the title.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:493
> +    static const char* checkResultScript = "document.title";
> +    WebKitJavascriptResult* javascriptResult = test->runJavaScriptAndWaitUntilFinished(checkResultScript, 0);
> +    g_assert(javascriptResult);
> +    GOwnPtr<char> valueString(WebViewTest::javascriptResultToCString(javascriptResult));

So, can't you simply use webkit_web_view_get_title() instead of running a script?

[Mario Sanchez Prada]

Created attachment 145725 [details]
Patch

[Mario Sanchez Prada]

Committed r119475: <http://trac.webkit.org/changeset/119475>