[Webkit-unassigned] [Bug 32789] [GTK] Implement a WebKitPageGroup object

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32789


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45523|                            |review-
               Flag|                            |




--- Comment #5 from Martin Robinson <mrobinson at webkit.org>  2010-12-15 15:19:36 PST ---
(From update of attachment 45523)
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;

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