Bug 15984 – Implement "navigation-requested" signal for WebKit Gtk

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

[Michael Natterer]

The GTK+ frontend lacks the implementation of the "navigation-requested"
signal. Patch follows.

[Michael Natterer]

Created attachment 17267 [details]
Patch implementing the above

- implement WebKitNetworkRequest
- made WebKitPage::navigation-requested a signal
- fix enum name:
  s/WEBKIT_NAVIGATION_REQUEST_RESPONSE/WebkitNavigationRequestResponse/
- emit it when navigation is requested
- in GtkLauncher, connect to WebKitPage::navigation-requested

[Alp Toker]

Hey,

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

Thanks!

[Alp Toker]

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.

[Alp Toker]

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.

[Michael Natterer]

Created attachment 17432 [details]
updated patch

Fixes the enum name and moves typedefs to right file.

[Alp Toker]

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 {
>     WEBKIT_ACCEPT_NAVIGATION_REQUEST,
>     WEBKIT_IGNORE_NAVIGATION_REQUEST,
>     WEBKIT_DOWNLOAD_NAVIGATION_REQUEST
>-} WEBKIT_NAVIGATION_REQUEST_RESPONSE;
>+} WebKitNavigationRequestResponse;

typedef enum {
     WEBKIT_NAVIGATION_RESPONSE_ACCEPT,
     WEBKIT_NAVIGATION_RESPONSE_IGNORE,
     WEBKIT_NAVIGATION_RESPONSE_DOWNLOAD
} 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.

[Michael Natterer]

I completely agree with this change, I just kept the names as they were before :)

[Alp Toker]

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.

[Alp Toker]

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?

Thanks!

[Michael Natterer]

Created attachment 17442 [details]
Updated patch

This one has the enum values sanitized, uses the right ChangeLogs
and is against latest upstream.

[Alp Toker]

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.

[Alp Toker]

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.

Thanks!