[webkit-reviews] review requested: [Bug 35210] [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame : [Attachment 49361] patch v2.3

[bugzilla-daemon at webkit.org]

arno. <arno at renevier.net> has asked  for review:
Bug 35210: [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame
https://bugs.webkit.org/show_bug.cgi?id=35210

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

------- Additional Comments from arno. <arno at renevier.net>
(In reply to comment #9)

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

ok, fixed in updated patch


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

ok, fixed in updated patch

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

It's called when permission has been asked, has not been set, but is not needed
anymore. Then for example, embedder can decide to not display the question
anymore. Currently, it's only called when the frame owning a geolocation is
destroyed.

> 
>  586	   gboolean isHandled = false;
> 
> Use FALSE here.

ok, fixed in updated patch

> 
> > +/**
> > + * 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.

great idea! fixed in updated patch.


(In reply to comment #10)
> > I still don't known how to modify DumpRenderTree to enable tests back.
> 
> About this, there is a file in which all skipped tests are listed, which is
> used by the run-webkit-tests script to skip them while running the tests:
> LayoutTests/platform/gtk/Skipped
> 
> So you just need to remove the relevant tests from there =).

Unfortunately, some tests still fail. I suppose (unsure) that's because our
implementation denies by default. A few tests pass. I don't known if this is by
only chance. I uncommented the passing tests in updated patch.