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

Mon Oct 10 22:54:35 PDT 2011

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

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #109143|review?                     |review-
               Flag|                            |

--- Comment #39 from Martin Robinson <mrobinson at webkit.org>  2011-10-10 22:54:35 PST ---
(From update of attachment 109143)
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  <object> or <embed> 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 <a ping>.

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.

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