Bug 83681 – [GTK] Add WebKitWebPage::send-request signal to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

To allow modifying the uri request of a resource before it's sent to the server or even cancel the load of a resource.

[Carlos Garcia Campos]

Created attachment 136648 [details]
Patch

willSendRequestForFrame is only available in the injected bundle API, so this patch introduces injected bundle support in WebKit2 GTK+ API.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Gustavo Noronha (kov)]

Comment on attachment 136648 [details]
Patch

Attachment 136648 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12379970

[Carlos Garcia Campos]

Created attachment 136652 [details]
Updated patch to fix the build

Make gtk-doc ignore files in WebKitInjectedBundle dir since it doesn't contain public API.

[Martin Robinson]

Comment on attachment 136652 [details]
Updated patch to fix the build

View in context: https://bugs.webkit.org/attachment.cgi?id=136652&action=review

This patch seems fine. Before we go too far down this path though, I think we should ensure that there is not an easier way to do this than the injected bundle. We have added custom IPC messages in the past, so why is it not possible in this case?

I have just a few small comments otherwise.

> Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundle/GNUmakefile.am:24
> +webbundledir = $(libdir)/webkit2gtk-3.0/$(WEBKITGTK_API_VERSION)/webbundle
> +webbundle_LTLIBRARIES = libwebkit2gtkwebbundle.la
> +
> +libwebkit2gtkwebbundle_la_SOURCES = \
> +	Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundle/WebKitInjectedBundle.cpp \
> +	Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundle/WebKitInjectedBundle.h \
> +	Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundle/WebKitInjectedBundleMain.cpp
> +
> +libwebkit2gtkwebbundle_la_LDFLAGS = \
> +	$(no_undefined) \
> +	-module \
> +	-avoid-version
> +
> +libwebkit2gtkwebbundle_la_CPPFLAGS = \
> +	-fno-strict-aliasing \
> +	-I$(top_builddir)/DerivedSources/InjectedBundle \
> +	-I$(top_builddir)/DerivedSources/WebKit2/include \
> +	$(global_cppflags) \
> +	$(javascriptcore_cppflags) \
> +	$(GLIB_CFLAGS)
> +
> +endif # ENABLE_WEBKIT2

webbundle is a bit ambiguous. I think I'd prefer libwebkit2gtkinjectedbundle.

> Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:151
> + * Set the URI of @request

I think this could be expanded quite a bit. It makes sense to explain here at what point this can change the request, for instance "only when handling the send-request" signal.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:289
> +    if (WKStringIsEqualToUTF8CString(messageName, "WebKitWebView")) {

I think it might be better here just to do:

ASSERT(WKStringIsEqualToUTF8CString(messageName, "WebKitWebView"))

and avoid the conditional.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:971
> +    if (WKStringIsEqualToUTF8CString(messageName, "WillSendRequestForFrame")) {

Again until we have another message, maybe just add an ASSERT and avoid the conditional.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 136652 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136652&action=review
> 
> This patch seems fine. Before we go too far down this path though, I think we should ensure that there is not an easier way to do this than the injected bundle. We have added custom IPC messages in the past, so why is it not possible in this case?

We have added IPC messages for things that are specific to our port, when there isn't even injected bundle api for that, this is a different case. There's injected bundle api for this.

> I have just a few small comments otherwise.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundle/GNUmakefile.am:24
> > +webbundledir = $(libdir)/webkit2gtk-3.0/$(WEBKITGTK_API_VERSION)/webbundle
> > +webbundle_LTLIBRARIES = libwebkit2gtkwebbundle.la
> > +
> > +libwebkit2gtkwebbundle_la_SOURCES = \
> > +	Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundle/WebKitInjectedBundle.cpp \
> > +	Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundle/WebKitInjectedBundle.h \
> > +	Source/WebKit2/UIProcess/API/gtk/WebKitInjectedBundle/WebKitInjectedBundleMain.cpp
> > +
> > +libwebkit2gtkwebbundle_la_LDFLAGS = \
> > +	$(no_undefined) \
> > +	-module \
> > +	-avoid-version
> > +
> > +libwebkit2gtkwebbundle_la_CPPFLAGS = \
> > +	-fno-strict-aliasing \
> > +	-I$(top_builddir)/DerivedSources/InjectedBundle \
> > +	-I$(top_builddir)/DerivedSources/WebKit2/include \
> > +	$(global_cppflags) \
> > +	$(javascriptcore_cppflags) \
> > +	$(GLIB_CFLAGS)
> > +
> > +endif # ENABLE_WEBKIT2
> 
> webbundle is a bit ambiguous. I think I'd prefer libwebkit2gtkinjectedbundle.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:151
> > + * Set the URI of @request
> 
> I think this could be expanded quite a bit. It makes sense to explain here at what point this can change the request, for instance "only when handling the send-request" signal.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:289
> > +    if (WKStringIsEqualToUTF8CString(messageName, "WebKitWebView")) {
> 
> I think it might be better here just to do:
> 
> ASSERT(WKStringIsEqualToUTF8CString(messageName, "WebKitWebView"))
> 
> and avoid the conditional.

This is WebContext, it might receive messages for other objects in the future.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:971
> > +    if (WKStringIsEqualToUTF8CString(messageName, "WillSendRequestForFrame")) {
> 
> Again until we have another message, maybe just add an ASSERT and avoid the conditional.

Ah, until we have another message, I still think it's confusing, it might me think the context can only receive messages from the view.

[Martin Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 136652 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136652&action=review
> > 
> > This patch seems fine. Before we go too far down this path though, I think we should ensure that there is not an easier way to do this than the injected bundle. We have added custom IPC messages in the past, so why is it not possible in this case?
> 
> We have added IPC messages for things that are specific to our port, when there isn't even injected bundle api for that, this is a different case. There's injected bundle api for this.

But as you explained in your email to the list, creating an injected bundle for the API  has the disadvantage of disallowing another injected bundle. If creating an IPC message for this API is an option, I think it would make the patch a lot simpler. If creating an IPC for this API isn't an option, the injected bundle seems like our only recourse.

It'd be interesting to hear if the Mac WebKit2 developers have an opinion on this issue.

Sorry for possibly stupid question, but what is the difference between this signal and resource-load-started signal in WebKitWebView?

Both of them seem to provide WebKitURIRequest parameter, thus it seems that I can change URI in both of them. (Or resource-load-started signal is emitted too late for that?)

[Carlos Garcia Campos]

(In reply to comment #8)
> Sorry for possibly stupid question, but what is the difference between this signal and resource-load-started signal in WebKitWebView?
> 
> Both of them seem to provide WebKitURIRequest parameter, thus it seems that I can change URI in both of them. (Or resource-load-started signal is emitted too late for that?)

In resource-load-started the request argument is supposed to be read-only, you can't change the request, or at least it won't have any effect.

[Carlos Garcia Campos]

Created attachment 190719 [details]
Updated patch to move the API to the injected bundle

Expose the signal in WebKitWebPage so that web process extensions can use it without any IPC message involved. This should apply on top of patch attached to bug #110614

[Martin Robinson]

Comment on attachment 190719 [details]
Updated patch to move the API to the injected bundle

View in context: https://bugs.webkit.org/attachment.cgi?id=190719&action=review

Looks good to me. :)

> Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:133
> + * Set the URI of @request

I still think it would be good here to say explicitly that this will only affect the request in the send-request signal handler.

> Source/WebKit2/UIProcess/API/gtk/tests/TestResources.cpp:625
> +    events = test->m_loadEvents;

I don't think you need to set this reference again actually.

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 190719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190719&action=review
> 
> Looks good to me. :)
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitURIRequest.cpp:133
> > + * Set the URI of @request
> 
> I still think it would be good here to say explicitly that this will only affect the request in the send-request signal handler.

Not necessarily, you could change the uri to reuse a request object to be loaded with webkit_web_view_load_request. We could say that this won't have any effect for a request already sent to the server, for example, or something like that, but I think it's pretty obvious.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestResources.cpp:625
> > +    events = test->m_loadEvents;
> 
> I don't think you need to set this reference again actually.

You are right

[Manuel Rego Casasnovas]

*** Bug 111620 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 193102 [details]
Updated patch

New patch addressing review comments and fixing a build error due to the shared headers. See the changelog for more details.

[Gustavo Noronha (kov)]

Comment on attachment 193102 [details]
Updated patch

The header changes look good to me, I'd name the new header WebKitForwardDeclarations to make it more obvious, though

[Carlos Garcia Campos]

Created attachment 193133 [details]
Updated patch

Renames WebKitForward.h as WEbKirForwardDeclarations.h as suggested by Gustavo.

[Carlos Garcia Campos]

Bug #110614 is now fixed, so this could land. Ping owners.

[Carlos Garcia Campos]

Created attachment 198735 [details]
Rebased to current git master

[Anders Carlsson]

Comment on attachment 198735 [details]
Rebased to current git master

View in context: https://bugs.webkit.org/attachment.cgi?id=198735&action=review

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:140
> +    WebURLRequest* newRequest = WebURLRequest::create(webkitURIRequestGetResourceRequest(request.get())).leakRef();

I think you should put this in a RefPtr instead if calling leakRef() here.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:150
> +    return toAPI(newRequest);

Then you can call release().leakRef() here.

[Carlos Garcia Campos]

Committed r148679: <http://trac.webkit.org/changeset/148679>