[webkit-reviews] review denied: [Bug 55308] [GTK] General mechanism for adjusting WebKitWebSettings in GtkLauncher. : [Attachment 90875] General mechanism for adjusting WebKitWebSettings in GtkLauncher

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Lukasz Slachciak
<l.slachciak at samsung.com>'s request for review:
Bug 55308: [GTK] General mechanism for adjusting WebKitWebSettings in
GtkLauncher.
https://bugs.webkit.org/show_bug.cgi?id=55308

Attachment 90875: General mechanism for adjusting WebKitWebSettings in
GtkLauncher
https://bugs.webkit.org/attachment.cgi?id=90875&action=review

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

Looks good, but I have a few remaining nits. Thanks for updating it!

> Tools/GtkLauncher/main.c:240
> +    if (strlen(optionNameFull) <= 2 || !webkitSettings)

When can this be null. Would it be better to use an

ASSERT(webkitSettings);

?

> Tools/GtkLauncher/main.c:244
> +    const gchar *optionName = optionNameFull + 2;

Why do we end up with two -- entries?

> Tools/GtkLauncher/main.c:245
> +    GParamSpec *spec =
g_object_class_find_property(G_OBJECT_GET_CLASS(webkitSettings),

This can be one line.

> Tools/GtkLauncher/main.c:252
> +    GValue valueString = {0, {{0}}};
> +    GValue valueProperty = {0, {{0}}};

GValue is such an annoying API. :)

> Tools/GtkLauncher/main.c:285
> +	   if (param && param->flags & G_PARAM_WRITABLE) {

Please use an early return here.

> Tools/GtkLauncher/main.c:287
> +	       /* Only int, boolean, float and string writable properties are
supported. */

You can remove this comment, as it's obvious from the code that follows.

> Tools/GtkLauncher/main.c:289
> +	       if (gParamType == G_TYPE_BOOLEAN || gParamType == G_TYPE_STRING
|| gParamType == G_TYPE_INT
> +		   || gParamType == G_TYPE_FLOAT) {

Ditto.

> Tools/GtkLauncher/main.c:291
> +		   /* Create option entry. */

This comment seems extraneous. Please remove it.

> Tools/GtkLauncher/main.c:294
> +		   /* No short name for optionEntry.
> +		      optionEntry.short_name=*/

Instead of just stating that there is no short name, it would be better to
explain why.

> Tools/GtkLauncher/main.c:296
> +		   /* For bool arguments "enable" type make option argument not
required. */
> +		   if (gParamType == G_TYPE_BOOLEAN && (strstr
(optionEntry.long_name, "enable")))

You have an extra space here after strstr. Maybe ths should be separated out
into a helper.

> Tools/GtkLauncher/main.c:326
> +    g_value_set_int(destValue, atoi(str));

Please just make this:

g_value_set_int(destValue, atoi(g_value_get_string(srcValue));

> Tools/GtkLauncher/main.c:332
> +    const char* str = g_value_get_string(srcValue);
> +    g_value_set_float(destValue, atof(str));

Ditto making this one line.