Bug 83681 - [GTK] Add WebKitWebPage::send-request signal to WebKit2 GTK+ API
Summary: [GTK] Add WebKitWebPage::send-request signal to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
: 111620 (view as bug list)
Depends on: 110614
Blocks: 112160
  Show dependency treegraph
 
Reported: 2012-04-11 01:59 PDT by Carlos Garcia Campos
Modified: 2013-04-18 09:11 PDT (History)
9 users (show)

See Also:


Attachments
Patch (51.24 KB, patch)
2012-04-11 02:27 PDT, Carlos Garcia Campos
gns: commit-queue-
Details | Formatted Diff | Diff
Updated patch to fix the build (51.93 KB, patch)
2012-04-11 03:20 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch to move the API to the injected bundle (18.85 KB, patch)
2013-02-28 06:57 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (31.94 KB, patch)
2013-03-14 03:37 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (32.11 KB, patch)
2013-03-14 09:03 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased to current git master (32.22 KB, patch)
2013-04-18 07:57 PDT, Carlos Garcia Campos
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-04-11 01:59:07 PDT
To allow modifying the uri request of a resource before it's sent to the server or even cancel the load of a resource.
Comment 1 Carlos Garcia Campos 2012-04-11 02:27:46 PDT
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.
Comment 2 WebKit Review Bot 2012-04-11 02:30:39 PDT
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
Comment 3 Gustavo Noronha (kov) 2012-04-11 02:39:23 PDT
Comment on attachment 136648 [details]
Patch

Attachment 136648 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12379970
Comment 4 Carlos Garcia Campos 2012-04-11 03:20:18 PDT
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.
Comment 5 Martin Robinson 2012-05-22 17:46:49 PDT
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.
Comment 6 Carlos Garcia Campos 2012-05-22 23:41:36 PDT
(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.
Comment 7 Martin Robinson 2012-05-23 08:35:28 PDT
(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.
Comment 8 sasha1024 2012-05-24 12:03:53 PDT
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?)
Comment 9 Carlos Garcia Campos 2012-05-24 12:08:37 PDT
(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.
Comment 10 Carlos Garcia Campos 2013-02-28 06:57:08 PST
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
Comment 11 Martin Robinson 2013-03-04 11:36:56 PST
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.
Comment 12 Carlos Garcia Campos 2013-03-05 01:43:42 PST
(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
Comment 13 Manuel Rego Casasnovas 2013-03-07 00:08:38 PST
*** Bug 111620 has been marked as a duplicate of this bug. ***
Comment 14 Carlos Garcia Campos 2013-03-14 03:37:06 PDT
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.
Comment 15 Gustavo Noronha (kov) 2013-03-14 06:45:53 PDT
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
Comment 16 Carlos Garcia Campos 2013-03-14 09:03:06 PDT
Created attachment 193133 [details]
Updated patch

Renames WebKitForward.h as WEbKirForwardDeclarations.h as suggested by Gustavo.
Comment 17 Carlos Garcia Campos 2013-04-15 00:39:54 PDT
Bug #110614 is now fixed, so this could land. Ping owners.
Comment 18 Carlos Garcia Campos 2013-04-18 07:57:00 PDT
Created attachment 198735 [details]
Rebased to current git master
Comment 19 Anders Carlsson 2013-04-18 08:52:27 PDT
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.
Comment 20 Carlos Garcia Campos 2013-04-18 09:11:09 PDT
Committed r148679: <http://trac.webkit.org/changeset/148679>