[Webkit-unassigned] [Bug 83681] [GTK] Add WebKitWebResource::send-request signal to WebKit2 GTK+ API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=83681





--- Comment #5 from Martin Robinson <mrobinson at webkit.org>  2012-05-22 17:46:49 PST ---
(From update of attachment 136652)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.