[Webkit-unassigned] [Bug 68371] [GTK][WEBKIT2] Add WebKitWebSettings GTK+ API

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-09-21 06:04:38 PST ---
(From update of attachment 108137)
View in context: https://bugs.webkit.org/attachment.cgi?id=108137&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:30
> +#include "WebKitGlobalsPrivate.h"

It seems you forgot to add this file to the patch and to GNUMakefile.am. I added a similar file already in patrch for bug https://bugs.webkit.org/show_bug.cgi?id=67931

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:83
> +static void webkitWebSettingsFinalize(GObject* object);
> +
> +static void webkitWebSettingsSetProperty(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec);
> +
> +static void webkitWebSettingsGetProperty(GObject* object, guint prop_id, GValue* value, GParamSpec* pspec);

Move the implementation of these methods here and you don't need prototypes.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:92
> +    GParamFlags flags = (GParamFlags)(WEBKIT_PARAM_READWRITE | G_PARAM_CONSTRUCT);

I think you should use a C++ style cast here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:106
> +                                                         _("Enable Java Script"),
> +                                                         _("Enable Java Script languages."),

I still wonder why we want to translate nick and blurb.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:291
> +    settings->priv->preferences = WKPreferencesCreate();

Are WKPreferences default values the same we are defining here for our properties? If they don't match we would need to initialize WKPreferences here with our default values.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:297
> +    delete WEBKIT_WEB_SETTINGS(object)->priv;
> +    G_OBJECT_CLASS(webkit_web_settings_parent_class)->finalize(object);

Use the placement new syntax in webkit_web_settings_init() and you don't need to delete the object here. It should be something like:

WebKitWebSettingsPrivate* priv = G_TYPE_INSTANCE_GET_PRIVATE(settings, WEBKIT_TYPE_WEB_SETTINGS, WebKitWebSettingsPrivate);
settings->priv = priv;
new (priv) WebKitWebSettingsPrivate();

And call g_type_class_add_private(klass, sizeof(WebKitWebSettingsPrivate)); in class_init()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:657
> +}

I prefer if all the public API is moved here (and documented)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.h:65
> +WK_EXPORT void
> +webkit_web_settings_set_enable_java_scripts(WebKitWebSettings* settings, gboolean enabled);

Don't use webkit coding style in public headers.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.h:100
> +/* Property get function */

I prefer to group functions per property instead of getters and setters (and the same in the .cpp file):

webkit_web_settings_set_foo();
webkit_web_settings_get_foo();

webkit_web_settings_set_bar();
webkit_web_settings_get_bar();

...

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:32
> +    gboolean scripts = FALSE;
> +    g_object_get(settings, "enable-java-script", &scripts, NULL);
> +    g_assert(scripts);

You could use the public getters for the tests, I think it's simpler and more clear

g_assert(webkit_web_settings_get_enable_java_script(settings));

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:198
> +    g_test_add_func("/webkit/websettings/hyperlinkauditing", testWebKitWebSettingsHyperLinkAuditing);

use /webkit2/websettings/foo

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