[webkit-reviews] review denied: [Bug 68371] [GTK][WEBKIT2] Add WebKitSettings GTK+ API. : [Attachment 110648] WebKitSettings patch

[bugzilla-daemon at webkit.org]

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

Attachment 110648: WebKitSettings patch
https://bugs.webkit.org/attachment.cgi?id=110648&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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->pr
eferences.get());
> +}

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