[webkit-reviews] review denied: [Bug 35210] [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame : [Attachment 49321] patch v2.2

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied arno. <arno at renevier.net>'s
request for review:
Bug 35210: [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame
https://bugs.webkit.org/show_bug.cgi?id=35210

Attachment 49321: patch v2.2
https://bugs.webkit.org/attachment.cgi?id=49321&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> diff --git a/ChangeLog b/ChangeLog
> index a263d76..129f77c 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -2,6 +2,15 @@
>  
>	   Reviewed by NOBODY (OOPS!).
>  
> +	   [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame
> +	   https://bugs.webkit.org/show_bug.cgi?id=35210
> +
> +	   * GNUmakefile.am:
> +
> +2010-02-23  Arno Renevier  <arno at renevier.net>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
>	   [Gtk] testwebview does not work when called with absolute path
>	   https://bugs.webkit.org/show_bug.cgi?id=34940
>  

This kind of changelog diffs always break our commit queue script, which is
said. If you can rebase your patch on top of trunk while applying the suggested
changes, I'd be grateful.

> +    if (!isHandled) {
> +	   // signal has not been handled, deny by default.
> +	   webkit_geolocation_policy_set_permission(policyDecision, FALSE);
> +    }

I think the comment is needless, as what you're doing is pretty obvious. I'd
remove it, and the braces.

> +void
ChromeClient::cancelGeolocationPermissionRequestForFrame(WebCore::Frame* frame)

> +{
> +    WebKitWebFrame* webFrame = kit(frame);
> +    WebKitWebView* webView = getViewFromFrame(webFrame);
> +    g_signal_emit_by_name(webView,
"geolocation-permission-request-canceled", webFrame);

Hrmmm. This is interesting. How is this used? It seems like all ports currently
have empty implementations for this. Is this required to remove permission from
a page when you close the window, or what?

 586	 gboolean isHandled = false;

Use FALSE here.

> +/**
> + * webkit_geolocation_policy_set_permission
> + * @decision: a #WebKitGeolocationPolicyDecision
> + *
> + * Will send the allow or deny decision to the policy implementer.
> + *
> + * Since: 1.1.22
> + */
> +void
webkit_geolocation_policy_set_permission(WebKitGeolocationPolicyDecision*
decision, bool allowed)
> +{
> +    g_return_if_fail(WEBKIT_IS_GEOLOCATION_POLICY_DECISION(decision));
> +
> +    WebKitGeolocationPolicyDecisionPrivate* priv = decision->priv;
> +    priv->geolocation->setIsAllowed(allowed);
> +}

Great work, this is indeed what I meant =). I think we should be closer to
WebPolicyDecision on how you set the decision as well, though. That means I'd
prefer to see webkit_geolocation_policy_accept(), and
webkit_geolocation_policy_deny() functions instead of a single function that
takes a boolean. In the future we may have more finer grained policies, for
instance, and having those would be more flexible.

Thanks for updating the documentation control files, too! I'll poke Xan, and
see if I can get him to take a quick look at the proposed API.