Bug 27792 - [GTK] new-window-policy-decision-requested provides no information about the target frame
Summary: [GTK] new-window-policy-decision-requested provides no information about the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2009-07-28 22:07 PDT by Martin Robinson
Modified: 2009-07-30 23:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch adding a frame name property to the WebKitWebNavigation object (9.28 KB, patch)
2009-07-28 22:12 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Updated patch (8.66 KB, patch)
2009-07-29 08:10 PDT, Martin Robinson
gns: review-
Details | Formatted Diff | Diff
Updated patch 2 (9.35 KB, patch)
2009-07-30 07:56 PDT, Martin Robinson
jmalonzo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2009-07-28 22:07:42 PDT
new-window-policy-decision-requested should include the contents of the target attribute of the link which spawned the policy decision.
Comment 1 Martin Robinson 2009-07-28 22:12:29 PDT
Created attachment 33692 [details]
Patch adding a frame name property to the WebKitWebNavigation object

Added a patch addressing this issue.
Comment 2 Xan Lopez 2009-07-29 02:43:03 PDT
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 :)
Comment 3 Xan Lopez 2009-07-29 02:44:08 PDT
Also, for the future, you need to set the review flag to '?' so that it appears in the review queue (webkit.org/pending-review).
Comment 4 Jan Alonzo 2009-07-29 03:20:56 PDT
(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.
Comment 5 Martin Robinson 2009-07-29 08:10:32 PDT
Created attachment 33715 [details]
Updated patch

Added a patch with the changes suggested.
Comment 6 Xan Lopez 2009-07-30 01:12:32 PDT
(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+.
Comment 7 Gustavo Noronha (kov) 2009-07-30 05:04:25 PDT
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 @@
> +2009-07-29  Martin Robinson  <martin.james.robinson@gmail.com>
> +
> +        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.
Comment 8 Martin Robinson 2009-07-30 07:56:17 PDT
Created attachment 33779 [details]
Updated patch 2

Whoops. Added an updated patch fixing these issues.
Comment 9 Jan Alonzo 2009-07-30 17:08:54 PDT
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.
Comment 10 Jan Alonzo 2009-07-30 23:18:36 PDT
(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*".