[Webkit-unassigned] [Bug 55308] [GTK] General mechanism for adjusting WebKitWebSettings in GtkLauncher.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55308


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #90875|review?                     |review-
               Flag|                            |




--- Comment #11 from Martin Robinson <mrobinson at webkit.org>  2011-04-26 15:57:39 PST ---
(From update of attachment 90875)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.