[webkit-reviews] review denied: [Bug 68371] [GTK][WEBKIT2] Add WebKitWebSettings GTK+ API : [Attachment 109143] WebKitWebSettings GTK+ API.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 10 22:54:35 PDT 2011


Martin Robinson <mrobinson at webkit.org> has denied Nayan Kumar K
<nayankk at motorola.com>'s request for review:
Bug 68371: [GTK][WEBKIT2] Add WebKitWebSettings GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=68371

Attachment 109143: WebKitWebSettings GTK+ API.
https://bugs.webkit.org/attachment.cgi?id=109143&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.


More information about the webkit-reviews mailing list