Bug 26104 – [GTK] Make NetworkRequest a proper GObject and expose SoupMessage

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

[Gustavo Noronha (kov)]

We landed the groundwork for making NetworkRequest meet our needs. There are still some steps needed going forward:

1. making it a proper GObject (with properties and such), exposing the SoupMessage
2. build a WebKitNetworkResponse class to go with it in delegates where they are both needed
3. implement the willSendRequest "delegate" as a signal, allowing us to better customize and track requests

Number 3 is very related to the DataSource implementation, since we want to give our API users a way to track the loading of each resource. This bug is here to track the first point in that list.

[Gustavo Noronha (kov)]

Created attachment 30813 [details]
Make WebKitNetworkRequest a proper GObject

 ChangeLog                                  |    8 ++
 GNUmakefile.am                             |    6 +
 WebKit/gtk/ChangeLog                       |   24 ++++
 WebKit/gtk/tests/testnetworkrequest.c      |  131 +++++++++++++++++++++
 WebKit/gtk/webkit/webkitnetworkrequest.cpp |  176 +++++++++++++++++++++++-----
 WebKit/gtk/webkit/webkitnetworkrequest.h   |    4 +
 6 files changed, 320 insertions(+), 29 deletions(-)

[Jan Alonzo]

Comment on attachment 30813 [details]
Make WebKitNetworkRequest a proper GObject

> diff --git a/WebKit/gtk/webkit/webkitnetworkrequest.cpp b/WebKit/gtk/webkit/webkitnetworkrequest.cpp
> index feef3ec..f97ca41 100644
> --- a/WebKit/gtk/webkit/webkitnetworkrequest.cpp
> +++ b/WebKit/gtk/webkit/webkitnetworkrequest.cpp
> @@ -22,6 +22,7 @@
>  #include "webkitnetworkrequest.h"
>  
>  #include "CString.h"
> +#include <glib/gi18n-lib.h>
>  #include "ResourceRequest.h"
>  #include "webkitprivate.h"

Usually the order of includes is something like:

** config.h **
** header of this source **

** WebCore/WebKit includes **

** third party **

I would like to follow the same here. If you can please put the glib include in the end that would be great.

>  
> @@ -41,12 +42,22 @@
>  G_DEFINE_TYPE(WebKitNetworkRequest, webkit_network_request, G_TYPE_OBJECT);
>  
>  struct _WebKitNetworkRequestPrivate {
> +    gboolean isConstructed;
>      gchar* uri;
>      SoupMessage* message;
>  };
>  
> +static GObject* webkit_network_request_constructor(GType gtype, guint n_properties, GObjectConstructParam *properties);

The asterisk in properties is misplaced.

> +static void webkit_network_request_get_property(GObject* object, guint prop_id, GValue* value, GParamSpec *pspec)

Ditto with *pspec.

> +static void webkit_network_request_set_property(GObject* object, guint prop_id, const GValue* value, GParamSpec *pspec)

Same.

> +{
> +    WebKitNetworkRequest* request = WEBKIT_NETWORK_REQUEST(object);
> +    WebKitNetworkRequestPrivate* priv = request->priv;
> +
> +    switch(prop_id) {
> +    case PROP_URI:
> +        if (priv->isConstructed) {
> +            webkit_network_request_set_uri(request, g_value_get_string(value));
> +            break;
> +        }
> +        if (g_value_get_string(value))
> +            priv->uri = g_value_dup_string(value);

Should we free uri first before setting a new value? Also, is it possible to refactor and maybe merge these two? Does it matter how we set the uri?

> +    case PROP_MESSAGE:
> +        priv->message = SOUP_MESSAGE(g_value_dup_object(value));

Should we check and free priv->message first as well?

> +     */
> +    g_object_class_install_property(objectClass, PROP_URI,
> +                                    g_param_spec_string("uri",
> +                                                        _("URI"),
> +                                                        _("The URI to which the request will be made."),
> +                                                        NULL,
> +                                                        static_cast<GParamFlags>(WEBKIT_PARAM_READWRITE|G_PARAM_CONSTRUCT)));

We've been using C-style casts in WebKit until now. Should we do the same here?

> +    g_object_class_install_property(objectClass, PROP_MESSAGE,
> +                                    g_param_spec_object("message",
> +                                                        _("Message"),
> +                                                        _("The SoupMessage that backs the request."),
> +                                                        SOUP_TYPE_MESSAGE,
> +                                                        static_cast<GParamFlags>(WEBKIT_PARAM_READWRITE|G_PARAM_CONSTRUCT_ONLY)));

Ditto.  

> +static GObject* webkit_network_request_constructor(GType gtype, guint n_properties, GObjectConstructParam *properties)

Can we make n_properties -> nProperties to be consistent with this file?

> +{
> +    GObject* object = G_OBJECT_CLASS(webkit_network_request_parent_class)->constructor(gtype, n_properties, properties);
> +    WebKitNetworkRequestPrivate* priv = WEBKIT_NETWORK_REQUEST(object)->priv;
> +
> +    priv->isConstructed = TRUE;
> +
> +    if ((priv->uri && priv->message) || (!priv->uri && !priv->message)) {
> +        if (priv->uri)
> +            g_free(priv->uri);

g_free is null-safe so we can skip the if statement.

> +        if (priv->message)
> +            g_object_unref(priv->message);

Is it possible to have an existing priv->message in construction?

> +        g_critical("WebKitNetworkMessage needs to be initialized with one (and only one) of a URI or a SoupMessage.");
> +        g_object_unref(object);
> +        return NULL;

I'm not sure if bailing out would be a good thing. Is it possible to just initialize with message if url and the message url are the same? Is there a better way to do this?

> +    }
> +
> +    if (priv->message)
> +        return object;
> +
> +    priv->message = soup_message_new("GET", priv->uri);
> +    if (!priv->message) {
> +        g_critical("Tried to initialize a WebKitNetworkRequest with an invalid URI: %s", priv->uri);
> +        g_free(priv->uri);

The message is a bit confusing. The URI can be valid it's just that Soup wasn't able to parse it. If that's the case, can we still let it through and let load-error propagate (and so users will get an error page about their request)?

+1 for the API. Patch looks good otherwise. I'm going to say r- for now until we address the points I raised above.

Thanks!

[Gustavo Noronha (kov)]

(In reply to comment #2)
> (From update of attachment 30813 [details] [review])
> > +    case PROP_URI:
> > +        if (priv->isConstructed) {
> > +            webkit_network_request_set_uri(request, g_value_get_string(value));
> > +            break;
> > +        }
> > +        if (g_value_get_string(value))
> > +            priv->uri = g_value_dup_string(value);
> 
> Should we free uri first before setting a new value? Also, is it possible to
> refactor and maybe merge these two? Does it matter how we set the uri?

That path will only be hit on construction, so priv->uri will always be NULL.

> > +    case PROP_MESSAGE:
> > +        priv->message = SOUP_MESSAGE(g_value_dup_object(value));
> 
> Should we check and free priv->message first as well?

Message is a CONSTRUCT_ONLY property, so we can count on it always being NULL, too.

static_cast<GParamFlags>(WEBKIT_PARAM_READWRITE|G_PARAM_CONSTRUCT)));
> 
> We've been using C-style casts in WebKit until now. Should we do the same here?

Sounds good =).

> Can we make n_properties -> nProperties to be consistent with this file?

Yeah, I had that in mind, and forgot to do it. I actually think numerOfProperties is more correct.

> > +        if (priv->message)
> > +            g_object_unref(priv->message);
> 
> Is it possible to have an existing priv->message in construction?

Yes. The construction properties are set before calling _constructor, the code takes advantage of that =).

> > +        g_critical("WebKitNetworkMessage needs to be initialized with one (and only one) of a URI or a SoupMessage.");
> > +        g_object_unref(object);
> > +        return NULL;
> 
> I'm not sure if bailing out would be a good thing. Is it possible to just
> initialize with message if url and the message url are the same? Is there a
> better way to do this?

We could, but that'd be programmer error, and additional complexity I don't think we should try to handle. I believe documenting and bailing out is the simplest and least error prone approach.

> The message is a bit confusing. The URI can be valid it's just that Soup wasn't
> able to parse it. If that's the case, can we still let it through and let
> load-error propagate (and so users will get an error page about their request)?

That'd be a bug in Soup, I think. Notice that not all requests depend on NetworkRequest objects being created correctly to get loaded - only those passed to _load_request by the user will end up being loaded. The other cases do not depend on the NetworkRequest being created correctly to load (the SoupMessage will be created by ResourceRequest in those cases). I think we should assume that when Soup doesn't like the URI, that the URI is not valid, and when that is not the case, we fix Soup's parsing. What do you think?

> +1 for the API. Patch looks good otherwise. I'm going to say r- for now until
> we address the points I raised above.

Thanks! I'm uploading another one right away!

[Gustavo Noronha (kov)]

Created attachment 30888 [details]
Make WebKitNetworkRequest a proper GObject

 ChangeLog                                  |    8 ++
 GNUmakefile.am                             |    6 +
 WebKit/gtk/ChangeLog                       |   24 ++++
 WebKit/gtk/tests/testnetworkrequest.c      |  131 ++++++++++++++++++++
 WebKit/gtk/webkit/webkitnetworkrequest.cpp |  180 +++++++++++++++++++++++-----
 WebKit/gtk/webkit/webkitnetworkrequest.h   |    4 +
 6 files changed, 324 insertions(+), 29 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 30888 [details]
Make WebKitNetworkRequest a proper GObject

Addresses all Jan comments I didn't reply to with a justification as comment.

[Gustavo Noronha (kov)]

Created attachment 30943 [details]
Refactor handling of the SoupMessage

 WebCore/ChangeLog                                  |   21 +++++
 .../platform/network/soup/ResourceHandleSoup.cpp   |    9 +-
 WebCore/platform/network/soup/ResourceRequest.h    |   31 ++-----
 .../platform/network/soup/ResourceRequestSoup.cpp  |   96 ++++----------------
 WebKit/gtk/ChangeLog                               |   18 ++++
 WebKit/gtk/tests/testhttpbackend.c                 |   46 +---------
 WebKit/gtk/webkit/webkitnetworkrequest.cpp         |   17 +++-
 7 files changed, 88 insertions(+), 150 deletions(-)

[Gustavo Noronha (kov)]

Created attachment 30944 [details]
Make WebKitNetworkRequest a proper GObject

 ChangeLog                                  |    8 ++
 GNUmakefile.am                             |    6 +
 WebKit/gtk/ChangeLog                       |   24 ++++
 WebKit/gtk/tests/testnetworkrequest.c      |  131 ++++++++++++++++++++
 WebKit/gtk/webkit/webkitnetworkrequest.cpp |  179 +++++++++++++++++++++++-----
 WebKit/gtk/webkit/webkitnetworkrequest.h   |    4 +
 6 files changed, 322 insertions(+), 30 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 30943 [details]
Refactor handling of the SoupMessage

So, a small rationale: after landing the original patch, I went ahead and started experimenting with implementing more functionality, using the willSendRequest 'delegate'.

I have a patch I'll post here soonish that implements that as a new signal. That patch exposed a bug in my original patch, though: I implemented a copy constructor to handle the fact that SoupMessage had to be treated in a special way, but I did not override operator=, which causes the message to be shallow-copied in some cases, and thus double-freed.

I did implement the operator= override, and it works, but I decided to avoid all this complexity by taking the same approach used by Qt: we create a new SoupMessage whenever we need one, and store nothing in the ResourceRequest object.

This lowers complexity, and will make us happy campers, not having to deal with lots of custom code needing updating in ResourceRequest, and less crashes, too.

[Gustavo Noronha (kov)]

Comment on attachment 30944 [details]
Make WebKitNetworkRequest a proper GObject

This is my GObjectization of NetworkRequest updated to the refactoring I proposed. There are still some things we need to think for this patch, such as, what to do about requests which are not handled by soup (file://, about:, data:), for instance.

I don't think these decisions are blockers for this change to happen, as I don't think we'll have API breakage caused by them.

[Gustavo Noronha (kov)]

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

 WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp |   15 ++++++++++++-
 WebKit/gtk/webkit/webkitnetworkrequest.cpp         |    4 +-
 WebKit/gtk/webkit/webkitwebview.cpp                |   21 ++++++++++++++++++++
 3 files changed, 36 insertions(+), 4 deletions(-)

[Gustavo Noronha (kov)]

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

This is not what I think the final patch should look like. I believe we may want to implement a placeholder WebKitNetworkResponse before this, but I'm submitting it so that people may comment.

[Xan Lopez]

Comment on attachment 30943 [details]
Refactor handling of the SoupMessage

Looks good to me. The only comment is that I'd name ResourceRequest::setSoupMessage something like ResourceRequest::setFromSoupMessage, which better explains what the function actually does. r+ me with a change like that.

[Xan Lopez]

Comment on attachment 30944 [details]
Make WebKitNetworkRequest a proper GObject

Two general comments:

- I think we should not fail to construct the object with invalid URIs, but set it anyway (with no message), and later on WebKit build our ResourceRequests from the URI when the message is not present in the WebKitNetworkRequest object.

- Also, just giving a warning when both URI nad message are passed (checked in a 'constructed' handler) is enough, no need to fail. The SoupMessage will have priority over the URI when both are present.

[Gustavo Noronha (kov)]

Comment on attachment 30943 [details]
Refactor handling of the SoupMessage

Landed as r44536.

[Gustavo Noronha (kov)]

Created attachment 31116 [details]
Make WebKitNetworkRequest a proper GObject

 ChangeLog                                  |    8 ++
 GNUmakefile.am                             |    6 +
 WebKit/gtk/ChangeLog                       |   24 +++++
 WebKit/gtk/tests/testnetworkrequest.c      |   97 +++++++++++++++++++
 WebKit/gtk/webkit/webkitnetworkrequest.cpp |  140 ++++++++++++++++++++++------
 WebKit/gtk/webkit/webkitnetworkrequest.h   |    4 +
 6 files changed, 251 insertions(+), 28 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 31116 [details]
Make WebKitNetworkRequest a proper GObject

Ok, here's another try. This time I'm going for the simple is beautiful approach.

[Gustavo Noronha (kov)]

Would be good to get some feedback from danw here, I think =).

[Dan Winship]

I don't have a whole lot to say; I'm really not familiar with how WebKit works above the level of WebCore/platform/network, so...

(In reply to comment #3)
> > The message is a bit confusing. The URI can be valid it's just that Soup wasn't
> > able to parse it. If that's the case, can we still let it through and let
> > load-error propagate (and so users will get an error page about their request)?
> 
> That'd be a bug in Soup, I think.

Yes, soup_uri_new() should accept any syntactically-correct URI.

(In reply to comment #9)
> There are still some things we need to think for this patch, such as,
> what to do about requests which are not handled by soup (file://, about:,
> data:), for instance.

I am hoping to have SoupURILoader (http://bugzilla.gnome.org/show_bug.cgi?id=557777) ready for 2.28. (There's a summer of code student working on it.) It should support ftp, file, and data itself, and allow you to plug in handlers for other URI schemes like "about" or "chrome" if you want. The API isn't really figured out yet, but feel free to make suggestions.

It *won't* be based on SoupMessage though, because that's too HTTP-specific.

[Gustavo Noronha (kov)]

(In reply to comment #18)
> I don't have a whole lot to say; I'm really not familiar with how WebKit works
> above the level of WebCore/platform/network, so...

I'm interested in whatever little you are able to fix in my understanding =).

> > That'd be a bug in Soup, I think.
> 
> Yes, soup_uri_new() should accept any syntactically-correct URI.

Yeah, I think my last patch acknowledges my approach was mistaken here.
 
> I am hoping to have SoupURILoader
> (http://bugzilla.gnome.org/show_bug.cgi?id=557777) ready for 2.28. (There's a
> summer of code student working on it.) It should support ftp, file, and data
> itself, and allow you to plug in handlers for other URI schemes like "about" or
> "chrome" if you want. The API isn't really figured out yet, but feel free to
> make suggestions.
> 
> It *won't* be based on SoupMessage though, because that's too HTTP-specific.

We will then expose it too, I guess, along-side message, and do some more magic inside WebKitNetworkRequest and ResourceHandleSoup. I think my current patch is future-proof in this regard. Thanks for the comments!

[Xan Lopez]

Comment on attachment 31116 [details]
Make WebKitNetworkRequest a proper GObject

I think you should add a test that passes both URI and message, probably. Also not sure if it's OK to be totally silent when passing both, but well, I guess this patch can go as-is.

[Gustavo Noronha (kov)]

(In reply to comment #20)
> (From update of attachment 31116 [details] [review])
> I think you should add a test that passes both URI and message, probably. Also
> not sure if it's OK to be totally silent when passing both, but well, I guess
> this patch can go as-is.
> 

Landed as r44598, with the additional test.

I also made the following change; since we had discussed this earlier, I thought it was not a problem to sneak it in here (there was a nasty crash otherwise):

diff --git a/WebKit/gtk/webkit/webkitwebframe.cpp b/WebKit/gtk/webkit/webkitwebframe.cpp
index b1eac26..60fb1bb 100644
--- a/WebKit/gtk/webkit/webkitwebframe.cpp
+++ b/WebKit/gtk/webkit/webkitwebframe.cpp
@@ -510,7 +510,14 @@ void webkit_web_frame_load_request(WebKitWebFrame* frame, WebKitNetworkRequest*
     if (!coreFrame)
         return;
 
-    coreFrame->loader()->load(ResourceRequest(webkit_network_request_get_message(request)), false);
+    SoupMessage* soupMessage = webkit_network_request_get_message(request);
+    if (soupMessage) {
+        coreFrame->loader()->load(ResourceRequest(soupMessage), false);
+        return;
+    }
+
+    KURL url = KURL(KURL(), String::fromUTF8(webkit_network_request_get_uri(request)));
+    coreFrame->loader()->load(ResourceRequest(url), false);
 }

I'm closing this bug, since the original intent is completed, and I'll open another one to post/track the implementation of willSendRequest as soon as possible.