Bug 26854 – [GTK] Needs API to allow more control over outgoing requests

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

[Gustavo Noronha (kov)]

Currently we lack ways of controlling the outgoing requests. We cannot modify headers, or other properties of the request, because none of the signals we currently provide in our API allow that. We specially need to implement the dispatchWillSendRequest delegate.

[Gustavo Noronha (kov)]

Created attachment 32084 [details]
Map the willSendRequest delegate to the 'outgoing-request' signal

 WebKit/gtk/ChangeLog                               |   16 +++++++++++
 WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp |   11 ++++++-
 WebKit/gtk/webkit/webkitwebview.cpp                |   28 ++++++++++++++++++++
 WebKit/gtk/webkitmarshal.list                      |    1 +
 4 files changed, 54 insertions(+), 2 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 32084 [details]
Map the willSendRequest delegate to the 'outgoing-request' signal

A first try.

[Gustavo Noronha (kov)]

Created attachment 32085 [details]
Basic adblocking

 embed/ephy-embed.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 32085 [details]
Basic adblocking

This demonstrates how the new API could be used to implement adblocking in Epiphany. It's far from finished, but serves as a good example, and should allow us to improve the proposed API.

[Jan Alonzo]

Comment on attachment 32084 [details]
Map the willSendRequest delegate to the 'outgoing-request' signal

> -void FrameLoaderClient::dispatchWillSendRequest(DocumentLoader*, unsigned long, ResourceRequest&, const ResourceResponse&)
> +void FrameLoaderClient::dispatchWillSendRequest(DocumentLoader*, unsigned long identifier, ResourceRequest& request, const ResourceResponse&)
>  {
> -    notImplemented();
> +    WebKitWebView* webView = getViewFromFrame(m_frame);
> +    WebKitNetworkRequest* networkRequest = webkit_network_request_new_with_core_request(request);
> +
> +    g_signal_emit_by_name(webView, "outgoing-request", m_frame, identifier, networkRequest);

We probably need to send the data source object as well and possibly make identifier an attribute of that object.

As for naming, I'm not sure what outgoing-request means. I think it needs to be more meaningful.

> +     * Emitted when a request is about to be sent. You can modify the
> +     * request, by adding/removing/replacing headers, or changing the
> +     * URI, using the #SoupMessage object it carries, if it is

Will changing the URI have any effect on security?

> +    webkit_web_view_signals[OUTGOING_REQUEST] = g_signal_new("outgoing-request",
> +            G_TYPE_FROM_CLASS(webViewClass),
> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
> +            0,
> +            NULL, NULL,
> +            webkit_marshal_VOID__OBJECT_OBJECT_ULONG,
> +            G_TYPE_NONE, 3,
> +            WEBKIT_TYPE_WEB_FRAME,
> +            G_TYPE_ULONG,
> +            WEBKIT_TYPE_NETWORK_REQUEST);

The marshaller seems to be off here.

Also, the DRT support for this is missing + the layout tests that will be enabled by this feature.

r- for now because of this.

[Gustavo Noronha (kov)]

(In reply to comment #5)
> > +    WebKitWebView* webView = getViewFromFrame(m_frame);
> > +    WebKitNetworkRequest* networkRequest = webkit_network_request_new_with_core_request(request);
> > +
> > +    g_signal_emit_by_name(webView, "outgoing-request", m_frame, identifier, networkRequest);
> 
> We probably need to send the data source object as well and possibly make
> identifier an attribute of that object.

I don't think there's a data source object here yet, is there? I think we may want to keep the identifier out of any objects.

> As for naming, I'm not sure what outgoing-request means. I think it needs to be
> more meaningful.

We need a way of expressing 'a request is going to be sent', outgoing-request was the first thing I could think of.

> > +     * Emitted when a request is about to be sent. You can modify the
> > +     * request, by adding/removing/replacing headers, or changing the
> > +     * URI, using the #SoupMessage object it carries, if it is
> 
> Will changing the URI have any effect on security?

It could, so it's best that the application know what it is doing =).

> The marshaller seems to be off here.

danw pointed that out, thanks =)

> Also, the DRT support for this is missing + the layout tests that will be
> enabled by this feature.

What kind of DRT support, do you mean? I don't think we will have layout tests enabled by this feature, too. This is purely API-related, AFAIK.

> 
> r- for now because of this.
> 

[Gustavo Noronha (kov)]

Created attachment 32134 [details]
Map the willSendRequest delegate to the 'outgoing-request' signal

 LayoutTests/ChangeLog                              |   12 +++++
 LayoutTests/platform/gtk/Skipped                   |    2 -
 WebKit/gtk/ChangeLog                               |   17 ++++++
 WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp |   15 +++++-
 WebKit/gtk/webkit/webkitwebview.cpp                |   52 ++++++++++++++++++++
 WebKit/gtk/webkitmarshal.list                      |    2 +
 WebKitTools/ChangeLog                              |   15 ++++++
 WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp  |   40 +++++++++++++++
 8 files changed, 150 insertions(+), 5 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 32134 [details]
Map the willSendRequest delegate to the 'outgoing-request' signal

After Jan pointed out that Mac used willSendRequest in its DRT implementation I went and investigated. I now believe we need to look at this problem with a wider perspective, and with the DataSource API in mind.

Delegates such as willSendRequest,  didFinishLoading, and other (I believe all that get a DocumentLoader and an identifier, in FrameLoaderClient) are used together for tracking the loading of the resources.

Mac provides as part of its API objects representing the resources, and maps the identifier to said objects in these delegates. We may want to adopt a similar approach.

[Jan Alonzo]

Comment on attachment 32134 [details]
Map the willSendRequest delegate to the 'outgoing-request' signal

> -void FrameLoaderClient::dispatchWillSendRequest(DocumentLoader*, unsigned long, ResourceRequest&, const ResourceResponse&)
> +void FrameLoaderClient::dispatchWillSendRequest(DocumentLoader*, unsigned long identifier, ResourceRequest& request, const ResourceResponse&)
>  {
> -    notImplemented();
> +    WebKitWebView* webView = getViewFromFrame(m_frame);
> +    WebKitNetworkRequest* networkRequest = webkit_network_request_new_with_core_request(request);
> +
> +    g_signal_emit_by_name(webView, "outgoing-request", m_frame, identifier, networkRequest);

I think instead of sending the identifier, we can do a WebResource lookup by identifier and send the resource instead. Thoughts?

>  void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long identifier, DocumentLoader*, const ResourceRequest&)
> @@ -860,7 +867,9 @@ void FrameLoaderClient::dispatchDidReceiveContentLength(DocumentLoader*, unsigne
>  
>  void FrameLoaderClient::dispatchDidFinishLoading(DocumentLoader*, unsigned long identifier)
>  {
> -    notImplemented();
> +    WebKitWebView* webView = getViewFromFrame(m_frame);
> +
> +    g_signal_emit_by_name(webView, "finished-request", m_frame, identifier);

I think we might need to prefix these resource-related signal names with resource- to avoid confusion with frame load events. So for example resource-will-send-request, resource-load-finished, etc..

> +     * WebKitWebView::outgoing-request:
> +     * @web_view: the object which received the signal
> +     * @web_frame: the #WebKitWebFrame whose load dispatched this request
> +     * @identifier: a numeric identifier for the resource that is
> +     * being loaded; in the future you will be able to use it with the
> +     * DataSource API, to track resource loading

I think the DS API is nearly there so instead of identifier, we can send the webresource instead as well as the datasource? Thoughts?

> +    /**
> +     * WebKitWebView::finished-request:
> +     * @web_view: the object which received the signal
> +     * @web_frame: the #WebKitWebFrame whose load dispatched this request
> +     * @identifier: a numeric identifier for the resource that is
> +     * being loaded; in the future you will be able to use it with the
> +     * DataSource API, to track resource loading

Ditto.

Patch looks good. Would be nice if we can agree on the identifier -> webresource approach soon.

[Xan Lopez]

Comment on attachment 32134 [details]
Map the willSendRequest delegate to the 'outgoing-request' signal

I'm r- this for now since I think it's clear we'll want to use the DS stuff in one way or the other when it's done.

[Gustavo Noronha (kov)]

(In reply to comment #9)
> I think instead of sending the identifier, we can do a WebResource lookup by
> identifier and send the resource instead. Thoughts?

That sounds good to me, yeah =D

[Gustavo Noronha (kov)]

Created attachment 39055 [details]
first draft post-DS

I'm setting the review flag just to get comments on the patch, and on the API. The patch still needs work, and we are probably going to add a WebKitNetworkResponse object to forward the response parameter willSendRequest gets.

[Gustavo Noronha (kov)]

Created attachment 39061 [details]
second draft post-DS; hah, now with response

I didn't think I'd be able to add response before going for lunch, but here it is. The Launcher patch is just for playing with the API, of course; I think it is good to enlighten how it is supposed to work. Comments, please! =)

[Gustavo Noronha (kov)]

Created attachment 39076 [details]
the traditional Epiphany implementation as an example

[Gustavo Noronha (kov)]

Created attachment 39081 [details]
third draft, with some comments by Xan addressed, and no load-status/load-error for resources

[Gustavo Noronha (kov)]

Created attachment 39082 [details]
load-status and load-error implementation for resource loading

This is the second part of the patch, split out so they can be reviewed and get in at separate times.

[Xan Lopez]

Comment on attachment 39081 [details]
third draft, with some comments by Xan addressed, and no load-status/load-error for resources

> +#include "config.h"
> +#include "ResourceResponse.h"
> +
> +#include "CString.h"
> +#include "GOwnPtr.h"
> +#include "PlatformString.h"

I'd say some of these are unused here?

> +
> +#include <libsoup/soup.h>

And this one is included in ResourceResponse.h already?

> +
> +using namespace std;
> +
> +namespace WebCore {
> +
> +SoupMessage* ResourceResponse::toSoupMessage() const
> +{
> +    // This GET here is just because SoupMessage wants it, we dn't really know.
> +    SoupMessage* soupMessage = soup_message_new("GET", url().string().utf8().data());
> +    if (!soupMessage)
> +        return 0;
> +
> +    soupMessage->status_code = httpStatusCode();
> +
> +    HTTPHeaderMap headers = httpHeaderFields();
> +    SoupMessageHeaders* soupHeaders = soupMessage->response_headers;
> +    if (!headers.isEmpty()) {
> +        HTTPHeaderMap::const_iterator end = headers.end();
> +        for (HTTPHeaderMap::const_iterator it = headers.begin(); it != end; ++it)
> +            soup_message_headers_append(soupHeaders, it->first.string().utf8().data(), it->second.utf8().data());
> +    }
> +
> +    // Body data is not in the message.
> +    return soupMessage;
> +}

As mentioned on IRC it would we good to share this function with ResourceRequest, since it's identical to what's there but for one line.

> +// We convert this to string because it's easier to use strings as
> +// keys in a GHashTable.
> +static char* identifierToString(unsigned long identifier)
> +{
> +    return g_strdup_printf("%ld", identifier);
> +}
> +
>  void FrameLoaderClient::dispatchWillSendRequest(WebCore::DocumentLoader* loader, unsigned long identifier, ResourceRequest& request, const ResourceResponse& redirectResponse)
>  {
> -    if (redirectResponse.isNull())
> +    WebKitWebView* webView = getViewFromFrame(m_frame);
> +
> +    GOwnPtr<gchar> identifierString(identifierToString(identifier));
> +    WebKitWebResource* webResource = webkit_web_view_get_resource(webView, identifierString.get());
> +
> +    WebKitNetworkRequest* networkRequest = webkit_network_request_new_with_core_request(request);
> +    WebKitNetworkResponse* networkResponse;
> +
> +    // We are adding one more resource to the load, or maybe we are
> +    // just redirecting a load, in which case we need to update our
> +    // WebKitWebResource's knowledge of the URI.
> +    if (redirectResponse.isNull()) {
>          static_cast<WebKit::DocumentLoader*>(loader)->increaseLoadCount(identifier);
> +
> +        networkResponse = WEBKIT_NETWORK_RESPONSE(g_object_new(WEBKIT_TYPE_NETWORK_RESPONSE, 0));

I think we should just pass NULL instead of a dummy object here? Since the point is to inform the user of whether this is a redirect and such, I think that makes it much more clear.

> +    } else {
> +        g_free(webResource->priv->uri);
> +        webResource->priv->uri = g_strdup(request.url().string().utf8().data());
> +
> +        networkResponse = webkit_network_response_new_with_core_response(redirectResponse);
> +    }
> +
> +    g_signal_emit_by_name(webView, "resource-request-starting", m_frame, webResource, networkRequest, networkResponse);
> +
> +    // Feed any changes back into the ResourceRequest object.
> +    request = core(networkRequest);
> +    g_object_unref(networkRequest);

Also you are leaking the networkResponse object AFAICT.

>  }
>  
> -void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long identifier, WebCore::DocumentLoader*, const ResourceRequest&)
> +void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long identifier, WebCore::DocumentLoader*, const ResourceRequest& request)
>  {
> -    notImplemented();
> +    WebKitWebResource* webResource = WEBKIT_WEB_RESOURCE(g_object_new(WEBKIT_TYPE_WEB_RESOURCE, 0));
> +
> +    // The URI is important to have in the resource because that is what we use
> +    // to fetch the WebCore object, later on. We use the private structure
> +    // directly here to avoid changing the property mode from READABLE to
> +    // READWRITE.
> +    webResource->priv->uri = g_strdup(request.url().string().utf8().data());

I don't understand the comment. You are creating the object now, so you certainly pass the URI you want in the parameters.

> +
> +    webkit_web_view_add_resource(getViewFromFrame(m_frame), identifierToString(identifier), webResource);
>  }
>  

> @@ -800,13 +842,22 @@ void FrameLoaderClient::dispatchDidReceiveContentLength(WebCore::DocumentLoader*
>  void FrameLoaderClient::dispatchDidFinishLoading(WebCore::DocumentLoader* loader, unsigned long identifier)
>  {
>      static_cast<WebKit::DocumentLoader*>(loader)->decreaseLoadCount(identifier);
> +
> +    GOwnPtr<gchar> identifierString(identifierToString(identifier));
> +    WebKitWebResource* webResource = webkit_web_view_get_resource(getViewFromFrame(m_frame), identifierString.get());
> +
> +    const char* uri = webkit_web_resource_get_uri(webResource);
> +    RefPtr<ArchiveResource> coreResource(loader->subresource(KURL(KURL(), uri)));
> +    webkit_web_resource_init_with_core_resource(webResource, coreResource.get());

I don't understand why are you init again the resource if you are about the remove it from the view?

> +
> +    webkit_web_view_remove_resource(getViewFromFrame(m_frame), identifierString.get());
>  }
>  
>  void FrameLoaderClient::dispatchDidFailLoading(WebCore::DocumentLoader* loader, unsigned long identifier, const ResourceError& error)
>  {
> -    // FIXME: when does this occur and what should happen?
> -
>      static_cast<WebKit::DocumentLoader*>(loader)->decreaseLoadCount(identifier);
> +
> +    notImplemented();

notImplemented()? The function does something. Do you know now what should it do that it's not doing? If so add a comment.

>  }
>  

> + */
> +
> +#include "config.h"
> +#include "webkitnetworkresponse.h"
> +
> +#include "CString.h"
> +#include "GOwnPtr.h"
> +#include "ResourceResponse.h"
> +#include "webkitprivate.h"
> +


> +static void webkit_network_response_init(WebKitNetworkResponse* response)
> +{
> +    WebKitNetworkResponsePrivate* priv = WEBKIT_NETWORK_RESPONSE_GET_PRIVATE(response);
> +    response->priv = priv;

No need for the variable here.


> diff --git a/WebKit/gtk/webkit/webkitwebresource.cpp b/WebKit/gtk/webkit/webkitwebresource.cpp
> index c162578..fd4abb7 100644
> --- a/WebKit/gtk/webkit/webkitwebresource.cpp
> +++ b/WebKit/gtk/webkit/webkitwebresource.cpp
> @@ -27,6 +27,8 @@
>  #include "KURL.h"
>  #include "PlatformString.h"
>  #include "SharedBuffer.h"
> +#include "webkitenumtypes.h"
> +#include "webkitmarshal.h"
>  #include "wtf/Assertions.h"
>  
>  #include <glib.h>
> @@ -50,13 +52,34 @@ enum {
>      PROP_URI,
>      PROP_MIME_TYPE,
>      PROP_ENCODING,
> -    PROP_FRAME_NAME
> +    PROP_FRAME_NAME,
>  };

Uh? :)

> +void webkit_web_resource_init_with_core_resource(WebKitWebResource* webResource, PassRefPtr<ArchiveResource> resource)
> +{
> +    WebKitWebResourcePrivate* priv = webResource->priv;
> +
> +    if (priv->resource)
> +        priv->resource->deref();
> +
> +    webkit_web_resource_cleanup(webResource);
> +
> +    priv->resource = resource.releaseRef();
> +}

Since this function is only used before removing a resource now I'm really curious about what's its point.

> +
>  /**
>   * webkit_web_resource_new:
>   * @data: the data to initialize the #WebKitWebResource
> @@ -270,11 +298,14 @@ G_CONST_RETURN gchar* webkit_web_resource_get_uri(WebKitWebResource* webResource
>      g_return_val_if_fail(WEBKIT_IS_WEB_RESOURCE(webResource), NULL);
>  
>      WebKitWebResourcePrivate* priv = webResource->priv;
> +
> +    if (priv->uri)
> +        return priv->uri;
> +
>      if (!priv->resource)
>          return NULL;
>  
> -    if (!priv->uri)
> -        priv->uri = g_strdup(priv->resource->url().string().utf8().data());
> +    priv->uri = g_strdup(priv->resource->url().string().utf8().data());
>  
>      return priv->uri;

This seems subtle enough to warrant a comment. Was it broken before? 

>  }
> @@ -344,3 +375,4 @@ G_CONST_RETURN gchar* webkit_web_resource_get_frame_name(WebKitWebResource* webR
>  
>      return priv->frameName;
>  }
> +

> +    /**
> +     * WebKitWebView::resource-request-starting:
> +     * @web_view: the object which received the signal
> +     * @web_frame: the #WebKitWebFrame whose load dispatched this request
> +     * @web_resource: an empty #WebKitWebResource object
> +     * @request: the #WebKitNetworkRequest that will be dispatched
> +     * @response: the #WebKitNetworkResponse representing the redirect
> +     * response, if any
> +     *
> +     * Emitted when a request is about to be sent. You can modify the
> +     * request, by adding/removing/replacing headers, or changing the
> +     * URI, using the #SoupMessage object it carries, if it is
> +     * present. See webkit_network_request_get_message(). Setting the
> +     * request URI to "about:blank" will effectively cause the request
> +     * to load nothing, and can be used to disable the loading of
> +     * specific resources.
> +     *
> +     * Notice that information about an eventual redirect are
> +     * available in @response's #SoupMessage, not in the #SoupMessage
> +     * carried by the @request. If @response doesn't have a
> +     * #SoupMessage (i.e. if webkit_network_response_get_message()
> +     * returns %NULL), then this is not a redirected request.

As mentioned, I think it's better to either have NULL or a full response.

> +     *
> +     * The #WebKitWebResource object will be the same throughout all
> +     * the lifetime of the resource, but the contents may change from
> +     * inbetween signal emissions.
> +     *
> +     * Since: 1.1.14
> +     */
> +    webkit_web_view_signals[RESOURCE_REQUEST_STARTING] = g_signal_new("resource-request-starting",
> +            G_TYPE_FROM_CLASS(webViewClass),
> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),
> +            0,
> +            NULL, NULL,
> +            webkit_marshal_VOID__OBJECT_OBJECT_OBJECT_OBJECT,
> +            G_TYPE_NONE, 4,
> +            WEBKIT_TYPE_WEB_FRAME,
> +            WEBKIT_TYPE_WEB_RESOURCE,
> +            WEBKIT_TYPE_NETWORK_REQUEST,
> +            WEBKIT_TYPE_NETWORK_RESPONSE);
> +

> +

> diff --git a/WebKit/gtk/webkitmarshal.list b/WebKit/gtk/webkitmarshal.list
> index aa0d40c..cff8018 100644
> --- a/WebKit/gtk/webkitmarshal.list
> +++ b/WebKit/gtk/webkitmarshal.list
> @@ -13,7 +13,11 @@ OBJECT:OBJECT
>  OBJECT:STRING,STRING,POINTER
>  OBJECT:VOID
>  VOID:OBJECT,OBJECT
> +VOID:OBJECT,OBJECT,OBJECT,OBJECT
> +VOID:OBJECT,POINTER
>  VOID:OBJECT,POINTER,POINTER
>  VOID:OBJECT,STRING
> +VOID:OBJECT,ULONG
>  VOID:STRING
>  VOID:STRING,STRING
> +VOID:STRING,POINTER

I don't think you need all these now, just the first one.

> -- 
> 1.6.3.3
> 

Marking as r- since I think there's some things to clarify/polish still, but I think the new APIs are good.

[Gustavo Noronha (kov)]

New Note 30

(In reply to comment #17)
> > +#include "CString.h"
> > +#include "GOwnPtr.h"
> > +#include "PlatformString.h"
> 
> I'd say some of these are unused here?

Only GOwnPtr.h =)

> > +#include <libsoup/soup.h>
> 
> And this one is included in ResourceResponse.h already?

Right =)

> As mentioned on IRC it would we good to share this function with
> ResourceRequest, since it's identical to what's there but for one line.

I am not sure where I would add a single function that both call, and I think the only thing that makes sense sharing is the copying of headers, for now, so I decided to not mess with this right now.
 
> I think we should just pass NULL instead of a dummy object here? Since the
> point is to inform the user of whether this is a redirect and such, I think
> that makes it much more clear.

OK, I for some reason have in my mind that passing NULL as the value of an object might blow up the signal emission, but I agree.

> Also you are leaking the networkResponse object AFAICT.

Yeah! Turned all of them into GOwnPtr's.
 
> > +    // The URI is important to have in the resource because that is what we use
> > +    // to fetch the WebCore object, later on. We use the private structure
> > +    // directly here to avoid changing the property mode from READABLE to
> > +    // READWRITE.
> > +    webResource->priv->uri = g_strdup(request.url().string().utf8().data());
> 
> I don't understand the comment. You are creating the object now, so you
> certainly pass the URI you want in the parameters.

Only explaining why I am setting the URI directly instead of initing with the core resource object, really, but this is gone - this part of the code is also only needed for the other signals. I removed all the identifier handling, and the hashtable.

> > +    const char* uri = webkit_web_resource_get_uri(webResource);
> > +    RefPtr<ArchiveResource> coreResource(loader->subresource(KURL(KURL(), uri)));
> > +    webkit_web_resource_init_with_core_resource(webResource, coreResource.get());
> 
> I don't understand why are you init again the resource if you are about the
> remove it from the view?

Yeah, this only makes sense with the load-status thing; this is the time when WebCore already has its ArchivedResource object, so I was depending on load-status to inform the user that the WebResource object just turned more useful than just containing the URI, so this is for the next patch.

> > +    notImplemented();
> 
> notImplemented()? The function does something. Do you know now what should it
> do that it's not doing? If so add a comment.

Yeah, this is load-error.

> >      PROP_ENCODING,
> > -    PROP_FRAME_NAME
> > +    PROP_FRAME_NAME,
> >  };
> 
> Uh? :)

More debris from removing load-status =/

> Since this function is only used before removing a resource now I'm really
> curious about what's its point.

Yeah, next patch material.

> This seems subtle enough to warrant a comment. Was it broken before? 

Not really, but now we want to be able to have an URI even with no ArchivedResource available, because we're creating the object before we have an ArchivedResource, and the URI is the only useful thing we have.

> I don't think you need all these now, just the first one.

okidoki

[Gustavo Noronha (kov)]

Created attachment 39146 [details]
one more draft, with Xan's comments addressed

[Xan Lopez]

Comment on attachment 39146 [details]
one more draft, with Xan's comments addressed

> +    GOwnPtr<WebKitWebResource> webResource(WEBKIT_WEB_RESOURCE(g_object_new(WEBKIT_TYPE_WEB_RESOURCE, 0)));
> +    webResource->priv->uri = g_strdup(request.url().string().utf8().data());

As discussed on IRC, set the property as CONSTRUCT_ONLY and pass as parameter to g_object_new

> +
> +    GOwnPtr<WebKitNetworkRequest> networkRequest(webkit_network_request_new_with_core_request(request));
> +
> +    g_signal_emit_by_name(webView, "resource-request-starting", m_frame, webResource.get(), networkRequest.get(), networkResponse.get());
> +
> +    // Feed any changes back into the ResourceRequest object.
> +    request = core(networkRequest.get());
>  }
>  
> -void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long identifier, WebCore::DocumentLoader*, const ResourceRequest&)
> +void FrameLoaderClient::assignIdentifierToInitialRequest(unsigned long, WebCore::DocumentLoader*, const ResourceRequest&)
>  {
>      notImplemented();
>  }
> @@ -792,7 +812,7 @@ void FrameLoaderClient::setTitle(const String& title, const KURL& url)
>      frameData->title = g_strdup(title.utf8().data());
>  }
>  
> -void FrameLoaderClient::dispatchDidReceiveContentLength(WebCore::DocumentLoader*, unsigned long identifier, int lengthReceived)
> +void FrameLoaderClient::dispatchDidReceiveContentLength(WebCore::DocumentLoader* loader, unsigned long identifier, int lengthReceived)
>  {
>      notImplemented();
>  }

No need to add the name here...

> @@ -800,13 +820,17 @@ void FrameLoaderClient::dispatchDidReceiveContentLength(WebCore::DocumentLoader*
>  void FrameLoaderClient::dispatchDidFinishLoading(WebCore::DocumentLoader* loader, unsigned long identifier)
>  {
>      static_cast<WebKit::DocumentLoader*>(loader)->decreaseLoadCount(identifier);
> +
> +    notImplemented();
>  }
>  

As discussed on IRC, add a comment on this one too.

>  void FrameLoaderClient::dispatchDidFailLoading(WebCore::DocumentLoader* loader, unsigned long identifier, const ResourceError& error)
>  {
> -    // FIXME: when does this occur and what should happen?
> -
>      static_cast<WebKit::DocumentLoader*>(loader)->decreaseLoadCount(identifier);
> +
> +    // This function should notify the application that a resource failed
> +    // loading (maybe a 'load-error' signal in the WebKitWebResource object
> +    notImplemented();
>  }
>  



> @@ -1975,6 +1978,46 @@ static void webkit_web_view_class_init(WebKitWebViewClass* webViewClass)
>              G_TYPE_NONE, 2,
>              G_TYPE_OBJECT, G_TYPE_OBJECT);
>  
> +    /**
> +     * WebKitWebView::resource-request-starting:
> +     * @web_view: the object which received the signal
> +     * @web_frame: the #WebKitWebFrame whose load dispatched this request
> +     * @web_resource: an empty #WebKitWebResource object
> +     * @request: the #WebKitNetworkRequest that will be dispatched
> +     * @response: the #WebKitNetworkResponse representing the redirect
> +     * response, if any
> +     *
> +     * Emitted when a request is about to be sent. You can modify the
> +     * request, by adding/removing/replacing headers, or changing the
> +     * URI, using the #SoupMessage object it carries, if it is
> +     * present. See webkit_network_request_get_message(). Setting the
> +     * request URI to "about:blank" will effectively cause the request
> +     * to load nothing, and can be used to disable the loading of
> +     * specific resources.

I read this like you have to modify the request through the SoupMessage if it's there, but you don't need to get the message to, for example, set the URI. Maybe reword it a bit?

r=me with those changes wooo!

[Gustavo Noronha (kov)]

Created attachment 39151 [details]
latest comments by Xan addressed

[Xan Lopez]

Comment on attachment 39151 [details]
latest comments by Xan addressed

r=me changing the g_warning with a g_return_if_fail as discussed on IRC.

[Gustavo Noronha (kov)]

Landed as r48118.

[Eric Seidel (no email)]

Comment on attachment 39082 [details]
load-status and load-error implementation for resource loading

Clearing r=? since this bug is closed.