[webkit-reviews] review granted: [Bug 76343] [GTK] [WK2] Implement the policy client : [Attachment 124317] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 76343: [GTK] [WK2] Implement the policy client
https://bugs.webkit.org/show_bug.cgi?id=76343

Attachment 124317: Patch
https://bugs.webkit.org/attachment.cgi?id=124317&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.