Bug 35210 – [Gtk] implements ChromeClient::requestGeolocationPermissionForFrame

• Component: WebKitGTK
• Status: RESOLVED FIXED

[arno.]

Hi,
in order for embedders to use gelocation, ChromeClient::requestGeolocationPermissionForFrame needs to be implemented. There is also a need for embedder to tell a frame its geolocation object is allowed. Therefore, after being notified it needs to set permission, embedders may reply in asynchronously.

[arno.]

Created attachment 49148 [details]
patch v1

Here is a first attempt. When ChromeClient::requestGeolocationPermissionForFrame is called, a signal named "geolocation-permission-requested" is emited. Then, embedders can call a method named webkit_web_frame_allow_geolocation to allow or deny geolocation for frame.

When I'll have feedback, I can work more on this patch if needed. Also, I think I'll need to modify DumpRenderTree to enable tests (maybe, I'll need help on that)

[WebKit Review Bot]

Attachment 49148 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitwebframe.h:179:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitwebview.cpp:2176:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 49148 [details]
patch v1

Thanks for the patch! I think the idea of a signal is a good one, but I think the frame_allow_geolocation function is not a very good interface. I think the signal should probably return a boolean to set if it is handled or not, and we could use a policy object, similar to the WebPolicyDecision. So, say, GeolocationPolicyDecision. You would emit the signal, and you would have a default implementation that denies the policy decision. I think that will allow us flexibility to further improve the API as the web standards move forward. I'll CC xan here, too, so he can comment. You also need to address the style issues.

[arno.]

Created attachment 49320 [details]
patch v2

[arno.]

(In reply to comment #4)
> Created an attachment (id=49320) [details]
> patch v2

This patch fixes issues raised in previous comment. I've looked at WebPolicyDecision as an example to implement GeolocationPolicyDecision.
I've also implemented cancelGeolocationPermissionRequestForFrame (as bug #34962 has landed today).

I still don't known how to modify DumpRenderTree to enable tests back.

[WebKit Review Bot]

Attachment 49320 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/303003

[arno.]

Created attachment 49321 [details]
patch v2.2

oops, I forgot to add the two new files

[WebKit Review Bot]

Attachment 49321 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 7 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Comment on attachment 49321 [details]
patch v2.2

> 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:
> +
> [phone]  Arno Renevier  <[e-mail]>
> +
> +        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.

[Gustavo Noronha (kov)]

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=49320) [details] [details]
> > patch v2
> 
> This patch fixes issues raised in previous comment. I've looked at
> WebPolicyDecision as an example to implement GeolocationPolicyDecision.
> I've also implemented cancelGeolocationPermissionRequestForFrame (as bug #34962
> has landed today).
> 
> 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 =).

[arno.]

Created attachment 49361 [details]
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.

[WebKit Review Bot]

Attachment 49361 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

(In reply to comment #11)
> > 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.

I think it's because our DumpRenderTree needs to be brought up to date with the GeoLocation stuff. If you look at these files, and search for Geo, you'll see what needs to be done:

WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp
WebKitTools/DumpRenderTree/LayoutTestController.cpp

I think we should try to get those implemented for this patch, so we can test the permission giving, and denying.

One more comment about the API. I agree to have the 'cancelled' signal, but can we name it geolocation-policy-decision-cancelled, so it's consistent with the other signal?

[arno.]

Created attachment 49423 [details]
patch v2.4

(In reply to comment #13)
> (In reply to comment #11)
> > > 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.
> 
> I think it's because our DumpRenderTree needs to be brought up to date with the
> GeoLocation stuff. If you look at these files, and search for Geo, you'll see
> what needs to be done:
> 
> WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp
> WebKitTools/DumpRenderTree/LayoutTestController.cpp
> 
> I think we should try to get those implemented for this patch, so we can test
> the permission giving, and denying.

Actually, the methods to implement in LayoutTestControllerGtk.cpp are setMockGeolocationPosition and setMockGeolocationError. But webkit gtk does not have an api that allow embedder to set location an error messages. I suppose we first need to implement client-based Geolocation support, so we'll be able to compile with ENABLE_CLIENT_BASED_GEOLOCATION, and the application will have the ability to set position and errors. But currently, I think we can't implement those methods. In my patch, I've still implemented the part that allows or denies request according to the value gLayoutTestController->geolocationPermission has been set.

> One more comment about the API. I agree to have the 'cancelled' signal, but can
> we name it geolocation-policy-decision-cancelled, so it's consistent with the
> other signal?

ok, fixed in updated patch.

My updated patch also include webkit/webkitgeolocationpolicydecision.h from webkit.h (it was not done in previous patch)

[WebKit Review Bot]

Attachment 49423 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:54:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:57:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitgeolocationpolicydecision.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 8 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[arno.]

Created attachment 49675 [details]
patch v2.5

updated patch:
include include xml/webkitgeolocationpolicydecision.xml in ./WebKit/gtk/docs/webkitgtk-docs.sgml

[arno.]

Created attachment 49676 [details]
patch v2.6

updated patch: patch did not apply to trunk anymore since changeset 55316.

[Gustavo Noronha (kov)]

Comment on attachment 49676 [details]
patch v2.6

 20 #include "config.h"
 21 
 22 #include "webkitgeolocationpolicydecision.h"
 23 
 24 #include "webkitprivate.h"
 25 
 26 #include "Geolocation.h"

These spaces are wrong. You should have this, instead:

 #include "config.h"
 #include "webkitgeolocationpolicydecision.h"

 #include "Geolocation.h"
 #include "webkitprivate.h"

 65 
 66 
 67     priv->frame = frame;

Too many empty lines here.

 78  * Since: 1.1.22

These should be 1.1.23, now =)

Except for these, I think this should be good to go.

[Xan Lopez]

Comment on attachment 49676 [details]
patch v2.6

>+/**
>+ * SECTION:webkitgeolocationpolicydecision
>+ * @short_description: Liaison between WebKit and the application regarding asynchronous geolocation policy decisions
>+ *
>+ * #WebKitGeolocationPolicyDecision objects are given to the application when
>+ * geolocation-policy-decision-requested signal is emitted. The application
>+ * uses it to tell the engine whether it wants to allow or deny geolocation for

missing 'a'

>+ * given frame.
>+ */
>+

>+typedef struct _WebKitGeolocationPolicyDecisionPrivate WebKitGeolocationPolicyDecisionPrivate;
>+struct _WebKitGeolocationPolicyDecision {
>+    GObject parent_instance;
>+
>+    /*< private >*/
>+    WebKitGeolocationPolicyDecisionPrivate* priv;
>+};
>+
>+struct _WebKitGeolocationPolicyDecisionClass {
>+    GObjectClass parent_class;
>+
>+    /* Padding for future expansion */
>+    void (*_webkit_reserved0) (void);
>+    void (*_webkit_reserved1) (void);
>+    void (*_webkit_reserved2) (void);
>+    void (*_webkit_reserved3) (void);
>+};

Should probably use camelCase for struct members.

Looks fine to me otherwise, r=1/2 me :)

[arno.]

Created attachment 49738 [details]
patch v2.7

updated patch:
fixes what's been pointed in the last two comments except one thing I did not understood:

> >+typedef struct _WebKitGeolocationPolicyDecisionPrivate WebKitGeolocationPolicyDecisionPrivate;
> >+struct _WebKitGeolocationPolicyDecision {
> >+    GObject parent_instance;
> >+
> >+    /*< private >*/
> >+    WebKitGeolocationPolicyDecisionPrivate* priv;
> >+};
> >+
> >+struct _WebKitGeolocationPolicyDecisionClass {
> >+    GObjectClass parent_class;
> >+
> >+    /* Padding for future expansion */
> >+    void (*_webkit_reserved0) (void);
> >+    void (*_webkit_reserved1) (void);
> >+    void (*_webkit_reserved2) (void);
> >+    void (*_webkit_reserved3) (void);
> >+};
> 
> Should probably use camelCase for struct members.
> 
> Looks fine to me otherwise, r=1/2 me :)


are you talking about webkit_reservedX and parent_class ? because in every other files in WebKit/gtk/webkit, case of those members is the same as the one in my patch. So, should that be different in mine ?

[Gustavo Noronha (kov)]

Comment on attachment 49738 [details]
patch v2.7

Thanks for the patch! I think the names are better kept as they are for consistency with the other headers, as you say.

[Xan Lopez]

(In reply to comment #21)
> (From update of attachment 49738 [details])
> Thanks for the patch! I think the names are better kept as they are for
> consistency with the other headers, as you say.

Well, in general I think it might make sense to keep breaking the style rules within one file to be consistent, but I don't really think it makes that much sense to keep adding new files that break them once we have decided we want to follow them as closely as possible? But maybe we haven't really done so... I think we should :)

[Gustavo Noronha (kov)]

(In reply to comment #22)
> Well, in general I think it might make sense to keep breaking the style rules
> within one file to be consistent, but I don't really think it makes that much
> sense to keep adding new files that break them once we have decided we want to
> follow them as closely as possible? But maybe we haven't really done so... I
> think we should :)

I don't think we did, but I think we should, indeed, so let's do it =D. In this specific case, though, we're talking about a public header, and that's the one case in which I think being consistent with the GTK+ style is more important.

[WebKit Commit Bot]

Comment on attachment 49738 [details]
patch v2.7

Clearing flags on attachment: 49738

Committed r55444: <http://trac.webkit.org/changeset/55444>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.