[webkit-reviews] review denied: [Bug 68996] [GTK][WEBKIT2] Add font and charset properties to WebKitWebSettings : [Attachment 114305] WebKitSettings patch with new APIs added in gtk-doc sections file

[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 68996: [GTK][WEBKIT2] Add font and charset properties to WebKitWebSettings
https://bugs.webkit.org/show_bug.cgi?id=68996

Attachment 114305: WebKitSettings patch with new APIs added in gtk-doc sections
file
https://bugs.webkit.org/attachment.cgi?id=114305&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114305&action=review


Looks okay, but I have some nittish things below.

The minimum font size controls how the absolute smallest size fonts will be
rendered. If I'm not mistaken there is another setting
default-minimum-logical-font-size. This patch should probably include that as
well. Some notes in general for the rest of the patch:

1. parameter names in the headers should_be_like_this, but shouldBeLikeThis in
the C++ files. gtkdoc comments should reflect the names of the paramters in the
headers.
2. The string that comes after parameters are not complete sentences so should
not begin with a capital letter.
3. Please run Source/WebKit2/UIProcess/API/gtk/docs/build-gtkdoc and ensure
your patch does not introduce any new warnings.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:559
> +    * The default font size (in pixels) to use for content displayed if

"in pixels" does not need to be in parenthesis.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:568
> +							 5, G_MAXINT, 12,

It's probably best just to set the minimum value as  zero, unless there's a
reason to use 5 that you can see. This parameter is a uint, so the maximum
should probably be G_MAXUINT, right?

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:574
> +    * The default font size (in pixels) to use for content displayed in

"in pixels" does not need to be in parenthesis.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:583
> +							 5, G_MAXINT, 10,

Again, unless there's a reason you think that 5 should be the minimum, it's
probably best to let it go to zero. G_MAXUINT here as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:589
> +    * The minimum font size (in points) used to display text.

Ditto. You should mention that values other than zero can potentially break
page layouts.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:597
> +							 0, G_MAXINT, 5,

Again, G_MAXUINT here as well. The default minimum font size should be zero.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:603
> +    * The default text charset used when interpreting content whose charset
is not specified.

This should read: "The default text charset used when interpreting content with
an unspecified charset."

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1115
> + * Get the #WebKitSettings:default-font-family.

Please change this to "Gets the #WebKitSettings:default-font-family property"
Please make similar changes to methods below.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1124
> +    WebKitSettingsPrivate* priv = settings->priv;
> +    return priv->defaultFontFamily.data();

This can be one line. Please make similar changes  below.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1130
> + * @defaultFontFamily: String to be set

This should read "default_font_family: the new default font family" The first
word should not be capitalized. The parameter name here should match the one
specified in the header. The parameter names in the header should use
this_kind_of_case. Please make similar changes below.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1132
> + * Set the #WebKitSettings:default-font-family.

Set the #WebKitSettings:default-font-family *property*. Please make similar
changes below.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1134
> +void webkit_settings_set_default_font_family(WebKitSettings* settings, const
gchar* defaultFontFamily)

defaultFontFamily should be called defaultFontFamily here even though the
parameter name in the header is default_font_family.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1141
> +    if (!g_ascii_strcasecmp(priv->defaultFontFamily.data(),
defaultFontFamily))
> +	   return;

How about g_string_equal instead of g_ascii_strcasecmp. I don't think there's
any guarantee that font names are ASCII and I think we should support changing
the case. Please make similar changes below.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1491
> +    WKPreferencesSetMinimumFontSize(priv->preferences.get(),
(uint32_t)fontSize);

Is it necessary to cast here? Please try removing the cast and checking if it
creates a warning. If it does result in a warning, it's fine to cast, but you
must always use C++ style casts in WebKit code. Please make the same
check/change in other places in this patch you use casts.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitSettings.cpp:42
> +

This seems like it was added accidentally. Please remove this.