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

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Christian Dywan
<christian at twotoasts.de>'s request for review:
Bug 32789: [GTK] Implement a WebKitPageGroup object
https://bugs.webkit.org/show_bug.cgi?id=32789

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

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

I think I'd prefer this class to be called WebKitPageGroup to match the WebCore
name and the name of the concept in other APIs. Failing that, perhaps
WebKitWebViewGroup would work.

> Source/WebKit/gtk/ChangeLog:2
> +2011-02-17  Christian Dywan	<christian at lanedo.com>
> +

The ChangeLog doesn't seem like it was generated with prepare-ChangeLog.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:34
> +#include "webkitenumtypes.h"
> +#include "webkitglobalsprivate.h"
> +#include "webkitwebviewprivate.h"
> +#include "webkitversion.h"
> +
> +#include "CString.h"
> +#include "PageGroup.h"
> +#include "PlatformString.h"
> +#include "GOwnPtrGtk.h"
> +
> +#include <glib/gi18n-lib.h>
> +

Please lump these all together in alphabetical order.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:41
> + * A #WebKitWebGroup can be applied to a #WebKitWebView to manage
> + * behavior of related instances. This includes visited links, DOM
> + * storage and injected user styles as well as scripts.

Maybe make the documentation a little less passive

Suggestion (perhaps not great):
To manage the behavior of a group of related #WebKitWebViews add them to a
#WebKitWebGroup.
When #WebKitWebViews are in the same page group, they shared visitied links,
DOM storage settings
and injected user styles and scripts.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:77
> +static void webkit_web_group_finalize(GObject*);
> +
> +static void webkit_web_group_set_property(GObject*, guint, const GValue*,
GParamSpec*);
> +
> +static void webkit_web_group_get_property(GObject*, guint, GValue*,
GParamSpec*);
> +

Do you mind remove the extra newlines here, just to show that these forward
declarations are associated with webkit_web_group_class_init?

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:91
> +	* The name of the group, an arbitrary unique string up to the
> +	* user of the API.

Here since there property is only readable, you should move the other facts
about it to the constructor argument. You can probaby just say:

The name of the page group.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:93
> +	* Since: 1.3.4

Should be 1.5.0 now, I think.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:101
> +				       NULL,

I think you should use 0 instead of NULL here.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:102
> +				       (GParamFlags)(WEBKIT_PARAM_READABLE |
G_PARAM_CONSTRUCT_ONLY)));

Please use static_cast here.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:115
> +    WebKitWebGroup* webGroup = WEBKIT_WEB_GROUP(object);
> +    WebKitWebGroupPrivate* priv = webGroup->priv;

Just make this	WebKitWebGroupPrivate* priv = WEBKIT_WEB_GROUP(object)->priv;

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:117
> +    g_free(priv->name);

Here you could use a placement new and just make priv->name be a CString.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:125
> +    WebKitWebGroup* webGroup = WEBKIT_WEB_GROUP(object);
> +    WebKitWebGroupPrivate* priv = webGroup->priv;

You don't need to have a webGroup variable here.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:140
> +    WebKitWebGroup* webGroup = WEBKIT_WEB_GROUP(object);
> +    WebKitWebGroupPrivate* priv = webGroup->priv;

Ditto.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:154
> + * @name: the name of the group

Here you should say more about what to pass in.

Something like:
@name: the name of the page group. This can be any arbitrary string, but must
be unique among all instantiated #WebKitWebGroups.

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

You can just do  g_return_val_if_fail(name, 0);

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:174
> + * Determines the name of the group.

You should mention here that the name is unique, I think.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:176
> + * Returns: a name

a name --> "the name of the #WebKitWebGroup.

> Source/WebKit/gtk/webkit/webkitwebgroup.cpp:202
> +    PageGroup* pageGroup =
PageGroup::pageGroup(String::fromUTF8(webGroup->priv->name));

Would it make sense to cache the pageGroup privately? Then you wouldn't have to
constantly get it from WebCore.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1401
> +    if (priv->webGroup)
> +	   priv->webGroup.clear();

You do not need to call clear here, because the destructor of the private
section is called in the finalize method.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3542
> +    priv->webGroup = 0;

GRefPtrs are initialized to zeros in their contstructor, so you can just remove
this.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5069
> + * Return value: a #WebKitWebGroup, or %NULL if unset

if unset -> if this #WebKitWebView does not belong to any group.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5075
> +    g_return_val_if_fail(WEBKIT_IS_WEB_VIEW(webView), NULL);

NULL -> 0.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5078
> +    WebKitWebViewPrivate* priv = webView->priv;
> +    return priv->webGroup.get();

No need to cache priv here, just do:

return webView->priv->webGroup.get();