[webkit-reviews] review denied: [Bug 26104] [GTK] Make NetworkRequest a proper GObject and expose SoupMessage : [Attachment 30813] Make WebKitNetworkRequest a proper GObject

[bugzilla-daemon at webkit.org]

Jan Alonzo <jmalonzo at gmail.com> has denied Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 26104: [GTK] Make NetworkRequest a proper GObject and expose SoupMessage
https://bugs.webkit.org/show_bug.cgi?id=26104

Attachment 30813: Make WebKitNetworkRequest a proper GObject
https://bugs.webkit.org/attachment.cgi?id=30813&action=review

------- Additional Comments from Jan Alonzo <jmalonzo at gmail.com>
> 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!