Bug 83879

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

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.

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

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.

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

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

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

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.

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?

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.

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

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.

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

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.

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

Created attachment 144313 [details] Patch New updated patch, as the old one didn't apply anymore

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

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.

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

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)

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.

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.

(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

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

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?

Created attachment 145725 [details] Patch

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