Bug 72468 – [WebKit2][gtk] Add few more properties to WebKitSettings

• Component: WebKit2
• Status: RESOLVED FIXED

[Nayan Kumar K]

Add following properties to WebKitSettings,

1. enable-private-browsing
2. enable-developer-extras
3. enable-resizable-text-areas
4. enable-tabs-to-links

[Nayan Kumar K]

Created attachment 115335 [details]
Addition of new properties

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 115335 [details]
Addition of new properties

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h:245
> +                                                                gboolean enabled);

We use GNOME coding style in public headers, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
It means the * should be adjacent to the settings and parameter identifiers are lined up. So this should be:

webkit_settings_set_enable_private_browsing                    (WebKitSettings *settings,
                                                                                                 gboolean            enabled);

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 115335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115335&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h:245
> > +                                                                gboolean enabled);
> 
> We use GNOME coding style in public headers, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
> It means the * should be adjacent to the settings and parameter identifiers are lined up. So this should be:
> 
> webkit_settings_set_enable_private_browsing                    (WebKitSettings *settings,
>                                                                                                  gboolean            enabled);

Ok formatting was not good, just look at other method in that file, like webkit_settings_set_enable_java as reference.

[Mario Sanchez Prada]

The patch looks good to me, with the exceptions Carlos already pointed out. About placing the *'s in the right place, I think the new patch should have them placed correctly, even if other functions already present in that file have it misplaced too (including those about caret browsing I've introduced yesterday that are also wrong, oops!).

We could always make a patch later to normalize that file (and maybe others) anyway, but newly introduced code should be correct already (/me self-slaps himself because of the caretBrowsing related ones)

[Nayan Kumar K]

Created attachment 115347 [details]
Review comments incorporated

[Martin Robinson]

Comment on attachment 115347 [details]
Review comments incorporated

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

Loos fine to me other than some minor English nits.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:645
> +     * will disable maintaining history, cache, auto-fill information for any

You can just remove "maintaining" here.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:660
> +     * Determines whether or not developer extras is enabled. This enables
> +     * developers to make use of debugging tools such as Web Inspector.

You might just replace this whole description and say "Determines whether or not developer tools, such as the Web Inspector, are enabled.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:687
> +     * By enabling this setting, user will be able to focus the next element 

By enabling this setting, user -> When this setting is enabled, users

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:689
> +     * then pressing tab key will insert tab character.

insert tab character -> insert the tab character

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp:179
> +    // Private browsing is disabled by default.
> +    g_assert(!webkit_settings_get_enable_private_browsing(settings));
> +    webkit_settings_set_enable_private_browsing(settings, TRUE);
> +    g_assert(webkit_settings_get_enable_private_browsing(settings));
> +
> +    // Developer extras is disabled by default.
> +    g_assert(!webkit_settings_get_enable_developer_extras(settings));
> +    webkit_settings_set_enable_developer_extras(settings, TRUE);
> +    g_assert(webkit_settings_get_enable_developer_extras(settings));
> +
> +    // Text areas are resizable by default.
> +    g_assert(webkit_settings_get_enable_resizable_text_areas(settings));
> +    webkit_settings_set_enable_resizable_text_areas(settings, FALSE);
> +    g_assert(!webkit_settings_get_enable_resizable_text_areas(settings));
> +
> +    // Tabs to link is disabled by default.
> +    g_assert(!webkit_settings_get_enable_tabs_to_links(settings));
> +    webkit_settings_set_enable_tabs_to_links(settings, TRUE);
> +    g_assert(webkit_settings_get_enable_tabs_to_links(settings));
> +

I think you can omit the comments. It should be clear from the code.

[Nayan Kumar K]

Created attachment 115404 [details]
Review comments incorporated

[Martin Robinson]

Comment on attachment 115404 [details]
Review comments incorporated

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

Was there a reason you didn't take some of the suggestions. Not a problem if you prefer your original language, just curious. There are a few places that are missing articles though.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:645
> +     * Determines whether or not private browsing is enabled. Private browsing
> +     * will disable history, cache, auto-fill information for any pages visited.

Might want to be explicit about this."..cache, and form auto-fill for any pages visited.""

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:659
> +     * Determines whether or not developer extras is enabled. This enables
> +     * developers to make use of debugging tools such as Web Inspector.

You didn't change this one...

at least it should be "the Web Inspector."

[Nayan Kumar K]

Created attachment 115518 [details]
Review comments incorporated

[Nayan Kumar K]

(In reply to comment #9)
> (From update of attachment 115404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115404&action=review
> 
> Was there a reason you didn't take some of the suggestions. Not a problem if you prefer your original language, just curious. 

Sorry, it was by mistake.

[WebKit Review Bot]

Comment on attachment 115518 [details]
Review comments incorporated

Clearing flags on attachment: 115518

Committed r100580: <http://trac.webkit.org/changeset/100580>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.