Bug 68371 – [GTK][WEBKIT2] Add WebKitSettings GTK+ API.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Nayan Kumar K]

Add WebKitWebSettings GTK+ API.

[Nayan Kumar K]

Created attachment 107880 [details]
WebKitWebSettings

First set of API's to set or get WebKit preferences via WebKitWebSettings object.

[Martin Robinson]

Comment on attachment 107880 [details]
WebKitWebSettings

This is missing documentation for all properties.

[Nayan Kumar K]

Created attachment 107971 [details]
WebKitWebSetting GTK+ API

What should be the 'Since' field for these API's? I have assumed that these API's will be the part of webkitgtk-2.0!

[Carlos Garcia Campos]

(In reply to comment #3)
> Created an attachment (id=107971) [details]
> WebKitWebSetting GTK+ API
> 
> What should be the 'Since' field for these API's? I have assumed that these API's will be the part of webkitgtk-2.0!

It's a new library, I think we don't need to use Since: tags.

[Carlos Garcia Campos]

Comment on attachment 107971 [details]
WebKitWebSetting GTK+ API

A few comments in general:

 - Use WebKit coding style for all private methods that are not autogenerated (_class_init and _init)
 - Use WebKit coding style for unit tests too
 - It would be nice to have getters and setters for all properties
 - Do we really need to translate nick and blurb of properties? martin?

[Nayan Kumar K]

Created attachment 107990 [details]
WebKitWebSettings GTK+ API

Resolved style issues.

[Nayan Kumar K]

(In reply to comment #5)
> (From update of attachment 107971 [details])
> A few comments in general:
> 
>  - Use WebKit coding style for all private methods that are not autogenerated (_class_init and _init)
Done
>  - Use WebKit coding style for unit tests too
Done
>  - It would be nice to have getters and setters for all properties
I will submit another patch with remaining properties (to match with the preferences exposed in WKPreferences.h), since the size of this patch is already ~35K.
>  - Do we really need to translate nick and blurb of properties? martin?

Also, removed 'Since' field from documentation.

[Carlos Garcia Campos]

(In reply to comment #7)
> >  - It would be nice to have getters and setters for all properties
> I will submit another patch with remaining properties (to match with the preferences exposed in WKPreferences.h), since the size of this patch is already ~35K.

I meant having webkit_web_settings_get_foo() and webkit_web_settings_set_foo() for the properties already added. Using g_object_get/set is convenient when getting/setting more than one property at the same time, but they are more inefficient and error prone (you can mispell the property name and it would still build).

[Nayan Kumar K]

(In reply to comment #8)
> (In reply to comment #7)
> > >  - It would be nice to have getters and setters for all properties
> > I will submit another patch with remaining properties (to match with the preferences exposed in WKPreferences.h), since the size of this patch is already ~35K.
> 
> I meant having webkit_web_settings_get_foo() and webkit_web_settings_set_foo() for the properties already added. Using g_object_get/set is convenient when getting/setting more than one property at the same time, but they are more inefficient and error prone (you can mispell the property name and it would still build).

It would be something like this then,

void webkit_web_setting_set_scripts_enabled(gobject, value)
{
    WebKitWebSettings* webSettings = WEBKIT_WEB_SETTINGS(object);
    WebKitWebSettingsPrivate* priv = webSettings->priv;
    WKPreferencesSetJavaScriptEnabled(priv->preferences, (bool)g_value_get_boolean(value));
}

static void webkitWebSettingsSetProperty(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec)
{   ....
    switch(prop_id) {
    case PROP_ENABLE_SCRIPTS:
         webkit_web_setting_set_scripts_enabled(gobject, value);
         break;
    ....
}

This would expose many API's for get/set properties. Should that be fine?

[Carlos Garcia Campos]

(In reply to comment #9)
> It would be something like this then,
> 
> void webkit_web_setting_set_scripts_enabled(gobject, value)
> {
>     WebKitWebSettings* webSettings = WEBKIT_WEB_SETTINGS(object);
>     WebKitWebSettingsPrivate* priv = webSettings->priv;
>     WKPreferencesSetJavaScriptEnabled(priv->preferences, (bool)g_value_get_boolean(value));
> }
> 
> static void webkitWebSettingsSetProperty(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec)
> {   ....
>     switch(prop_id) {
>     case PROP_ENABLE_SCRIPTS:
>          webkit_web_setting_set_scripts_enabled(gobject, value);
>          break;
>     ....
> }

No, it should be something like this:

void webkit_web_setting_set_enable_scripts(WebKitWebSettings* settings, gboolean enabled)
{
    g_return_if_fail(WEBKIT_IS_WEB_SETTINGS(settings));
    WebKitWebSettingsPrivate* priv = settings->priv;
    gboolean currentValue = WKPreferencesGetJavaScriptEnabled(priv->preferences);
    if (enabled == currentValue)
        return;

    WKPreferencesSetJavaScriptEnabled(priv->preferences, enabled);
    g_object_notify(G_OBJECT(settings), "enable-scripts");
}

switch(prop_id) {
case PROP_ENABLE_SCRIPTS:
    webkit_web_setting_set_enable_scripts(settings, g_value_get_boolean(value));
    break;

> This would expose many API's for get/set properties. Should that be fine?

It's fine for me, Martin?

[Nayan Kumar K]

Created attachment 108137 [details]
WebKitWebSetting GTK+ API

Exposed setter/getter API's for all properties.

[Carlos Garcia Campos]

Comment on attachment 108137 [details]
WebKitWebSetting GTK+ API

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

[Nayan Kumar K]

Created attachment 108286 [details]
WebKitWebSettings GTK+ API

[Nayan Kumar K]

(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

[Nayan Kumar K]

Created attachment 108288 [details]
WebKitWebSettings GTK+ API

By mistake, earlier patch included the changes in testwebcontext to resolve the compilation errors (which is anyway solved in 67931 now). 

Reverted those changes now.

[Carlos Garcia Campos]

Comment on attachment 108286 [details]
WebKitWebSettings GTK+ API

View in context: https://bugs.webkit.org/attachment.cgi?id=108286&action=review

This looks much better, I still have a few comments, though

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:81
> +static void webkitWebSettingsFinalize(GObject* object)
> +{
> +    G_OBJECT_CLASS(webkit_web_settings_parent_class)->finalize(object);
> +}

We don't need this, at least for now that all properties are boolean.

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

It should be propId instead of prop_id

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:89
> +        webkit_web_settings_set_java_script_enabled(settings, (bool)g_value_get_boolean(value));

webkit_web_settings_set_java_script_enabled() takes a gboolean, bool cast is wrong there.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:92
> +        webkit_web_settings_set_loads_images_automatically(settings, (bool)g_value_get_boolean(value));

I think methods should follow the pattern webkit_web_settings_[get|set]_property_name(), in this case webkit_web_settings_set_auto_load_images()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:229
> +                                                         "load-icon-ignoring-image-load-pref",

The name is long, but I think we should still use preference instead of pref

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:248
> +                                                         FALSE,

This is enabled by default in WebKit1, I think we should use the same default values to make the transition easier and consistent.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:360
> +                                                         TRUE,

This is FALSE by default in WebKit1

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:374
> +                                                         TRUE,

This is FALSE by default in WebKit1

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:406
> + * Returns: TRUE - If Java Script is enabled.

Use %TRUE and remove the -

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:412
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), false);

The function returns a gboolean, use FALSE instead of false here.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebcontext.c:27
> -    g_assert(webkit_web_context_get_default(context) == webkit_web_context_get_default(context));
> +    g_assert(webkit_web_context_get_default() == webkit_web_context_get_default());

This change is unrelated. I already fixed it in bug https://bugs.webkit.org/show_bug.cgi?id=67931.

[Nayan Kumar K]

Created attachment 108306 [details]
WebKitWebSettings GTK+ API

[Nayan Kumar K]

Created attachment 108311 [details]
WebKitWebSetting GTK+ API

Fixed few style issues

[Nayan Kumar K]

Comment on attachment 108286 [details]
WebKitWebSettings GTK+ API

View in context: https://bugs.webkit.org/attachment.cgi?id=108286&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:83
>> +static void webkitWebSettingsSetProperty(GObject* object, guint prop_id, const GValue* value, GParamSpec* pspec)
> 
> It should be propId instead of prop_id

Done

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:89
>> +        webkit_web_settings_set_java_script_enabled(settings, (bool)g_value_get_boolean(value));
> 
> webkit_web_settings_set_java_script_enabled() takes a gboolean, bool cast is wrong there.

Done.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:92
>> +        webkit_web_settings_set_loads_images_automatically(settings, (bool)g_value_get_boolean(value));
> 
> I think methods should follow the pattern webkit_web_settings_[get|set]_property_name(), in this case webkit_web_settings_set_auto_load_images()

Done. I followed the naming convention exposed by WKPreference. But, yes, it does make sense to follow webkit_web_settings_[get|set]_property_name convention

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:229
>> +                                                         "load-icon-ignoring-image-load-pref",
> 
> The name is long, but I think we should still use preference instead of pref

Done.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:248
>> +                                                         FALSE,
> 
> This is enabled by default in WebKit1, I think we should use the same default values to make the transition easier and consistent.

Done.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:360
>> +                                                         TRUE,
> 
> This is FALSE by default in WebKit1

Done.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:374
>> +                                                         TRUE,
> 
> This is FALSE by default in WebKit1

Done

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:406
>> + * Returns: TRUE - If Java Script is enabled.
> 
> Use %TRUE and remove the -

Done

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:412
>> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), false);
> 
> The function returns a gboolean, use FALSE instead of false here.

Done

>> Source/WebKit2/UIProcess/API/gtk/tests/testwebcontext.c:27
>> +    g_assert(webkit_web_context_get_default() == webkit_web_context_get_default());
> 
> This change is unrelated. I already fixed it in bug https://bugs.webkit.org/show_bug.cgi?id=67931.

Yes, I realized it after submitting the patch and added new one later excluding this change

[Martin Robinson]

Comment on attachment 108311 [details]
WebKitWebSetting GTK+ API

View in context: https://bugs.webkit.org/attachment.cgi?id=108311&action=review

We should decide whether we want the documention on the properties or the methods and then link back from the other place.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:76
> +    PROP_ENABLE_JAVA_SCRIPT,

JavaScript is camel case, but it's always written as one word.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:201
> +                                    g_param_spec_boolean(

No need for a newline here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:218
> +                                    g_param_spec_boolean(

No need for a newline here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:234
> +                                    g_param_spec_boolean(

No need for a newline here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:332
> +                                    g_param_spec_boolean(

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:407
> + * webkit_web_settings_get_enable_java_script:

JavaScript is always written as one word.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:409
> + * Check if Java Script execution within a page is supported or not.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:411
> + * Returns: %TRUE If Java Script is enabled.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:413
> + * @see_also: webkit_web_settings_set_enable_java_script

Ditto. Etc. :)

[Nayan Kumar K]

Created attachment 108444 [details]
WebKitWebSettings GTK+ API

[Carlos Garcia Campos]

(In reply to comment #20)
> (From update of attachment 108311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108311&action=review
> 
> We should decide whether we want the documention on the properties or the methods and then link back from the other place.
> 

Yes, I think we could add complete documentation in properties, and a simple line like set property foo, get property foo in the methods, where foo links to the property.

[Alejandro G. Castro]

Comment on attachment 108444 [details]
WebKitWebSettings GTK+ API

View in context: https://bugs.webkit.org/attachment.cgi?id=108444&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:43
> +        /* By default, offline-web-application-cache in true in WebKit-GTK */

s/in/is/

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:606
> +gboolean webkit_web_settings_get_database_enabled(WebKitWebSettings* settings)

The name does not follow the pattern, it is even linked in the next function with a different name in the doc.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:29
> +    // Scripts is enabled by default

Dot at the end of the comment.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:39
> +    // By default auto-load-image is true

Ditto.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:49
> +    // load-icon-ignoring-image-load-pref is false by default

Ditto, there are a lot, check all of them.

[Nayan Kumar K]

Created attachment 108640 [details]
WebKitWebSettings

Updated the patch, incorporating review comments.

[Nayan Kumar K]

Created attachment 108659 [details]
WebKitWebSetting API

[Nayan Kumar K]

Created attachment 109004 [details]
WebKitWebSettings GTK+ API

Header file inclusion is changed to <webkit2/webkit2.h>, instead of webkit2gtk.h.

[WebKit Review Bot]

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

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

Last 3072 characters of output:
/API/gtk/WebKitWebSettings.cpp:588:  webkit_web_settings_set_enable_html5_local_storage is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:612:  webkit_web_settings_get_enable_html5_database is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:629:  webkit_web_settings_set_enable_html5_database is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:653:  webkit_web_settings_get_enable_xss_auditor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:670:  webkit_web_settings_set_enable_xss_auditor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:694:  webkit_web_settings_get_enable_frame_flattening is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:711:  webkit_web_settings_set_enable_frame_flattening is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:735:  webkit_web_settings_get_enable_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:752:  webkit_web_settings_set_enable_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:776:  webkit_web_settings_get_enable_java is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:793:  webkit_web_settings_set_enable_java is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:817:  webkit_web_settings_get_javascript_can_open_windows_automatically is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:834:  webkit_web_settings_set_javascript_can_open_windows_automatically is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:858:  webkit_web_settings_get_enable_hyperlink_auditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:875:  webkit_web_settings_set_enable_hyperlink_auditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 80 in 6 files


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

[Nayan Kumar K]

Created attachment 109140 [details]
WebKitWebSettings GTK+ API added.

1. Rebase'd the patch to latest trunk.
2. Corrected the format of ChangeLog
3. Clubbed all tests together to a single function.

[WebKit Review Bot]

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

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

Last 3072 characters of output:
/API/gtk/WebKitWebSettings.cpp:588:  webkit_web_settings_set_enable_html5_local_storage is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:612:  webkit_web_settings_get_enable_html5_database is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:629:  webkit_web_settings_set_enable_html5_database is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:653:  webkit_web_settings_get_enable_xss_auditor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:670:  webkit_web_settings_set_enable_xss_auditor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:694:  webkit_web_settings_get_enable_frame_flattening is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:711:  webkit_web_settings_set_enable_frame_flattening is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:735:  webkit_web_settings_get_enable_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:752:  webkit_web_settings_set_enable_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:776:  webkit_web_settings_get_enable_java is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:793:  webkit_web_settings_set_enable_java is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:817:  webkit_web_settings_get_javascript_can_open_windows_automatically is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:834:  webkit_web_settings_set_javascript_can_open_windows_automatically is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:858:  webkit_web_settings_get_enable_hyperlink_auditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:875:  webkit_web_settings_set_enable_hyperlink_auditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 80 in 6 files


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

[Nayan Kumar K]

Created attachment 109141 [details]
WebKitWebSettings GTK+ API

[WebKit Review Bot]

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

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

Last 3072 characters of output:
/API/gtk/WebKitWebSettings.cpp:588:  webkit_web_settings_set_enable_html5_local_storage is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:612:  webkit_web_settings_get_enable_html5_database is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:629:  webkit_web_settings_set_enable_html5_database is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:653:  webkit_web_settings_get_enable_xss_auditor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:670:  webkit_web_settings_set_enable_xss_auditor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:694:  webkit_web_settings_get_enable_frame_flattening is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:711:  webkit_web_settings_set_enable_frame_flattening is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:735:  webkit_web_settings_get_enable_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:752:  webkit_web_settings_set_enable_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:776:  webkit_web_settings_get_enable_java is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:793:  webkit_web_settings_set_enable_java is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:817:  webkit_web_settings_get_javascript_can_open_windows_automatically is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:834:  webkit_web_settings_set_javascript_can_open_windows_automatically is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:858:  webkit_web_settings_get_enable_hyperlink_auditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:875:  webkit_web_settings_set_enable_hyperlink_auditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 80 in 6 files


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

[Carlos Garcia Campos]

Comment on attachment 109141 [details]
WebKitWebSettings GTK+ API

View in context: https://bugs.webkit.org/attachment.cgi?id=109141&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:41
> +        preferences = WKPreferencesCreate();

Why did you remove default values initialization?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:402
> + * Returns: %TRUE If JavaScript is enabled.
> + *          %FALSE If JavaScript is not enabled.

This looks a bit redundant, maybe you could use something like %TRUE if JavaScript is enabled, %FALSE otherwise.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebsettings.c:100
> +    return g_test_run ();

extra space there.

[Nayan Kumar K]

Comment on attachment 109141 [details]
WebKitWebSettings GTK+ API

View in context: https://bugs.webkit.org/attachment.cgi?id=109141&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:41
>> +        preferences = WKPreferencesCreate();
> 
> Why did you remove default values initialization?

Property set function gets called when we install the property using g_object_class_install_property, with the default value passed as g_param_spec (http://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html#g-object-class-install-property), thus setting the WebKit preference to the one specified via g_param_spec. Hence it is not necessary to initialize them separately.

[Nayan Kumar K]

Created attachment 109143 [details]
WebKitWebSettings GTK+ API.

Addressed review comments.

[WebKit Review Bot]

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

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

Last 3072 characters of output:
/API/gtk/WebKitWebSettings.cpp:588:  webkit_web_settings_set_enable_html5_local_storage is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:612:  webkit_web_settings_get_enable_html5_database is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:629:  webkit_web_settings_set_enable_html5_database is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:653:  webkit_web_settings_get_enable_xss_auditor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:670:  webkit_web_settings_set_enable_xss_auditor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:694:  webkit_web_settings_get_enable_frame_flattening is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:711:  webkit_web_settings_set_enable_frame_flattening is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:735:  webkit_web_settings_get_enable_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:752:  webkit_web_settings_set_enable_plugins is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:776:  webkit_web_settings_get_enable_java is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:793:  webkit_web_settings_set_enable_java is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:817:  webkit_web_settings_get_javascript_can_open_windows_automatically is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:834:  webkit_web_settings_set_javascript_can_open_windows_automatically is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:858:  webkit_web_settings_get_enable_hyperlink_auditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:875:  webkit_web_settings_set_enable_hyperlink_auditing is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 80 in 6 files


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

[Carlos Garcia Campos]

(In reply to comment #33)
> (From update of attachment 109141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109141&action=review
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:41
> >> +        preferences = WKPreferencesCreate();
> > 
> > Why did you remove default values initialization?
> 
> Property set function gets called when we install the property using g_object_class_install_property, with the default value passed as g_param_spec (http://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html#g-object-class-install-property), thus setting the WebKit preference to the one specified via g_param_spec. Hence it is not necessary to initialize them separately.

property_set function is called on an object instance, while g_object_class_install_property() is called on the object class struct, so there isn't any instance to call set_property when installing properties for a class.

[Nayan Kumar K]

> > Property set function gets called when we install the property using g_object_class_install_property, with the default value passed as g_param_spec (http://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html#g-object-class-install-property), thus setting the WebKit preference to the one specified via g_param_spec. Hence it is not necessary to initialize them separately.
> 
> property_set function is called on an object instance, while g_object_class_install_property() is called on the object class struct, so there isn't any instance to call set_property when installing properties for a class.

But, what I observed was, *_set functions of properties get called when a new instance of WebKitWebSettings gets created via webkit_web_settings_new. Below is the backtrace,

Breakpoint 1, 0x00007ffff68085b0 in webkit_web_settings_set_enable_offline_web_application_cache () from /home/xc0ffee/Sources/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
(gdb) bt
#0  0x00007ffff68085b0 in webkit_web_settings_set_enable_offline_web_application_cache () from /home/xc0ffee/Sources/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#1  0x00007ffff3c558ff in object_set_property (type=<value optimized out>, n_construct_properties=8, construct_params=0x45e2d0) at /build/buildd/glib2.0-2.28.6/./gobject/gobject.c:1185
#2  g_object_constructor (type=<value optimized out>, n_construct_properties=8, construct_params=0x45e2d0) at /build/buildd/glib2.0-2.28.6/./gobject/gobject.c:1629
#3  0x00007ffff3c58389 in g_object_newv (object_type=4417680, n_parameters=<value optimized out>, parameters=<value optimized out>) at /build/buildd/glib2.0-2.28.6/./gobject/gobject.c:1479
#4  0x00007ffff3c5963c in g_object_new (object_type=4417680, first_property_name=0x0) at /build/buildd/glib2.0-2.28.6/./gobject/gobject.c:1308
#5  0x00007ffff68081aa in webkit_web_settings_new () from /home/xc0ffee/Sources/WebKit/WebKitBuild/Release/.libs/libwebkit2gtk-3.0.so.0
#6  0x0000000000401536 in testWebKitWebSettings ()
#7  0x00007ffff33adc89 in test_case_run (suite=0x435f40, path=0x7ffff33fd3fe "") at /build/buildd/glib2.0-2.28.6/./glib/gtestutils.c:1174
#8  g_test_run_suite_internal (suite=0x435f40, path=0x7ffff33fd3fe "") at /build/buildd/glib2.0-2.28.6/./glib/gtestutils.c:1223
#9  0x00007ffff33addf6 in g_test_run_suite_internal (suite=<value optimized out>, path=0x7ffff33fd3fe "") at /build/buildd/glib2.0-2.28.6/./glib/gtestutils.c:1233
#10 0x00007ffff33addf6 in g_test_run_suite_internal (suite=<value optimized out>, path=0x7ffff33fd3fe "") at /build/buildd/glib2.0-2.28.6/./glib/gtestutils.c:1233
#11 0x00007ffff33ae0cb in g_test_run_suite (suite=0x435f00) at /build/buildd/glib2.0-2.28.6/./glib/gtestutils.c:1282

[Carlos Garcia Campos]

(In reply to comment #37)
> > > Property set function gets called when we install the property using g_object_class_install_property, with the default value passed as g_param_spec (http://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html#g-object-class-install-property), thus setting the WebKit preference to the one specified via g_param_spec. Hence it is not necessary to initialize them separately.
> > 
> > property_set function is called on an object instance, while g_object_class_install_property() is called on the object class struct, so there isn't any instance to call set_property when installing properties for a class.
> 
> But, what I observed was, *_set functions of properties get called when a new instance of WebKitWebSettings gets created via webkit_web_settings_new. Below is the backtrace,
> 

That's because G_PARAM_CONSTRUCT is used, I thought we were not using it. In that case you are right, sorry for the noise.

[Martin Robinson]

Comment on attachment 109143 [details]
WebKitWebSettings GTK+ API.

View in context: https://bugs.webkit.org/attachment.cgi?id=109143&action=review

I think this class should be called either WebKitSettings (preferred) or WebKitWebViewSettings. Below I've made a lot of suggestions for rephrasing simple descriptions. In reality, I just want the phrasing to be consistent either use:

Determines whether or not <foo> is enabled or
Controls whether or not 

In general, this looks like it's almost done.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:36
> +public:

A struct doesn't need a private label.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:37
> +    WKPreferencesRef preferences;

Should this be a WKRefPtr?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:54
> + * /<!-- -->* Create a new websettings and disable java script *<!-- -->/

java script => JavaScipt

The comment is missing a period.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:56
> + * g_object_set (G_OBJECT(settings), "enable-scripts", FALSE, NULL);

enable-scripts -> enable-javascript

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:58
> + * /<!-- -->* Apply the result *<!-- -->/

You can omit this comment.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:59
> + * webkit_web_view_set_settings (WEBKIT_WEB_VIEW(my_webview), settings);

There should be a space after WEBKIT_WEB_VIEW

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

pspec should be paramSpec.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:130
> +static void webkitWebSettingsGetProperty(GObject* object, guint propId, GValue* value, GParamSpec* pspec)

Ditto.

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

Please give this variable a more descriptive name such as readWriteConstructParamFlags.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:190
> +    * This property needs to be set to enable the execution of 
> +    * Javascript within a page.
> +    *

This description is misleading. It suggests that the embedders must touch this to enable script to run on a page. I think a sentence like "Determines whether or not JavaScript executes within a page."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:196
> +                                                         _("Enable JavaScript languages."),

s/languages.//

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:206
> +    * Determines whether images should be automatically loaded or not.
> +    * On devices, where network bandwidth is of concern, it might be
> +    * useful to turn this property off.
> +    *

No commoa necessary after "On devices"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:219
> +    * By enabling this property, site icons (e.g, favicon) will be loaded

I think it's fine to just say favicons directly.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:220
> +    * irrespective of the value of #Object:auto-load-images.

#Object Should be #WebKitSettings, right?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:226
> +                                                         _("Loads Site Icons ignoring image load preference"),

The capitalization of the blurb is inconsistent here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:236
> +    * Whether to enable HTML5 offline web application cache support. Offline
> +    * Web Application Cache ensures web applications are available even when
> +    * the user is not connected to the network.

Offline Web Application Cache -> The offline web application cache

I think it would be useful to link to the spec here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:242
> +                                                         _("Enable offline web application cache"),

Isn't the blurb supposed to be capitalized?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:251
> +    * Whether to enable HTML5 localStorage support. localStorage provides
> +    * simple synchronous storage access.

localStorage should be local storage. Please link to the spec.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:269
> +    * Whether to enable HTML5 client-side SQL database support. Client-side
> +    * SQL database allows web pages to store structured data and be able to
> +    * use SQL to manipulate that data asynchronously.
> +    *
> +    */

Please link to the spec.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:281
> +    * Whether to enable the XSS Auditor. This feature filters some kinds of

Auditor -> auditor

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:299
> +    * On touch devices, it is desired to not have any scrollable sub parts of the page as

sub parts -> subparts

I think the last two sentences should be simplified to say: On touch devices, scrollable subframes on a page can result in a confusing user experience.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:309
> +                                                         _("Whether to enable Frame Flattening"),

Frame Flattening should not be capitalized here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:317
> +    * Plugins emdedded within a page can be turned off by disabling
> +    * this property. 

"Determines whether or not plugins on the page are enabled."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:333
> +    * Enable or disable support for the Java. This property needs to be enabled
> +    * for displaying java contents via  &lt;object&gt; or &lt;embed&gt; tag.
> +    *

"Determines whether or not Java is enabled on the page." You can omit the second sentence.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:353
> +                                                         _("JavaScript can open windows automatically"),

Shouldn't the blurb be capitalized?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:361
> +    * Enable or disable support for &lt;a ping&gt;.

Might want to link to the spec here as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:379
> +    WebKitWebSettingsPrivate* priv = G_TYPE_INSTANCE_GET_PRIVATE(settings, WEBKIT_TYPE_WEB_SETTINGS, WebKitWebSettingsPrivate);
> +    settings->priv = priv;
> +    new (priv) WebKitWebSettingsPrivate();

You need to call the WebKitWebSettingsPrivate destructor somewhere too.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:411
> +
> +    WebKitWebSettingsPrivate* priv = settings->priv;
> +    return WKPreferencesGetJavaScriptEnabled(priv->preferences);

This can be one line. Please make the same changes for all similar methods below.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:428
> +    gboolean currentValue = WKPreferencesGetJavaScriptEnabled(priv->preferences);

Store the value as a bool.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:432
> +    WKPreferencesSetJavaScriptEnabled(priv->preferences, (bool)enabled);

Please don't cast to bool here. If it produces a warning (which I don't think it should) use a static_cast. Please make these same changes for all similar methods below.

[Martin Robinson]

(In reply to comment #39)

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebSettings.cpp:36
> > +public:u
> 
> A struct doesn't need a private label.

Sorry. I meant public label.

[Nayan Kumar K]

Created attachment 110515 [details]
Adds WebKitSettings GTK+ API's.

[Nayan Kumar K]

Created attachment 110637 [details]
WebKitSettings patch

Updated as per review comments.

[Nayan Kumar K]

Created attachment 110646 [details]
WebKitSettings patch

Modified unit test as per new the new unit test design.

[WebKit Review Bot]

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

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

Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 7 files


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

[Nayan Kumar K]

Created attachment 110648 [details]
WebKitSettings patch

Missed adding bug number in previous patch.

[Martin Robinson]

Comment on attachment 110648 [details]
WebKitSettings patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110648&action=review

Okay! This is so close!

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:46
> +    _WebKitSettingsPrivate()
> +    {
> +        preferences = adoptWK(WKPreferencesCreate());
> +    }

Do you mind removing this contructor and just moving this line to webkit_settings_init? This is a neat approach, but we sould be doing it the same way in all classes.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:50
> + * SECTION:webkitwebsettings

This heading is now wrong. It should be webkitsettings.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:60
> + * g_object_set (G_OBJECT(settings), "enable-java-script", FALSE, NULL);

You're missing a space after G_OBJECT. This should be enable-java-script.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:74
> +    PROP_LOADS_ICONS_IGNORING_IMAGE_LOAD_PREFERENCE,

Do you mind changing this setting to PROP_LOAD_ICONS_IGNORING_IMAGE_LOAD_SETTING. Note that LOADS is now LOAD. I changed PREFERENCE to setting here because in the C API these are "preferences," but in our API they are "settings."

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:239
> +    * WebKitSettings:loads-icons-ignoring-image-load-preference
> +    *
> +    * Determines whether a site can load favicons irrespective
> +    * of the value of #WebSettings:auto-load-images.
> +    *
> +    */
> +    g_object_class_install_property(gObjectClass,
> +                                    PROP_LOADS_ICONS_IGNORING_IMAGE_LOAD_PREFERENCE,
> +                                    g_param_spec_boolean("loads-icons-ignoring-image-load-preference",
> +                                                         _("Loads site icons ignoring image load preference"),
> +                                                         _("Whether to load site icon ignoring image load preference."),
> +                                                         FALSE,
> +                                                         readWriteConstructParamFlags));
> +

Please change this setting to load-icons-ignoring-image-load-setting. The string "Whether to load site icon ignoring image load preference." should be "Whether to load site icons ignoring image load setting." Note that icons is plural now.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:244
> +    * web application cache ensures web applications are available even when

ensure to "allow" maybe. Ensure suggests that it works with all web applications automatically.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:266
> +    * http://dev.w3.org/TR/webstorage/.

This link doesn't work! Looks like the real link is http://dev.w3.org/html5/webstorage/

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:317
> +    * On touch devices, scrollable subframes on a page can result in a confusing user experience.

No comma necessary after "On touch devices"

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:376
> +    * Hyperlink auditing specification is available at

"The hyperlink auditing"...

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:414
> + * Get the property #WebKitSettings:enable-javascript.

Should be "Get the #WebKitSettings:enable-javascript property." Please make this fix for all properties below as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:419
> + * @see_also: webkit_settings_set_enable_javascript()

I think it's more useful to link to the property here since that's where the description of the feature is. Since you've already linked above, you can probably remove this line if you like. Please make this fix for all properties below as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:454
> + * Get the property #WebKitSettings:auto-load-images.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:458
> + * @see_also: webkit_settings_set_auto_load_images()

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:504
> + * webkit_settings_get_loads_icons_ignoring_image_load_preference:
> + * @settings: a #WebKitSettings
> + *
> + * Get the property #WebKitSettings:loads-icons-ignoring-image-load-preference.
> + *
> + * Returns: %TRUE If site icon can be loaded irrespective of image loading preference or %FALSE otherwise.
> + *
> + * @see_also: webkit_settings_set_loads_icons_ignoring_image_load_preference()
> + **/
> +gboolean webkit_settings_get_loads_icons_ignoring_image_load_preference(WebKitSettings* settings)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_SETTINGS(settings), FALSE);
> +
> +    return WKPreferencesGetLoadsSiteIconsIgnoringImageLoadingPreference(settings->priv->preferences.get());
> +}

Don't forget to update preference -> setting here.

[Nayan Kumar K]

Created attachment 110804 [details]
WebKitSettings patch

[Nayan Kumar K]

Created attachment 110805 [details]
Adds WebKitSettings GTK+ API's.

[Martin Robinson]

Comment on attachment 110805 [details]
Adds WebKitSettings GTK+ API's.

View in context: https://bugs.webkit.org/attachment.cgi?id=110805&action=review

I'll land this one.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:636
> + * Returns: %TRUE If xss auditing is enabled or %FALSE otherwise.

xss -> XSS I'll fix it when landing.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:672
> + *.

Extra period here, but I'll fix when landing.

[Martin Robinson]

Created attachment 110951 [details]
Patch

[Martin Robinson]

Committed r97438: <http://trac.webkit.org/changeset/97438>

[Ryosuke Niwa]

It seems like this API test is crashing on a bot:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/26850/steps/API%20tests/logs/stdio

[Ryosuke Niwa]

32-bit bot: http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Debug/builds/17723/steps/API%20tests/logs/stdio

[Martin Robinson]

The test is skipped here: https://bugs.webkit.org/show_bug.cgi?id=70129
The bug is here: https://bugs.webkit.org/show_bug.cgi?id=70127