[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 #14 from Nayan Kumar K <nayankk at motorola.com>  2011-09-22 00:15:03 PST ---
(In reply to comment #12)
> (From update of attachment 108137 [details])
> 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
I am using WebKitPrivate.h added in https://bugs.webkit.org/show_bug.cgi?id=67990. This bug is anyway dependent on 67990.
> 
> > 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.
Done
> 
> > 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.
Done
> 
> > 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.
Martin, do we need to translate nick and blurb? I guess it's good to have.
> 
> > 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.
Yes, default values of WKPreference are the defaults of WebKitWebSettings.
> 
> > 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()
> 
Done
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:657
> > +}
> 
> I prefer if all the public API is moved here (and documented)
> 
Done
> > 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.
Done.
> 
> > 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();
> 
> ...
> 
Done
> > 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));
> 
Done
> > Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:198
> > +    g_test_add_func("/webkit/websettings/hyperlinkauditing", testWebKitWebSettingsHyperLinkAuditing);
> 
> use /webkit2/websettings/foo
Done

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