Bug 27792 – [GTK] new-window-policy-decision-requested provides no information about the target frame

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

[Martin Robinson]

new-window-policy-decision-requested should include the contents of the target attribute of the link which spawned the policy decision.

[Martin Robinson]

Created attachment 33692 [details]
Patch adding a frame name property to the WebKitWebNavigation object

Added a patch addressing this issue.

[Xan Lopez]

First of all, I guess it's reasonable to add the target frame in the NavigationAction object (the other possibility without breaking API is the NetworkRequest, which sounds like a worse place), what do the other reviewers think?

Second, about the patch, I think the target frame property should be CONSTRUCT_ONLY, and the set_target_frame function shouldn't be public, since this information won't change during the life cycle of the navigation action object.

Other than that this looks reasonable for me. Since it's new API we need the input of another reviewer to get this in, CCing some :)

[Xan Lopez]

Also, for the future, you need to set the review flag to '?' so that it appears in the review queue (webkit.org/pending-review).

[Jan Alonzo]

(In reply to comment #2)
> First of all, I guess it's reasonable to add the target frame in the
> NavigationAction object (the other possibility without breaking API is the
> NetworkRequest, which sounds like a worse place), what do the other reviewers
> think?

I rather have it in the signal. I couldn't see how else we can do this so +1 from me.

[Martin Robinson]

Created attachment 33715 [details]
Updated patch

Added a patch with the changes suggested.

[Xan Lopez]

(In reply to comment #5)
> Created an attachment (id=33715) [details]
> Updated patch
> 
> Added a patch with the changes suggested.

Only two nitpicks:

- The set_target_frame function should be static now.
- Maybe we should set the target frame to 'NULL' to indicate that it's not set, instead of ""?

Other than that it looks good to me. Since this adds new API I'll let another reviewer give the final r+.

[Gustavo Noronha (kov)]

Comment on attachment 33715 [details]
Updated patch

> + * Since: 1.1.12

Should be 1.1.13, now =).

> +void webkit_web_navigation_action_set_target_frame(WebKitWebNavigationAction* navigationAction, const gchar* targetFrame)
> +{
> +    g_return_if_fail(WEBKIT_IS_WEB_NAVIGATION_ACTION(navigationAction));

No need for this check, after making it static.

> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,14 @@
> [phone]  Martin Robinson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [GTK] new-window-policy-decision-requested provides no information about the target frame
> +        https://bugs.webkit.org/show_bug.cgi?id=27792
> +
> +        * GtkLauncher/main.c:
> +        (policy_callback):
> +        (create_browser):
> +

The ChangeLog entry should be in WebKit/gtk/ChangeLog, and list the correct funtions =).

I'm OK with the approach and with the API additions. I'll say r-, though, because I see too many nits to be addressed, and because of the ChangeLog issue, but feel free to r+ the next patch, Jan, or Xan.

[Martin Robinson]

Created attachment 33779 [details]
Updated patch 2

Whoops. Added an updated patch fixing these issues.

[Jan Alonzo]

Comment on attachment 33779 [details]
Updated patch 2

> +    g_object_class_install_property(objectClass, PROP_TARGET_FRAME,
> +                                    g_param_spec_string("target-frame",
> +                                                        _("Target frame"),
> +                                                        _("The target frame for the navigation"),
> +                                                        "",
> +                                                        (GParamFlags)(WEBKIT_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));

This should be initialized to NULL.

r=me. I'll fixed the nit prior to landing.

[Jan Alonzo]

(In reply to comment #9)
> (From update of attachment 33779 [details])
> > +    g_object_class_install_property(objectClass, PROP_TARGET_FRAME,
> > +                                    g_param_spec_string("target-frame",
> > +                                                        _("Target frame"),
> > +                                                        _("The target frame for the navigation"),
> > +                                                        "",
> > +                                                        (GParamFlags)(WEBKIT_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)));
> 
> This should be initialized to NULL.
> 
> r=me. I'll fixed the nit prior to landing.

Landed as http://trac.webkit.org/changeset/46622. 

I simplified the strcmp expression in set_target_frame as suggested by Xan in IRC. I also changed the return type of get_target_frame from "const char*" to "G_CONST_RETURN gchar*" to make it consistent with most of WebKitGtk API that return a "const char*".