Bug 76343 – [GTK] [WK2] Implement the policy client

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

The policy client is required to port devhelp and yelp.

[Andreas Kling]

Committed r105031: <http://trac.webkit.org/changeset/105031>

[David Kilzer (:ddkilzer)]

I'm pretty sure Andreas referenced the wrong bug.  Reopening.

[Andreas Kling]

(In reply to comment #2)
> I'm pretty sure Andreas referenced the wrong bug.  Reopening.

Double-duh! Sorry about that :/

[Martin Robinson]

Created attachment 123455 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 123455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123455&action=review

Great work, and great documentation. Thanks!

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:24
> +#include "WebKitPolicyDecisionPrivate.h"

This is not needed here, it just contains webkitPolicyDecisionSetListener

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:28
> +#include <WebKit2/WKAPICast.h>

Isn't this already included by WebKitPrivate.h?

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:160
> +                                    g_param_spec_uint("modifiers",
> +                                                      _("Mouse event modifiers"),
> +                                                      _("The modifiers active if this decision was triggered by a mouse event"),
> +                                                      0, G_MAXUINT, 0,
> +                                                      WEBKIT_PARAM_READABLE));

Why not using a flags param spec with GDK_TYPE_MODIFIER_TYPE?

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:177
> +     * WebKitNavigationPolicyDecision:frame-name:

Maybe this could be just target, I would avoid using the name Frame since we are not exposing frames in the API for now.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:181
> +     * In all other cases, this value will be null.

Use %NULL

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:192
> +/*

API docs should start with double *

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:206
> +/*

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:220
> +/*

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:234
> +/*

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:240
> + * Returns: The URI request that is associated with this navigation

Mark this as transfer none

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:248
> +/*

Double * here

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:249
> + * webkit_navigation_policy_decision_get_frame_name:

what about get_link_target? this way we also make it clear that this is was triggered by a link click.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:254
> + * Returns: The value of the target attribute if this navigation was triggered by a link click

I would mention this returns NULL otherwise

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:266
> +COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_NAVIGATION_TYPE_LINK_CLICKED, kWKFrameNavigationTypeLinkClicked);
> +COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_NAVIGATION_TYPE_FORM_SUBMITTED, kWKFrameNavigationTypeFormSubmitted);
> +COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_NAVIGATION_TYPE_BACK_FORWARD, kWKFrameNavigationTypeBackForward);
> +COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_NAVIGATION_TYPE_FORM_RESUBMITTED, kWKFrameNavigationTypeFormResubmitted);
> +COMPILE_ASSERT_MATCHING_ENUM(WEBKIT_NAVIGATION_TYPE_OTHER, kWKFrameNavigationTypeOther);

RELOAD is missing here.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:276
> +        return WEBKIT_MOUSE_BUTTON_LEFT;

This should be middle, not left.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:305
> +    decision->priv->navigationType = static_cast<WebKitNavigationType>(navigationType);
> +    decision->priv->mouseButton = wkEventMouseButtonToWebKitMouseButton(mouseButton);
> +    decision->priv->modifiers = wkEventModifiersToUnsigned(modifiers);
> +    decision->priv->request = adoptGRef(webkitURIRequestCreateForResourceRequest(toImpl(request)->resourceRequest()));
> +    decision->priv->frameName = frameName;

I think this should all be construct only properties.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h:44
> + **/

No need for double * here.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h:64
> + * Enum values used to denote the various mouse buttons that triggered
> + * events within WebKit. This enum is designed to match up to the button
> + * values used in GDK events, except for @WEBKIT_MOUSE_BUTTON_NONE which
> + * has no equivalent in GDK.

I would just the GDK values in public API. Internally I would use this enum, and add a method to the API to check whether the decision is for an action triggered by a mouse button that simply checks whether button is none.

> Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecisionPrivate.h:24
> +#include <WebKitPrivate.h>

Use the quoted form for header files of WebKit2

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClient.cpp:29
> +#include <WebKit2/WKAPICast.h>

Isn't this already included by WebKitPrivate.h?

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:24
> +#include "config.h"
> +#include "WebKitPolicyDecision.h"
> +
> +#include "WebKitPrivate.h"
> +#include <WebKit2/WKAPICast.h>

This should include PolicyDecisionPrivate for webkitPolicyDecisionSetListener

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:46
> +    WKFramePolicyListenerRef listener;

Do we know the lifetime of the listener? maybe we could use WKRetainPtr to make sure

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:63
> +    // This is the default choice for all policy decisions in WebPageProxy.cpp.
> +    if (!priv->madePolicyDecision)
> +        WKFramePolicyListenerUse(priv->listener);

Why do we need to call this on finalize?

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:71
> +     WEBKIT_POLICY_DECISION(decision)->priv->listener = listener;

You don't need the cast.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:569
> +     * Returns: %TRUE to stop other handlers from being invoked for the event.
> +     *   %FALSE to propagate the event further.

Move this to the end of the block comment

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:578
> +     * gboolean

static gboolean

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:606
> +     * It is possible to make policy decision asynchronous, by simply calling g_object_ref()
> +     * on the @decision argument. If the last reference is removed on a #WebKitPolicyDecision
> +     * and no decision has been made explicitly, webkit_policy_decision_use() will be the
> +     * default policy decision.

Now I understand why are using calling Use in the decision finalize(). What's the use case of an asnyc policy decision? I think we should just add a default impl here that calls _use().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:47
> + *   webkit_policy_decision_use or webkit_policy_decision_ignore. This

webkit_policy_decision_use() or webkit_policy_decision_ignore()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:52
> + *   webkit_policy_decision_ignore. This type of policy decision is always

webkit_policy_decision_ignore()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:53
> + *   a #WebKitNavigationPolicyDecision. This signal is useful for implementing

What signal?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:63
> + *   a #WebKitResponsePolicyDecision. This is signal is useful for forcing

What signal?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitPolicyClient.cpp:161
> +    g_object_set(webkit_web_view_get_settings(test->m_webView), "javascript-can-open-windows-automatically", TRUE, NULL);

It's probably clear to use webkit_settings_set_javascript_can_open_windows_automatically()

[Martin Robinson]

(In reply to comment #6)
> (From update of attachment 123455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123455&action=review
> 
> Great work, and great documentation. Thanks!

Thanks for the incredibly thorough review!

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:24
> > +#include "WebKitPolicyDecisionPrivate.h"
> 
> This is not needed here, it just contains webkitPolicyDecisionSetListener

It's actually used in this file. I confirmed that removing it causes a compilation error.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:28
> > +#include <WebKit2/WKAPICast.h>
> Isn't this already included by WebKitPrivate.h?

Yep! It wasn't when I original wrote this bit.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:160
> > +                                    g_param_spec_uint("modifiers",
> > +                                                      _("Mouse event modifiers"),
> > +                                                      _("The modifiers active if this decision was triggered by a mouse event"),
> > +                                                      0, G_MAXUINT, 0,
> > +                                                      WEBKIT_PARAM_READABLE));
> Why not using a flags param spec with GDK_TYPE_MODIFIER_TYPE?

The modifiers mask is a bitmask (unsigned) and not a GdkModifier.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:177
> > +     * WebKitNavigationPolicyDecision:frame-name:
> 
> Maybe this could be just target, I would avoid using the name Frame since we are not exposing frames in the API for now.

I'm not sure the API presents any guarantee that a frame names necessarily means this policy decision is from clicking on links. Both the WebCore and WebKit2 API use this name, so I don't think we should deviate here. I've fixed the documentation to give the target attribute as an example of one instance when there is a frame name instead of the only instance.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:181
> > +     * In all other cases, this value will be null.
> 
> Use %NULL

Fixed!

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:192
> > +/*
> 
> API docs should start with double *

I think I fixed all these now.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:240
> > + * Returns: The URI request that is associated with this navigation
> 
> Mark this as transfer none

Doh! I meant to do this before posting the first patch. Thanks for spotting it.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:249
> > + * webkit_navigation_policy_decision_get_frame_name:
> 
> what about get_link_target? this way we also make it clear that this is was triggered by a link click.

See my comment above. I'm don't think it's entirely correct to be that specific here.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:254
> > + * Returns: The value of the target attribute if this navigation was triggered by a link click
> 
> I would mention this returns NULL otherwise

Fixed!

> RELOAD is missing here.

Ack. Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:276
> > +        return WEBKIT_MOUSE_BUTTON_LEFT;
> 
> This should be middle, not left.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:305
> > +    decision->priv->navigationType = static_cast<WebKitNavigationType>(navigationType);
> > +    decision->priv->mouseButton = wkEventMouseButtonToWebKitMouseButton(mouseButton);
> > +    decision->priv->modifiers = wkEventModifiersToUnsigned(modifiers);
> > +    decision->priv->request = adoptGRef(webkitURIRequestCreateForResourceRequest(toImpl(request)->resourceRequest()));
> > +    decision->priv->frameName = frameName;
> 
> I think this should all be construct only properties.

I didn't take this suggestion because I think making these construct-only send the message that it's fine to construct this object with g_object_new. It also adds a lot of boilerplate, to accomplish the same behavior. If we want to add an external construction site, we can make them construct-only.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h:44
> > + **/
> 
> No need for double * here.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h:64
> > + * Enum values used to denote the various mouse buttons that triggered
> > + * events within WebKit. This enum is designed to match up to the button
> > + * values used in GDK events, except for @WEBKIT_MOUSE_BUTTON_NONE which
> > + * has no equivalent in GDK.
> 
> I would just the GDK values in public API. Internally I would use this enum, and add a method to the API to check whether the decision is for an action triggered by a mouse button that simply checks whether button is none.

GDK doesn't really have any values published in the documentation. The mapping of buttons to integers seems, at best, underdocumented in GDK.  Making an enum here removes all ambiguity. If GDK adds such an enum in the future, we can switch to using it without worrying about breaking API/ABI.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecisionPrivate.h:24
> > +#include <WebKitPrivate.h>
> 
> Use the quoted form for header files of WebKit2

Fixed.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClient.cpp:29
> > +#include <WebKit2/WKAPICast.h>
> 
> Isn't this already included by WebKitPrivate.h?

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:24
> > +#include "config.h"
> > +#include "WebKitPolicyDecision.h"
> > +
> > +#include "WebKitPrivate.h"
> > +#include <WebKit2/WKAPICast.h>
> 
> This should include PolicyDecisionPrivate for webkitPolicyDecisionSetListener

It's not entirely necessary, but it will prevent making the signatures clash by accident, so I've included it.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:46
> > +    WKFramePolicyListenerRef listener;
> 
> Do we know the lifetime of the listener? maybe we could use WKRetainPtr to make sure

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:71
> > +     WEBKIT_POLICY_DECISION(decision)->priv->listener = listener;
> 
> You don't need the cast.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:569
> > +     * Returns: %TRUE to stop other handlers from being invoked for the event.
> > +     *   %FALSE to propagate the event further.
> 
> Move this to the end of the block comment

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:578
> > +     * gboolean
> 
> static gboolean

Fixed.

> Now I understand why are using calling Use in the decision finalize(). What's the use case of an asnyc policy decision? I think we should just add a default impl here that calls _use().

We had a pretty thorough discussion via Jabber. I hope I convinced you this is a very important feature.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:47
> > + *   webkit_policy_decision_use or webkit_policy_decision_ignore. This
> 
> webkit_policy_decision_use() or webkit_policy_decision_ignore()
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:52
> > + *   webkit_policy_decision_ignore. This type of policy decision is always
> 
> webkit_policy_decision_ignore()

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:53
> > + *   a #WebKitNavigationPolicyDecision. This signal is useful for implementing
> 
> What signal?

Removed all references to signals here. This was left over from when each decision type was a signal.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitPolicyClient.cpp:161
> > +    g_object_set(webkit_web_view_get_settings(test->m_webView), "javascript-can-open-windows-automatically", TRUE, NULL);
> 
> It's probably clear to use webkit_settings_set_javascript_can_open_windows_automatically()

Okay.

[Martin Robinson]

Created attachment 123828 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #7)
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:24
> > > +#include "WebKitPolicyDecisionPrivate.h"
> > 
> > This is not needed here, it just contains webkitPolicyDecisionSetListener
> 
> It's actually used in this file. I confirmed that removing it causes a compilation error.

right, sorry.

> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:160
> > > +                                    g_param_spec_uint("modifiers",
> > > +                                                      _("Mouse event modifiers"),
> > > +                                                      _("The modifiers active if this decision was triggered by a mouse event"),
> > > +                                                      0, G_MAXUINT, 0,
> > > +                                                      WEBKIT_PARAM_READABLE));
> > Why not using a flags param spec with GDK_TYPE_MODIFIER_TYPE?
> 
> The modifiers mask is a bitmask (unsigned) and not a GdkModifier.

I always forget this is not possible in C++.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.cpp:305
> > > +    decision->priv->navigationType = static_cast<WebKitNavigationType>(navigationType);
> > > +    decision->priv->mouseButton = wkEventMouseButtonToWebKitMouseButton(mouseButton);
> > > +    decision->priv->modifiers = wkEventModifiersToUnsigned(modifiers);
> > > +    decision->priv->request = adoptGRef(webkitURIRequestCreateForResourceRequest(toImpl(request)->resourceRequest()));
> > > +    decision->priv->frameName = frameName;
> > 
> > I think this should all be construct only properties.
> 
> I didn't take this suggestion because I think making these construct-only send the message that it's fine to construct this object with g_object_new. 

I don't see why, you can still use g_object_new even if properties are not construct only.

> It also adds a lot of boilerplate, to accomplish the same behavior. If we want to add an external construction site, we can make them construct-only.

It's just a SetProperty method, I don't think that's a lot of boilerplate.
 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitNavigationPolicyDecision.h:64
> > > + * Enum values used to denote the various mouse buttons that triggered
> > > + * events within WebKit. This enum is designed to match up to the button
> > > + * values used in GDK events, except for @WEBKIT_MOUSE_BUTTON_NONE which
> > > + * has no equivalent in GDK.
> > 
> > I would just the GDK values in public API. Internally I would use this enum, and add a method to the API to check whether the decision is for an action triggered by a mouse button that simply checks whether button is none.
> 
> GDK doesn't really have any values published in the documentation. The mapping of buttons to integers seems, at best, underdocumented in GDK.  Making an enum here removes all ambiguity. If GDK adds such an enum in the future, we can switch to using it without worrying about breaking API/ABI.

We would break the API/ABI if we remove the enum, or we would end up with a deprecated enum. Also I don't think the policy client code is the right place for that enum. I'm already working on a patch for GDK so that we don't need this enum. The patch will just add a #define, so we can simply use guint for button numbers.

> > Now I understand why are using calling Use in the decision finalize(). What's the use case of an asnyc policy decision? I think we should just add a default impl here that calls _use().
> 
> We had a pretty thorough discussion via Jabber. I hope I convinced you this is a very important feature.

Yes, I'm convinced the feature is important, but I'm not so convinced that the approach of calling g_object_ref means I want to take the decision later, is clear enough even if it's documented.

[Martin Robinson]

(In reply to comment #9)

> > We had a pretty thorough discussion via Jabber. I hope I convinced you this is a very important feature.
> 
> Yes, I'm convinced the feature is important, but I'm not so convinced that the approach of calling g_object_ref means I want to take the decision later, is clear enough even if it's documented.

For what it's worth, this was the behavior in WebKit1 as well and no one ever complained or asked about it on the channel in as long as I can remember. There are alternatives though:

1. Call webkit_policy_use in the default signal handler. From the user's point of view this would be exactly the same though. They'd block the signal handler as well as calling g_object_ref/g_object_unref on the decision.
2. We could wrap g_object_ref/g_object_unref in some friendler methods. I don't mind that, but this approch doesn't prevent it either.

The nice thing about this design is that it enforces an important invariant: the WebKitPolicyListener never dies when a policy decision has not been made yet.

[Carlos Garcia Campos]

Comment on attachment 123828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123828&action=review

Looks good! I would just remove the button enum and use guint as we already talked.

> Source/WebKit2/GNUmakefile.am:540
> +	Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClient.cpp \
> +	Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClientPrivate.h \

PolicyClient is already private API, we use Private.h for private declaration of public API, so in this case we could just use WebKitPolicyClient.h like the other clients. I thought you had forgotten to add the file the first time saw it was missing in the makefile.

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClient.cpp:27
> +#include "WebKitWebView.h"
> +#include "WebKitWebViewPrivate.h"

WebKitWebViewPrivate.h already includes WebKitWebView.h. As a general rule FooPrivate.h always includes Foo.h

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClient.cpp:62
> +void attachPolicyClientToPage(WKPageRef page)

Other clients use the View instead of the page, and pass the view as clientInfo to the callbacks so that we don't need to get it from the page.

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClientPrivate.h:22
> +#if !defined(__WEBKIT2_H_INSIDE__) && !defined(WEBKIT2_COMPILATION)
> +#error "Only <webkit2/webkit2.h> can be included directly."
> +#endif

This is only for public headers.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:74
> +typedef enum {
> +    WEBKIT_POLICY_DECISION_TYPE_NAVIGATION_ACTION,
> +    WEBKIT_POLICY_DECISION_TYPE_NEW_WINDOW_ACTION,
> +    WEBKIT_POLICY_DECISION_TYPE_RESPONSE,
> +} WebKitPolicyDecisionType;

Shouldn't this be in WebKitPolicyDecision.h?

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > > We had a pretty thorough discussion via Jabber. I hope I convinced you this is a very important feature.
> > 
> > Yes, I'm convinced the feature is important, but I'm not so convinced that the approach of calling g_object_ref means I want to take the decision later, is clear enough even if it's documented.
> 
> For what it's worth, this was the behavior in WebKit1 as well and no one ever complained or asked about it on the channel in as long as I can remember.

That might be because nobody uses the async alternative. 

> There are alternatives though:
> 
> 1. Call webkit_policy_use in the default signal handler. From the user's point of view this would be exactly the same though. They'd block the signal handler as well as calling g_object_ref/g_object_unref on the decision.

In general, returning TRUE means that you have handled the signal and you don't want any other handlers to be invoked. In this case it doesn't matter whether you return TRUE or FALSE if you takes a reference. What I would expect is that if I return FALSE, the default handler ends up handling the default decision. If I return TRUE is because I want to handle the decision, so I can just do it and return or postpone it taking a reference of the decision object and returning TRUE.

> 2. We could wrap g_object_ref/g_object_unref in some friendler methods. I don't mind that, but this approch doesn't prevent it either.

I think It's more obvious that I have to take a reference of the decision object if I say somehow (returning TRUE) that I want to handle the decision.

> The nice thing about this design is that it enforces an important invariant: the WebKitPolicyListener never dies when a policy decision has not been made yet.

We can leave the _use() in the finalize, because it's protected by the decisionMade variable.

[Martin Robinson]

(In reply to comment #11)

> > Source/WebKit2/GNUmakefile.am:540
> > +	Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClient.cpp \
> > +	Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClientPrivate.h \
> 
> PolicyClient is already private API, we use Private.h for private declaration of public API, so in this case we could just use WebKitPolicyClient.h like the other clients. I thought you had forgotten to add the file the first time saw it was missing in the makefile.

I'm not sure I entirely agree with this approach, but I don't see any harm in it when we can just ignore *Client* in the gtkdoc script.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClient.cpp:27
> > +#include "WebKitWebView.h"
> > +#include "WebKitWebViewPrivate.h"
> 
> WebKitWebViewPrivate.h already includes WebKitWebView.h. As a general rule FooPrivate.h always includes Foo.h

Okay. Perhaps we should just make WebKitPrivate.h include everything. It would clean up the header list in a lot of the API.

> > Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClient.cpp:62
> > +void attachPolicyClientToPage(WKPageRef page)
> 
> Other clients use the View instead of the page, and pass the view as clientInfo to the callbacks so that we don't need to get it from the page.

Fixed. This change was made recently. :)

> > Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClientPrivate.h:22
> > +#if !defined(__WEBKIT2_H_INSIDE__) && !defined(WEBKIT2_COMPILATION)
> > +#error "Only <webkit2/webkit2.h> can be included directly."
> > +#endif
> 
> This is only for public headers.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:74
> > +typedef enum {
> > +    WEBKIT_POLICY_DECISION_TYPE_NAVIGATION_ACTION,
> > +    WEBKIT_POLICY_DECISION_TYPE_NEW_WINDOW_ACTION,
> > +    WEBKIT_POLICY_DECISION_TYPE_RESPONSE,
> > +} WebKitPolicyDecisionType;
> 
> Shouldn't this be in WebKitPolicyDecision.h?

It's only used in WebKitWebView.

[Martin Robinson]

(In reply to comment #12)

> In general, returning TRUE means that you have handled the signal and you don't want any other handlers to be invoked. In this case it doesn't matter whether you return TRUE or FALSE if you takes a reference. What I would expect is that if I return FALSE, the default handler ends up handling the default decision. If I return TRUE is because I want to handle the decision, so I can just do it and return or postpone it taking a reference of the decision object and returning TRUE.
> 
> > 2. We could wrap g_object_ref/g_object_unref in some friendler methods. I don't mind that, but this approch doesn't prevent it either.
> 
> I think It's more obvious that I have to take a reference of the decision object if I say somehow (returning TRUE) that I want to handle the decision.
> 
> > The nice thing about this design is that it enforces an important invariant: the WebKitPolicyListener never dies when a policy decision has not been made yet.
> 
> We can leave the _use() in the finalize, because it's protected by the decisionMade variable.

I'm not entirely convinced, but as you say it's not as common to handle these decisions asynchronously. So since you seem to feel strongly about this, I have taken your suggestion and made it a requirement to return TRUE here to handle decisions aysnchronously.

[Martin Robinson]

Created attachment 123985 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #13)
> (In reply to comment #11)
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:74
> > > +typedef enum {
> > > +    WEBKIT_POLICY_DECISION_TYPE_NAVIGATION_ACTION,
> > > +    WEBKIT_POLICY_DECISION_TYPE_NEW_WINDOW_ACTION,
> > > +    WEBKIT_POLICY_DECISION_TYPE_RESPONSE,
> > > +} WebKitPolicyDecisionType;
> > 
> > Shouldn't this be in WebKitPolicyDecision.h?
> 
> It's only used in WebKitWebView.

It's used by WebView, but it logically belongs to PolicyDecision. WebKitWebView.h already includes WebKitPolicyDecision.h in your patch to use it.

[Carlos Garcia Campos]

(In reply to comment #14)
> (In reply to comment #12)
> 
> > In general, returning TRUE means that you have handled the signal and you don't want any other handlers to be invoked. In this case it doesn't matter whether you return TRUE or FALSE if you takes a reference. What I would expect is that if I return FALSE, the default handler ends up handling the default decision. If I return TRUE is because I want to handle the decision, so I can just do it and return or postpone it taking a reference of the decision object and returning TRUE.
> > 
> > > 2. We could wrap g_object_ref/g_object_unref in some friendler methods. I don't mind that, but this approch doesn't prevent it either.
> > 
> > I think It's more obvious that I have to take a reference of the decision object if I say somehow (returning TRUE) that I want to handle the decision.
> > 
> > > The nice thing about this design is that it enforces an important invariant: the WebKitPolicyListener never dies when a policy decision has not been made yet.
> > 
> > We can leave the _use() in the finalize, because it's protected by the decisionMade variable.
> 
> I'm not entirely convinced, but as you say it's not as common to handle these decisions asynchronously. So since you seem to feel strongly about this, I have taken your suggestion and made it a requirement to return TRUE here to handle decisions aysnchronously.

Not only to handle decisions aysnchronously, but also to handle then synchronously. Returning TRUE means you have handled the event (signal) and you don't want any other handlers to be invoked.

[Carlos Garcia Campos]

Comment on attachment 123985 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123985&action=review

Looks great, if I were a reviewer this would be a r+, please consider my comments before landing :-)

> Source/WebKit2/GNUmakefile.am:542
> +	Source/WebKit2/UIProcess/API/gtk/WebKitPolicyClientPrivate.h \

This is WebKitPolicyClient.h now

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:74
> +typedef enum {
> +    WEBKIT_POLICY_DECISION_TYPE_NAVIGATION_ACTION,
> +    WEBKIT_POLICY_DECISION_TYPE_NEW_WINDOW_ACTION,
> +    WEBKIT_POLICY_DECISION_TYPE_RESPONSE,
> +} WebKitPolicyDecisionType;

I still think this belongs to WebKitPolicyDecision.h

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:51
> +WebKitPolicyDecisionType

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitPolicyClient.cpp:88
> +        if (test->m_respondToPolicyDecisionAsynchronously) {
> +            g_idle_add(reinterpret_cast<GSourceFunc>(respondToPolicyDecisionLater), test);
> +            return FALSE;

You handled the signal here, so you should return TRUE to prevent other handlers to be invoked.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitPolicyClient.cpp:92
> +        respondToPolicyDecision(test, decision);
> +        return FALSE;

Ditto.

[Martin Robinson]

(In reply to comment #18)
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitPolicyClient.cpp:88
> > +        if (test->m_respondToPolicyDecisionAsynchronously) {
> > +            g_idle_add(reinterpret_cast<GSourceFunc>(respondToPolicyDecisionLater), test);
> > +            return FALSE;
> 
> You handled the signal here, so you should return TRUE to prevent other handlers to be invoked.

Actually this shouldn't matter since only the first policy decision should have any affect. I'll update the documentation to reflect this fact. Returning FALSE here serves as a test for that.

[Carlos Garcia Campos]

(In reply to comment #19)
> (In reply to comment #18)
> 
> > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitPolicyClient.cpp:88
> > > +        if (test->m_respondToPolicyDecisionAsynchronously) {
> > > +            g_idle_add(reinterpret_cast<GSourceFunc>(respondToPolicyDecisionLater), test);
> > > +            return FALSE;
> > 
> > You handled the signal here, so you should return TRUE to prevent other handlers to be invoked.
> 
> Actually this shouldn't matter since only the first policy decision should have any affect. I'll update the documentation to reflect this fact. Returning FALSE here serves as a test for that.

The fact that it doesn't matter in the end it's an implementation detail and it doesn't mena is correct. As I mentioned before, you should return TRUE from callbacks when you handled the thing to prevent other handlers to be invoked, even if the invocation of other handlers doesn't change anything. That's how gtk works, and that's why I've insisted in returning TRUE.

[Carlos Garcia Campos]

btw, we are already saying it in the docs:

Returns: %TRUE to stop other handlers from being invoked for the event.
   %FALSE to propagate the event further.

[Martin Robinson]

(In reply to comment #20)

> The fact that it doesn't matter in the end it's an implementation detail and it doesn't mena is correct. As I mentioned before, you should return TRUE from callbacks when you handled the thing to prevent other handlers to be invoked, even if the invocation of other handlers doesn't change anything. That's how gtk works, and that's why I've insisted in returning TRUE.

All of the pieces necessary to understand this code will be documented in the API.

1. The default signal handler has no side effects other than choosing the USE policy decision.
2. Only the first policy decision should matter.

Thus this is perfectly reasonable use of the API. It isn't an implementation detail -- it's a documented property of the API. Now perhaps the test could be more explicit about what it's testing, so I'll make it more obvious via comments and additional tests.

[Carlos Garcia Campos]

What I'm saying is that you should always return TRUE from the callback when you handle it, no matter whether the default handler affects it or not. If you return FALSE and we change the impl of the default handler in the future, for whatever reason, it might have odd effects in the behaviour. So, you should always return TRUE to make sure other handlers are not invoked.

[Carlos Garcia Campos]

Note, that this is not only about the default handler, a callback connected by the user with g_signal_connect_after would be invoked if you return FALSE.

[Martin Robinson]

(In reply to comment #23)
> What I'm saying is that you should always return TRUE from the callback when you handle it, no matter whether the default handler affects it or not. If you return FALSE and we change the impl of the default handler in the future, for whatever reason, it might have odd effects in the behaviour. So, you should always return TRUE to make sure other handlers are not invoked.

We cannot really change the behavior of the default handler because that would be an API break. I think it should remain consistent, so I've encoded the current behavior into the test. Thus it's important to return FALSE unless handling the decision asynchronously. I fixed the latter case.

[Martin Robinson]

(In reply to comment #24)
> Note, that this is not only about the default handler, a callback connected by the user with g_signal_connect_after would be invoked if you return FALSE.

This is a detail of some potential embedder and has no bearing on the test.

[Carlos Garcia Campos]

(In reply to comment #25)
> (In reply to comment #23)
> > What I'm saying is that you should always return TRUE from the callback when you handle it, no matter whether the default handler affects it or not. If you return FALSE and we change the impl of the default handler in the future, for whatever reason, it might have odd effects in the behaviour. So, you should always return TRUE to make sure other handlers are not invoked.
> 
> We cannot really change the behavior of the default handler because that would be an API break.

I mean in case the default handler does something else apart from calling_use(). That woulnd't be an API break.

> I think it should remain consistent, so I've encoded the current behavior into the test. Thus it's important to return FALSE unless handling the decision asynchronously. I fixed the latter case.

No, you have to return FALSE when you are not interested in the decision, so that you leave the action to the other handlers. This is how even propagation works in gtk+, it's not because I want it.

[Carlos Garcia Campos]

(In reply to comment #26)
> (In reply to comment #24)
> > Note, that this is not only about the default handler, a callback connected by the user with g_signal_connect_after would be invoked if you return FALSE.
> 
> This is a detail of some potential embedder and has no bearing on the test.

Tests should use the API in the right way even if doing it wrong doesn't affect the result.

[Martin Robinson]

Created attachment 124317 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 124317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124317&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:588
> +     *         /<!-- -->* Making no decision results in webkit_policy_decision_use(). *<!-- -->/
> +     *         break;

We should return FALSE here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:595
> +     * on the @decision argument and returning %TRUE to block the default signal handler.

to block other handlers

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:74
> +typedef enum {
> +    WEBKIT_POLICY_DECISION_TYPE_NAVIGATION_ACTION,
> +    WEBKIT_POLICY_DECISION_TYPE_NEW_WINDOW_ACTION,
> +    WEBKIT_POLICY_DECISION_TYPE_RESPONSE,
> +} WebKitPolicyDecisionType;

I still think this belongs to WebKitPolicyDecision.h, is there any reason why you think it should be defined here in webKitWebView?

> Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt:51
> +WebKitPolicyDecisionType

Ditto.

[Martin Robinson]

Comment on attachment 124317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124317&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:588
>> +     *         break;
> 
> We should return FALSE here.

It's still valid because the default action is USE no matter if the default signal handler fires or not.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:595
>> +     * on the @decision argument and returning %TRUE to block the default signal handler.
> 
> to block other handlers

Okay.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:74
>> +} WebKitPolicyDecisionType;
> 
> I still think this belongs to WebKitPolicyDecision.h, is there any reason why you think it should be defined here in webKitWebView?

It's only used in WebKitWebView. WebKitPolicyClient doesn't use it at all. It's weird to me that the abstract policy decision class "knows" about the different types of the decisions. To me it's a logical property of the WebView. I considered moving it, but it felt wrong.

[Carlos Garcia Campos]

(In reply to comment #31)
> (From update of attachment 124317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124317&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:588
> >> +     *         break;
> > 
> > We should return FALSE here.
> 
> It's still valid because the default action is USE no matter if the default signal handler fires or not.

In the other cases there's a comment saying that the decision is made, in this one the comment says "Making no decision", so you should return FALSE to let the default handler make the decision. 

> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:595
> >> +     * on the @decision argument and returning %TRUE to block the default signal handler.
> > 
> > to block other handlers
> 
> Okay.
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:74
> >> +} WebKitPolicyDecisionType;
> > 
> > I still think this belongs to WebKitPolicyDecision.h, is there any reason why you think it should be defined here in webKitWebView?
> 
> It's only used in WebKitWebView. WebKitPolicyClient doesn't use it at all. It's weird to me that the abstract policy decision class "knows" about the different types of the decisions. To me it's a logical property of the WebView. I considered moving it, but it felt wrong.

It's a policy decision type, it's used by web view, but it's about policy decision.

[Martin Robinson]

Comment on attachment 124317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124317&action=review

>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:588
>>>> +     *         break;
>>> 
>>> We should return FALSE here.
>> 
>> It's still valid because the default action is USE no matter if the default signal handler fires or not.
> 
> In the other cases there's a comment saying that the decision is made, in this one the comment says "Making no decision", so you should return FALSE to let the default handler make the decision.

The same decision is made either way. Should I made the documentation below clearer about this point?

[Carlos Garcia Campos]

(In reply to comment #33)
> (From update of attachment 124317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124317&action=review
> 
> >>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:588
> >>>> +     *         break;
> >>> 
> >>> We should return FALSE here.
> >> 
> >> It's still valid because the default action is USE no matter if the default signal handler fires or not.
> > 
> > In the other cases there's a comment saying that the decision is made, in this one the comment says "Making no decision", so you should return FALSE to let the default handler make the decision.
> 
> The same decision is made either way. Should I made the documentation below clearer about this point?

The doc is clear: 

Returns: %TRUE to stop other handlers from being invoked for the event. %FALSE to propagate the event further.

So simply return FALSE there.

[Martin Robinson]

(In reply to comment #34)

> > The same decision is made either way. Should I made the documentation below clearer about this point?
> 
> The doc is clear: 
> 
> Returns: %TRUE to stop other handlers from being invoked for the event. %FALSE to propagate the event further.
> 
> So simply return FALSE there.

I think the example illustrates an important property of the API.

[Carlos Garcia Campos]

(In reply to comment #35)
> (In reply to comment #34)
> 
> > > The same decision is made either way. Should I made the documentation below clearer about this point?
> > 
> > The doc is clear: 
> > 
> > Returns: %TRUE to stop other handlers from being invoked for the event. %FALSE to propagate the event further.
> > 
> > So simply return FALSE there.
> 
> I think the example illustrates an important property of the API.

I think it uses the api wrongly, even though the result is the same.

[Martin Robinson]

(In reply to comment #36)
> I think it uses the api wrongly, even though the result is the same.

I like the way the API is very flexible. This is a really minor point though. Perhaps we can take another pass through the API to clean up little details like these?

[Carlos Garcia Campos]

The point is that it's not following the event propagation approach of GTK. We are making the decision in finalize to prevent a misuse of it, but we shouldn't promote it in the api docs.

[Martin Robinson]

(In reply to comment #38)
> The point is that it's not following the event propagation approach of GTK. We are making the decision in finalize to prevent a misuse of it, but we shouldn't promote it in the api docs.

It has the property that it prevents mistakes, but it's also nice feature of the API, IMO.

[Carlos Garcia Campos]

And it's nice indeed, but we shouldn't promote it in the api docs, we already mention in the doc that use will be used anyway, so let's return FALSE when not handling it as expected.

[Martin Robinson]

(In reply to comment #40)
> And it's nice indeed, but we shouldn't promote it in the api docs, we already mention in the doc that use will be used anyway, so let's return FALSE when not handling it as expected.

I'd just like to respectfully disagree with your premise that is a misuse of the API. I think maybe we won't agree now about whether it is ugly or not. In my mind it's perfectly valid.

[Martin Robinson]

(In reply to comment #41)
> (In reply to comment #40)
> > And it's nice indeed, but we shouldn't promote it in the api docs, we already mention in the doc that use will be used anyway, so let's return FALSE when not handling it as expected.
> 
> I'd just like to respectfully disagree with your premise that is a misuse of the API. I think maybe we won't agree now about whether it is ugly or not. In my mind it's perfectly valid.

Another alternative is to make it so that this signal does not have a return value. That would remove all of these concerns entirely. We can simply say: "If you do not handle this signal, all policy decisions will be accepted."

[Gustavo Noronha (kov)]

Comment on attachment 124317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124317&action=review

Pretty good, thanks a lot Carlos for the thorough review =D

> Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp:28
> +using namespace WebCore;
> +

Is this needed? Seems like it's the only change in this file.

> Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:43
> +G_DEFINE_TYPE(WebKitPolicyDecision, webkit_policy_decision, G_TYPE_OBJECT)

Should this be G_DEFINE_ABSTRACT_TYPE?

>>>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:588
>>>>>> +     *         break;
>>>>> 
>>>>> We should return FALSE here.
>>>> 
>>>> It's still valid because the default action is USE no matter if the default signal handler fires or not.
>>> 
>>> In the other cases there's a comment saying that the decision is made, in this one the comment says "Making no decision", so you should return FALSE to let the default handler make the decision.
>> 
>> The same decision is made either way. Should I made the documentation below clearer about this point?
> 
> The doc is clear: 
> 
> Returns: %TRUE to stop other handlers from being invoked for the event. %FALSE to propagate the event further.
> 
> So simply return FALSE there.

I agree with Carlos in this matter. Returning TRUE without making a decision should better stay undefined behaviour. It is an implementation detail that people should not be relying on. I am not 100% on this, but I fear language bindings could mess up this expectation by keeping the object alive for longer, which would be another issue.

>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:595
>>>> +     * on the @decision argument and returning %TRUE to block the default signal handler.
>>> 
>>> to block other handlers
>> 
>> Okay.
> 
> It's a policy decision type, it's used by web view, but it's about policy decision.

So, I'm ambivalent in this matter, but I tend to take Martin's side, I think, in that it's the web view's use of this enum that gives any meaning to it. While the policy decision class is the mechanism used to operate on, it's the web view that knows what kinds of decisions it needs to be taken. But, that makes me think - would it be better to have the decisions (use, ignore, download) in the concrete classes that represent the different types of policy decisions? A download policy may not make sense for a new window policy decision, say  (although I may be forgetting some specificality of how this is handled inside webcore). In those cases you'd probably want to have an allow/deny decision instead of the three options that we currently have, and we could reuse this system for things like geolocation/webgl permissions. Food for thought, I don't think it's important enough to hold this patch, but we should consider that.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitPolicyClient.cpp:115
> +    // Ideally we'd like to have a more intensive test here, but it's still pretty tricky
> +    // to trigger different types of navigations with the GTK+ WebKit2 API.

We can probably trigger those by loading HTML with embedded js to simulate stuff? Gotta think a bit more about that.

[Martin Robinson]

Comment on attachment 124317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124317&action=review

>>>>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:588
>>>>>>> +     *         break;
>>>>>> 
>>>>>> We should return FALSE here.
>>>>> 
>>>>> It's still valid because the default action is USE no matter if the default signal handler fires or not.
>>>> 
>>>> In the other cases there's a comment saying that the decision is made, in this one the comment says "Making no decision", so you should return FALSE to let the default handler make the decision.
>>> 
>>> The same decision is made either way. Should I made the documentation below clearer about this point?
>> 
>> The doc is clear: 
>> 
>> Returns: %TRUE to stop other handlers from being invoked for the event. %FALSE to propagate the event further.
>> 
>> So simply return FALSE there.
> 
> I agree with Carlos in this matter. Returning TRUE without making a decision should better stay undefined behaviour. It is an implementation detail that people should not be relying on. I am not 100% on this, but I fear language bindings could mess up this expectation by keeping the object alive for longer, which would be another issue.

Okay.

>>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:595
>>>>> +     * on the @decision argument and returning %TRUE to block the default signal handler.
>>>> 
>>>> to block other handlers
>>> 
>>> Okay.
>> 
>> It's a policy decision type, it's used by web view, but it's about policy decision.
> 
> So, I'm ambivalent in this matter, but I tend to take Martin's side, I think, in that it's the web view's use of this enum that gives any meaning to it. While the policy decision class is the mechanism used to operate on, it's the web view that knows what kinds of decisions it needs to be taken. But, that makes me think - would it be better to have the decisions (use, ignore, download) in the concrete classes that represent the different types of policy decisions? A download policy may not make sense for a new window policy decision, say  (although I may be forgetting some specificality of how this is handled inside webcore). In those cases you'd probably want to have an allow/deny decision instead of the three options that we currently have, and we could reuse this system for things like geolocation/webgl permissions. Food for thought, I don't think it's important enough to hold this patch, but we should consider that.

I considered briefly having the different decisions return a boolean specifying whether or not the decision was valid or not. There is also a client method that can inform you if you've made an invalid policy decision. I think that "download" is invalid for "new-window" action for instance, but I'm not certain.

[Martin Robinson]

(In reply to comment #43)
> (From update of attachment 124317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124317&action=review
> 
> Pretty good, thanks a lot Carlos for the thorough review =D
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp:28
> > +using namespace WebCore;
> > +
> 
> Is this needed? Seems like it's the only change in this file.

I removed the WebCore namespace from ASSERT_MATCHING_ENUMS, so it's necessary here unless I add it to the enum values below.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitPolicyDecision.cpp:43
> > +G_DEFINE_TYPE(WebKitPolicyDecision, webkit_policy_decision, G_TYPE_OBJECT)
> 
> Should this be G_DEFINE_ABSTRACT_TYPE?

Yep! I'll fix before landing.

[Martin Robinson]

Committed r106142: <http://trac.webkit.org/changeset/106142>