[Webkit-unassigned] [Bug 35210] [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=35210


arno. <arno at renevier.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #49321|0                           |1
        is obsolete|                            |
  Attachment #49361|                            |review?
               Flag|                            |




--- Comment #11 from arno. <arno at renevier.net>  2010-02-23 23:16:58 PST ---
Created an attachment (id=49361)
 --> (https://bugs.webkit.org/attachment.cgi?id=49361)
patch v2.3

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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.