[gtk] Implement WebPolicyDelegate methods

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Pierre-Luc Beaudoin]

Only the NavigationAction delegate method is implemented.  MimeType and NewWindow are missing.

[Pierre-Luc Beaudoin]

Created attachment 18040 [details]
Suggested implementation

Open to discussion!

[Pierre-Luc Beaudoin]

Created attachment 18276 [details]
Applied some comments

bdash suggested, with reason, that the mime-type-requested signal name was not clear, we settled on mime-type-policy-decision-requested.  Since new-window-requested and navigation-requested are clear this way, I won't change them although we loose on consistency.

I removed the _real_mime_type function to place the default behavior in FrameLoaderClientGtk::dispatchDecidePolicyForMIMEType instead.  This is the only place the signal is emitted, it makes sens to me.  

I added WEBKIT_POLICY_RESPONSE_NULL to allow an handler to return an error or force the default behavior.  Also, when there is no handler to mime-type-policy-decision-requested, the return value defaults to WEBKIT_POLICY_RESPONSE_NULL which will trigger the default behavior.

[Christian Dywan]

(In reply to comment #2)
> Created an attachment (id=18276) [edit]
> Applied some comments

> I added WEBKIT_POLICY_RESPONSE_NULL to allow an handler to return an error or
> force the default behavior.  Also, when there is no handler to
> mime-type-policy-decision-requested, the return value defaults to
> WEBKIT_POLICY_RESPONSE_NULL which will trigger the default behavior.

This feels a bit ugly to me. For once _RESPONSE_NULL does not have any obvious meaning and your implementation allows a client to ACCEPT input that the webview can not handle.

What about using ACCEPT as the default policy, which is automatically replaced by DOWNLOAD if it cannot be displayed?

[Pierre-Luc Beaudoin]

For default behavior, each three delegates have their own.  Mime-type-policy-deicision-request has the most complex one of the three.  I needed a default value (ie. 0) to check if an handler has been run.  I'll see if there are other ways to do this in GObjects. 

If _ACCEPT is sent on something that webkit can't handle, the content of the file will be displayed... it is also the current behavior if I'm not mistaken and it is correct IMHO.

I do have to admit _NULL doesn't state the behavior.  _RESPONSE_NONE or _RESPONSE_ERROR might be more explicit.  I think there is a need for a fallback response, may it be ERROR or NONE.  When receiving such response, the default behavior would apply as if the signal had not be handled.

[Pierre-Luc Beaudoin]

Created attachment 18357 [details]
Small improvements and a fix

I changed _NULL to _ERROR for a more explicit signification and it matches WebPolicyDecisionListener.

Also included: webkit_web_view_can_show_mime_type() which should be used by policy handle to determine what to do with a MIME type.

I removed the signal handling in main.c as it didn't make much sens in this patch.  Removing it made me discover a bug in RessourceHandleManager.  If a ressource get a PolicyIgnore, Curl would crash as the easy handle would have been cleaned up during its processing.  Using the delayed cancel for all jobs solves this issue (said fix included in this patch).

[Pierre-Luc Beaudoin]

Created attachment 18372 [details]
Removed the fix 

Removed the fix since it is already fixed and corrected my Engrish.

[Christian Dywan]

Comment on attachment 18372 [details]
Removed the fix 

>+    g_print(" %d \n", response);

Better remove that.

>+bool FrameLoaderClient::canShowMIMEType(const String& type) const 
>+{ 
>+    // FIXME: add supported mimestypes of plugins

s/mimestypes/mimetypes

>+    g_print("DEBUG");

Better remove that as well.

>+static WebKitPolicyResponse webkit_web_view_real_new_window_requested(WebKitWebView*, WebKitWebFrame* frame, WebKitNetworkRequest*)
>+{
>+    notImplemented();
>+    // FIXME: Change this to accept once it is implemented

I wonder how that could be implemented at all. WebKit does not normally have any idea what a 'window' is in the context of the client application.

>+     * WebKitWebView::new-window-requested:
>+     * @web_view: the object on which the signal is emitted
>+     * @web_frame: the frame on which the action 
>+     * @return: WEBKIT_POLICY_RESPONSE_ACCEPT to allow WebKit to display a new window,
>+                WEBKIT_POLICY_RESPONSE_IGNORE not to display a new window
>+     *
>+     * Decides whether or not to allow a targeted navigation event, such as opening a link in a new window.
>+     * The default behavior is to accept.

The default behavior is to ignore the request.

[Pierre-Luc Beaudoin]

Created attachment 18394 [details]
Applied comments and more

"100 fois sur le métier remettez votre travail" ;)

This version is more complete.  After reviewing Christian's comments, I figured the patch was missing the action information from WebPolicyDelegates.  This times, this patch includes it and in a way that the API won't break when we fully support all other action information.  

I tested a little new-window-requested.  This event only happens on links with a target.  I set it to the default ACCEPT, but there aren't any change on the behavior.  So it is unimplemented.

[Mark Rowe (bdash)]

Comment on attachment 18394 [details]
Applied comments and more

The default logic in dispatchDecidePolicyForMIMEType doesn't quite match the Mac equivalent implementation.  You can find the Mac implementation in WebKit/mac/DefaultDelegates/WebDefaultPolicyDelegate.m.  The differences are in how file:/// are treated.

There is quite a bit of code duplication between new-window-requested and navigation-requested.  It would be good to remove some of the redundancy here.

If webkit_web_view_real_new_window_requested is the default implementation, I don't think it needs to have a notImplemented().

There are some coding style issues present too.  In some places you have the { on the incorrect line, and are missing spaces between "if" and the "(".  There are places where you have an "else" immediately after an "if" that always returns.

 703     // FIXME: add supported mimestypes of plugins

Should probably be "Add mime types supported by plugins."


Alp should probably look over this from an API point of view.

[Pierre-Luc Beaudoin]

Created attachment 18579 [details]
Applied comments and more

Applied bdash's comments.  Too many parts in the API are missing right now to implement webkit_web_view_real_navigation_requested's default behavior.

I also changed webkit_navigation_request_handled so that the signal propagation stops on ACCEPT and IGNORE which can both be valid response.  If an handler wants to delegate the handling, it should return ERROR.

[Luca Bruno]

I've this suggestion:
1) Do not create specific responses
2) Create only one general response holding a callback, a data and an enum for different response types

WebKitPolicy object is a GClosure.
"authentication-received" is created with g_signal_accumulator_true_handled.

Usage in the auth:

WEBKIT_AUTHENTICATION_RESPONSE_USE,
WEBKIT_AUTHENTICATION_RESPONSE_CONTINUE_WITHOUT_CREDENTIAL,
WEBKIT_AUTHENTICATION_RESPONSE_CANCEL

static authenticationChallengeResponseCb(WebKitPolicy*, int response, gpointer data) {
...
}

FrameLoaderClient::dispatchDidReceiveAuthenticationChallenge(...)
WebKitPolicy* policy = webkit_policy_new(authenticationChallengeResponseCb, data);
signal_emit_("authentication-received", ..., policy);
}

webkit_web_view_real_authentication_received(..., WebKitPolicy* policy) {
...
webkit_policy_response(WEBKIT_AUTHENTICATION_RESPONSE_USE);
or
webkit_policy_response(WEBKIT_AUTHENTICATION_RESPONSE_CONTINUE_WITHOUT_CREDENTIAL);
or
webkit_policy_response(WEBKIT_AUTHENTICATION_RESPONSE_CANCEL);
}

If someone connected to the signal does its job just like real_authentication_received do and can return TRUE to stop other handlers (including the the _real_ which would be the default behavior) from being invoked.

[Luca Bruno]

Created attachment 18603 [details]
async proof

This is a proof which shows how to handle authentication asyncronously using the ResponseListener.
Note that ResponseListener can be used by any other response type.
Conclusion: don't read the code that creates the GUI for the authentication dialog :P

[Pierre-Luc Beaudoin]

Created attachment 18970 [details]
New Async version

See Changelog for details and limitations.

Please apply and try this.  If I have no comments by Friday, I'll set it up to be reviewed.

[Pierre-Luc Beaudoin]

Created attachment 18971 [details]
Some UI that may help testing

This patch contains some dialog window I used to test the signals.  Note that redirection navigation evens needs to be sync so you will for sure have an assertion using this patch when visiting www.google.com outside US.

[Christian Dywan]

(In reply to comment #13)
> Please apply and try this.  If I have no comments by Friday, I'll set it up to
> be reviewed.

You should just mark it for review. Else nobody might think there's anything to look at.

I would like to know a) why asynch doesn't work for navigation and whether one can expect this to work in the future, and b) *if* this is not going to work anytime soon, I'd think a simple callback with no PolicyDesision object is more reasonable since it's simpler and faster.

[Pierre-Luc Beaudoin]

(In reply to comment #15)
> I would like to know a) why asynch doesn't work for navigation and whether one
> can expect this to work in the future, and b) *if* this is not going to work
> anytime soon, I'd think a simple callback with no PolicyDecision object is more
> reasonable since it's simpler and faster.

a) it does work, but not when the navigation request comes from a redirection.  Plus you have to return your answer before another policy started. As per WebCore limitation.
b) it does work within limited situations, the same than the mac port has AFAIK.

[Pierre-Luc Beaudoin]

Created attachment 19530 [details]
Proposed patch

Fixes styling issues and apply suggested changes by Zecke:
- drop switch since the enums are sync;
- don't check for file existence;
- return instead of indenting lots of code.

[Srinivas Rao M Hamse]

About the patch:
https://bugs.webkit.org/attachment.cgi?id=19530&action=view

This patch file was not compatible with the latest nightlies(r31848). Hence i manually applied the those hunks that were failing to respective files. I could compile and build GtkLauncher. I was unable to open any new window/popup when i click on the following href links


<html>
<body>
HTML version

<a href="a.html" target="_blank">Open window</a>
<br>

Javascript version

<a href="javascript:window.open('a.html', '_blank')">Open window</a>

</body>
</html>

On the console i got the following error messages, This came only for the first time:

for HTML Version i got :

UNIMPLEMENTED:
(../WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:770 virtual WebCore::Frame* WebKit::FrameLoaderClient::dispatchCreatePage())
UNIMPLEMENTED:
(../WebCore/page/gtk/EventHandlerGtk.cpp:85 bool WebCore::EventHandler::eventActivatedView(const WebCore::PlatformMouseEvent&) const)


for JavaScript Version I got:

UNIMPLEMENTED:
(../WebKit/gtk/webkit/webkitwebview.cpp:488 void webkit_web_view_real_window_object_cleared(WebKitWebView*, WebKitWebFrame*, OpaqueJSContext*, OpaqueJSValue*))
UNIMPLEMENTED:
(../WebCore/platform/gtk/PlatformScreenGtk.cpp:73 WebCore::FloatRect WebCore::screenAvailableRect(WebCore::Widget*))
UNIMPLEMENTED:
(../WebKit/gtk/webkit/webkitwebview.cpp:481 WebKitWebView* webkit_web_view_real_create_web_view(WebKitWebView*))


Adding these logs as a reference to the author of this patch.

[Marco Barisione]

Created attachment 21001 [details]
Implement WebPolicyDelegate methods

Updated patch, I modified it so it can be applied and fixed a couple of problems.

How about replacing the action_information hash table with an object?

(Sorry but I cannot mark the old patch as obsolete as I didn't submit it)

[Marco Barisione]

Created attachment 21288 [details]
Implement WebPolicyDelegate methods

I removed the new-window-requested signal as it works only for links with target=_blank and doesn't seem as useful as the solution proposed in bug #19143. We can add it later if it's really needed.

The actionInformation hash table has been replaced with a WebKitWebNavigationAction class. Note that this classes as an enum property, so this patch dependes on bug #19171.

[Wouter Bolsterlee]

(In reply to comment #11)
> static authenticationChallengeResponseCb(WebKitPolicy*, int response, gpointer
> data) {

The 'int response' should probably something like 'enum WebKitResponseType' from a GTK API point of view (cf. 'enum GtkResponseType').

[Eric Seidel (no email)]

Comment on attachment 21288 [details]
Implement WebPolicyDelegate methods

Assigning to Alp for review for for re-assignment to someone who can review this.  It's silly to have this in the general queue since most of us have no clue about Gtk.

[Marco Barisione]

Created attachment 23794 [details]
Implement WebPolicyDelegate methods

Updated patch that fix some issues find with a lot of more testing.

Note that this patch doesn't add function pointers for the new signals so we don't break ABI compatibility but they should be added when we break it.

[Jan Alonzo]

Comment on attachment 23794 [details]
Implement WebPolicyDelegate methods

Alp to review...

[Xan Lopez]

+     * Decide whether or not to display of not the given MIME type.

lose the 'of not'?

+     * If this signal is not handled, the default behavior is to show the
+     * content of the requested URI if WebKit can show this MIME type,
+     * else it will be ignored.

I'm not a native speaker, but I think 'otherwise' sounds better than 'else' here.

[Xan Lopez]

+typedef struct _WebKitWebPolicyDecisionPrivate WebKitWebPolicyDecisionPrivate;
+struct _WebKitWebPolicyDecision {
+    GObject parent;

parent_instance

+    WebKitWebPolicyDecisionPrivate* priv;
+};
+
+struct _WebKitWebPolicyDecisionClass {
+    GObjectClass parent;

parent_class

+};

[Xan Lopez]

+    g_object_class_install_property(objectClass, PROP_ORIGINAL_URI,
+                                    g_param_spec_string("original-uri",
+                                                        "Original URI",
+                                                        "The URI that was requested as the target for the navigation",
+                                                        "",
+                                                        WEBKIT_PARAM_READWRITE));

Maybe you can make the property CONSTRUCT and save setting it in _init?

[Gustavo Noronha (kov)]

Created attachment 25924 [details]
patch reworked to apply cleanly

This is barisione's patch only with some minor "editorial" changes I had to do to make it apply and build in current WebKit.

[Gustavo Noronha (kov)]

Created attachment 26036 [details]
reworked patch, with changes proposed by Xan

[Holger Freyther]

(In reply to comment #29)
> Created an attachment (id=26036) [review]
> reworked patch, with changes proposed by Xan

Can we obsolete the other patch in the review queue?

[Holger Freyther]

Comment on attachment 23794 [details]
Implement WebPolicyDelegate methods

This is obsoleted.

[Holger Freyther]

(In reply to comment #29)
> Created an attachment (id=26036) [review]
> reworked patch, with changes proposed by Xan

I will not finish a review today. So let me start with

style issues:
 -  /* and */ should be on separate lines (FrameLoaderClient::dispatchDecidePolicyForNewWindowAction). 
 - When I would do if (handled)\n return (as trained by aroben) you do it the otherway around but that is okay.

api doc comments:
 - I would be cool if you add /*< private >*/ in the Instance and Class of the new structs.
 - When referencing a signal you need the full path WebKitFoo:signal (and maybe even the #)


ref counting:
  - With the still open FrameLoaderClient bug we will work on the ref counting. In ~FrameLoaderClient() you might want to set the internal Frame of the WebPolicy to 0 and add null checks to the WebPolicy.


Let us accumulate the issues and then only do one final version before landing.

[Gustavo Noronha (kov)]

Created attachment 26149 [details]
proposed patch, with comments by zecke covered

[Gustavo Noronha (kov)]

Created attachment 26157 [details]
with since tags

[Christian Dywan]

> A new object WebKitWebPolicyDecision allow the browser to delay its

"allow" -> "allows"

> gpointer navigationAction = g_object_new(WEBKIT_TYPE_WEB_NAVIGATION_ACTION,

Why is this a gpointer? This should be WebKitTypeWebNavigation.

> + if (navigationAction->priv->originalUri &&
> +        (!strcmp(navigationAction->priv->originalUri, originalUri)))
> +        return;

You're missing the case where both is NULL.

> + * modify it under the terms of the GNU Library General Public

It's *Lesser* General Public License these days :)

> +     * @return: a WebKitNavigationResponse

We normally use "Return value: ", better keep it consistent.

> +gboolean webkit_web_view_can_show_mime_type (WebKitWebView* webView, const gchar* > mimeType)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), FALSE);

What kind of string is expected here? It's not NULL checked - possibly intentional - and what about the encoding, is WebCore expecting UTF-8? Apologies if it's actually fine like that.

> +    g_object_class_install_property(objectClass, PROP_REASON,
> +                                    g_param_spec_enum("reason",
> +                                                      "Reason",
> +                                                      "The reason why this navigation is occurring",
> +                                                      WEBKIT_TYPE_WEB_NAVIGATION_REASON,
> +                                                      WEBKIT_WEB_NAVIGATION_REASON_OTHER,
> +                                                      (GParamFlags)(WEBKIT_PARAM_READWRITE |
> G_PARAM_CONSTRUCT)));
> +
> +    g_object_class_install_property(objectClass, PROP_ORIGINAL_URI,
> +                                    g_param_spec_string("original-uri",
> +                                                        "Original URI",
> +                                                        "The URI that was requested as the target for the navigation",
> +                                                        "",
> +                                                        (GParamFlags)(WEBKIT_PARAM_READWRITE | > G_PARAM_CONSTRUCT)));

We need doc comments there, not to forget Since tags.

[Holger Freyther]

(In reply to comment #35)
> > A new object WebKitWebPolicyDecision allow the browser to delay its
> "allow" -> "allows"

right, will fix that when landing.


 


> > gpointer navigationAction = g_object_new(WEBKIT_TYPE_WEB_NAVIGATION_ACTION,
> Why is this a gpointer? This should be WebKitTypeWebNavigation.

this avoids a cast... but I change that when landing




> 
> > + if (navigationAction->priv->originalUri &&
> > +        (!strcmp(navigationAction->priv->originalUri, originalUri)))
> > +        return;
> 
> You're missing the case where both is NULL.

This can not happen as the uri property is marked as construct.




> 
> > + * modify it under the terms of the GNU Library General Public
> It's *Lesser* General Public License these days :)


Yeah, I agree with you, we are just following the majority of webkit in this regard.



> 
> > +     * @return: a WebKitNavigationResponse
> 
> We normally use "Return value: ", better keep it consistent.
_PARAM_CONSTRUCT)));
> 
> We need doc comments there, not to forget Since tags.
> 



will try to fix both when landing.

[Holger Freyther]

Comment on attachment 26157 [details]
with since tags

Okay, let us see what this is breaking. :)

[Holger Freyther]

Landed in r39421.