[webkit-reviews] review denied: [Bug 32789] [GTK] Implement a WebKitPageGroup object : [Attachment 45523] Initial WebKitWebGroup with no additional features

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied  review:
Bug 32789: [GTK] Implement a WebKitPageGroup object
https://bugs.webkit.org/show_bug.cgi?id=32789

Attachment 45523: Initial WebKitWebGroup with no additional features
https://bugs.webkit.org/attachment.cgi?id=45523&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=45523&action=review

Nice work. This patch needs to be updated to work with ToT. The use of the page
group API in the DRT is probably enough exercise to constitute test coverage,
but having separate unit tests would be nice as well.

> WebKit/gtk/webkit/webkitprivate.h:156
> +	   WebKitWebGroup* webGroup;

Using a PlatformRefPtr here would avoid the need to do manual reference
counting.

> WebKit/gtk/webkit/webkitprivate.h:-322
> -    webkit_web_view_set_group_name(WebKitWebView* web_view, const gchar*
group_name);

Should be webView and groupName, since this isn't public API. I'd rather see
this method in DumpRenderTreeSupport.

> WebKit/gtk/webkit/webkitwebgroup.cpp:32
> +#include "webkitenumtypes.h"
> +#include "webkitprivate.h"
> +#include "webkitversion.h"
> +
> +#include "CString.h"
> +#include "PageGroup.h"
> +#include "PlatformString.h"
> +#include <wtf/gtk/GOwnPtr.h>
> +
> +#include <glib/gi18n-lib.h>

These should be in one block and ordered as if they were filtered through
'sort'.

> WebKit/gtk/webkit/webkitwebgroup.cpp:63
> +    gchar* name;

If you managed the private section of this GObject with new and delete you
could change this to a CString and avoid manual memory management.

> WebKit/gtk/webkit/webkitwebgroup.cpp:78
> +static void webkit_web_group_finalize(GObject* object);
> +
> +static void webkit_web_group_set_property(GObject* object, guint prop_id,
const GValue* value, GParamSpec* pspec);
> +
> +static void webkit_web_group_get_property(GObject* object, guint prop_id,
GValue* value, GParamSpec* pspec);

Please remove the argument names in these declarations.

> WebKit/gtk/webkit/webkitwebgroup.cpp:82
> +    GObjectClass* gobject_class = G_OBJECT_CLASS(klass);

This should be gobjectClass.

> WebKit/gtk/webkit/webkitwebgroup.cpp:92
> +	* The name of the group.

It makes sense to expand this. Does every group have a unique name?

> WebKit/gtk/webkit/webkitwebgroup.cpp:123
> +static void webkit_web_group_set_property(GObject* object, guint prop_id,
const GValue* value, GParamSpec* pspec)

Variable names should use WebKit style. prop_id should be propId and pspec
should be paramSpec.

> WebKit/gtk/webkit/webkitwebgroup.cpp:138
> +static void webkit_web_group_get_property(GObject* object, guint prop_id,
GValue* value, GParamSpec* pspec)

Same as above.

> WebKit/gtk/webkit/webkitwebgroup.cpp:166
> +    g_return_val_if_fail(name != NULL, NULL);

This should be: g_return_val_if_fail(name, 0);

> WebKit/gtk/webkit/webkitwebgroup.cpp:183
> +    g_return_val_if_fail(WEBKIT_IS_WEB_GROUP(webGroup), NULL);

This should be g_return_val_if_fail(WEBKIT_IS_WEB_GROUP(webGroup), 0);

> WebKit/gtk/webkit/webkitwebgroup.cpp:203
> +    WebKitWebGroupPrivate* priv = webGroup->priv;

No need to cache this if you're only going to use it once.

> WebKit/gtk/webkit/webkitwebview.cpp:1046
> +	   g_object_unref(priv->webGroup);
> +	   priv->webGroup = NULL;

Should be priv->webGroup = 0;