[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