Bug 32789 – [GTK] Implement a WebKitPageGroup object

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Christian Dywan]

The page groups in WebCore are used for the following:

  Tracking visited links. You can add links manually if needed.

  Adding user styles and scripts to a group.

  DOM local storage.

So I propose we add a WebKitWebGroup object that has functions for fain-grained control over the above features. Note that I suggest WebGroup because we have Views, not Pages in our terminology.

[Christian Dywan]

Created attachment 45523 [details]
Initial WebKitWebGroup with no additional features

This adds a WebKitWebGroup object, replaces the private webkit_web_view_set_group_name with webkit_web_view_get_group and updates Dump Render Tree to use it.

[Alexander Butenko]

ping?

[Nicolas Dufresne]

Oh, also PageGroup are required to get frameLookup to work with named popup (e.g. <a href="#" target="myTarget">test</a> will always create new window without placing all pages in the same group)

[Alexander Butenko]

guys, anybody can take a look on this? :)

[Martin Robinson]

Comment on attachment 45523 [details]
Initial WebKitWebGroup with no additional features

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;

[Christian Dywan]

Created attachment 82790 [details]
Initial WebKitWebGroup with no additional features #2

Updated patch to TOT, updated variable names (back then the correct style was different).

[WebKit Review Bot]

Attachment 82790 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Last 3072 characters of output:
l/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/gtk/webkit/webkitwebgroup.cpp:197:  Extra space between WebKitWebView* and webView  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitwebgroup.h:49:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebgroup.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebgroup.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebgroup.h:52:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebgroup.h:56:  WEBKIT_API should only appear in the chromium public directory.  [readability/webkit_api] [5]
Source/WebKit/gtk/webkit/webkitwebgroup.h:56:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebgroup.h:59:  WEBKIT_API should only appear in the chromium public directory.  [readability/webkit_api] [5]
Source/WebKit/gtk/webkit/webkitwebgroup.h:59:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebgroup.h:62:  WEBKIT_API should only appear in the chromium public directory.  [readability/webkit_api] [5]
Source/WebKit/gtk/webkit/webkitwebgroup.h:62:  The parameter name "web_group" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebgroup.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebgroup.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit/gtk/webkit/webkitwebgroup.h:65:  WEBKIT_API should only appear in the chromium public directory.  [readability/webkit_api] [5]
Source/WebKit/gtk/webkit/webkitwebgroup.h:65:  The parameter name "web_group" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebgroup.h:66:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/gtk/webkit/webkitwebgroup.h:66:  Extra space between WebKitWebView* and web_view  [whitespace/declaration] [3]
Source/WebKit/gtk/ChangeLog:3:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 32 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Christian Dywan]

Created attachment 82792 [details]
webkit_web_group_add_user_script + clear

As a basis for discussion a provisional patch for add_user_script() and clear() methods.

I'm still not sure what exactly the use case of world ids is. They are even objects in WebCore now.

The use case I am coming from is this: the application adds scripts, to a set of web views. Extensions may separately provide scripts, presumably under a different group name. One extension adds scripts provided by the user, also under one group name.

I am also unsure about specifying when and in what frames to apply a script. The only sensible case from my perspective appears to be all frames, after existing styles.

[Christian Dywan]

Features I would like to see on the page group:

set_view_mode on the page group. It's not a setting and updating the value for each tab in a browser is awkward.

PageGroup::setShouldTrackVisitedLinks → track-visible-links

[Martin Robinson]

Comment on attachment 82790 [details]
Initial WebKitWebGroup with no additional features #2

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
> [phone]  Christian Dywan  <[e-mail]>
> +

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();

[Martin Robinson]

Probably not going to make it since we have shifted focus to WebKit2 now.

[Martin Robinson]

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