[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 #6 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-05-22 23:41:36 PST ---
(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.

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