Bug 15984

Summary: Implement "navigation-requested" signal for WebKit Gtk
Product: WebKit Reporter: Michael Natterer <mitch>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Severity: Enhancement CC: alp, cosimoc
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Description Flags
Patch implementing the above
updated patch
alp: review-
Updated patch alp: review+

Description Michael Natterer 2007-11-14 04:40:43 PST
The GTK+ frontend lacks the implementation of the "navigation-requested"
signal. Patch follows.
Comment 1 Michael Natterer 2007-11-14 06:12:07 PST
Created attachment 17267 [details]
Patch implementing the above

- implement WebKitNetworkRequest
- made WebKitPage::navigation-requested a signal
- fix enum name:
- emit it when navigation is requested
- in GtkLauncher, connect to WebKitPage::navigation-requested
Comment 2 Alp Toker 2007-11-20 11:28:23 PST

Don't forget to mark patches for review otherwise they won't get looked at promptly.

Comment 3 Alp Toker 2007-11-20 14:34:31 PST
At first glance I noticed the capitalisation of WebkitNavigationRequestResponse is wrong. Some gtkdoc would be nice too, but that can be added after this lands I guess.

Will look at it more closely soon.
Comment 4 Alp Toker 2007-11-20 15:05:43 PST
I Have a feeling this set of policy decisions should be made through WebKitFrame, not WebKitPage. That's how the Win port does it. WebKitPage is just the view widget the way I see things.

Did you follow the example of the Qt port here rather than Win? I wonder what the merits of the two approaches are -- we need to choose one and stick to it.

I think the request class will be mostly a wrapper around ResourceRequest, though just carrying around a URL string is OK for an initial implementation.
Comment 5 Michael Natterer 2007-11-21 08:55:48 PST
Created attachment 17432 [details]
updated patch

Fixes the enum name and moves typedefs to right file.
Comment 6 Alp Toker 2007-11-21 09:27:03 PST
Comment on attachment 17432 [details]
updated patch

>diff --git a/WebKit/gtk/Api/webkitgtkpage.h b/WebKit/gtk/Api/webkitgtkpage.h
>index 23215b4..920ca7c 100644
>--- a/WebKit/gtk/Api/webkitgtkpage.h
>+++ b/WebKit/gtk/Api/webkitgtkpage.h
>@@ -48,7 +48,7 @@ typedef enum {
>+} WebKitNavigationRequestResponse;

typedef enum {
} WebKitNavigationResponse;

My suggestion looks more conventional. "RequestResponse" sounds redundant, and I wasn't too sure about naming of the enum members. What do you think of this change?

Looks good otherwise.
Comment 7 Michael Natterer 2007-11-21 10:41:32 PST
I completely agree with this change, I just kept the names as they were before :)
Comment 8 Alp Toker 2007-11-21 10:54:45 PST
No API stability yet. Now's the time to make this API as elegant and succinct as possible before it starts to freeze. Waiting for your changes.
Comment 9 Alp Toker 2007-11-21 18:41:24 PST
Comment on attachment 17432 [details]
updated patch

Please update the enum name and order of the element names as we discussed.

I tried to apply this patch and it does not compile. Can you test against a non-git checkout to make sure the next revision applies cleanly and builds?

Also, could you use the closest available ChangeLog file rather than the root ChangeLog?

Comment 10 Michael Natterer 2007-11-22 08:50:16 PST
Created attachment 17442 [details]
Updated patch

This one has the enum values sanitized, uses the right ChangeLogs
and is against latest upstream.
Comment 11 Alp Toker 2007-11-22 17:50:20 PST
Comment on attachment 17442 [details]
Updated patch

Thanks Mitch!

I'm landing this (with a couple of whitespace fixes).

It'll become apparent whether this API is good for the long term as we start to add support for more delegates and get more applications trying to use them.
Comment 12 Alp Toker 2007-11-22 17:56:08 PST
All of the patch except the GtkLauncher change landed in r27976.

GtkLauncher shouldn't be so chatty on the console, but it'd still be nice to provide example code for this feature.

Mitch: Looks like a _DOWNLOAD response may not be handled by your code. Would it be correct to use a switch here and take that case into account as well? If so, please submit a follow up patch.

We also need to start looking at how we can provide more context for policy decisions now.