[webkit-reviews] review requested: [Bug 83879] [GTK][WK2] Implement API for Geolocation permission requests in the GTK port : [Attachment 145630] Patch proposal + Unit test

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  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 145630: Patch proposal + Unit test
https://bugs.webkit.org/attachment.cgi?id=145630&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
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.