[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 #84897|review?                     |review-
               Flag|                            |




--- Comment #8 from Martin Robinson <mrobinson at webkit.org>  2011-03-25 09:52:33 PST ---
(From update of attachment 84897)
View in context: https://bugs.webkit.org/attachment.cgi?id=84897&action=review

I sincerely apologize for the very late review! This change looks quite good, though I have recommend some stylistic changes below. Please ensure that all comments are complete sentences and also begin with a capital letter and end with a period. Great work!

> Tools/GtkLauncher/main.c:257
> +/**
> +  * Callback which is called when parsing option entries.
> +  * Sets WebKitWebSettings properties
> +  */

Thanks for documenting this function. Instead of having the documentation here though, I'd rather the function be given a longer name. Something like parseOptionEntryCallback.

> Tools/GtkLauncher/main.c:259
> +gboolean parseWebSettings(const gchar *optionNameFull, const gchar *value,
> +                            gpointer data, GError **error)

The indentation seems off here. You can just put them all on one line.

> Tools/GtkLauncher/main.c:284
> +    if (strlen(optionNameFull) > 2 && webkitSettings) {
> +        /* we have two -- in option name so remove them*/
> +        const gchar *optionName = optionNameFull + 2;
> +        GParamSpec *spec = g_object_class_find_property(G_OBJECT_GET_CLASS(webkitSettings),
> +                                                    optionName);
> +        if (spec) {
> +            /* convert string to proper type */
> +            GValue valueString = {0, {{0}}};
> +            GValue valueProperty = {0, {{0}}};
> +            g_value_init(&valueString, G_TYPE_STRING);
> +            g_value_init(&valueProperty, G_PARAM_SPEC_VALUE_TYPE(spec));
> +            g_value_set_static_string(&valueString, value);
> +            if (g_value_transform(&valueString, &valueProperty)) {
> +                /* set WebKitWebSettings properties */
> +                g_object_set_property(G_OBJECT(webkitSettings), optionName, &valueProperty);
> +                parsedCorrectly = TRUE;
> +            }
> +        }
> +    }
> +    return parsedCorrectly;
> +}

Typically in WebKit we use early returns.  If you did that you could remove the parsedCorrectly variable altogether.  I'm imaginging something like this:

if (strlen(optionNameFull <= 2) || !webkitSettings)
    return FALSE;

const gchar *optionName = optionNameFull + 2;
GParamSpec *spec = g_object_class_find_property(G_OBJECT_GET_CLASS(webkitSettings), optionName);
if (!spec)
    returrn FALSE;

etc, etc.

> Tools/GtkLauncher/main.c:290
> +/**
> +  * Creates GArray of GOptionEntries basing
> +  * on the WebKitWebSettings writable properties
> +  */
> +static GArray* getOptionEntries(WebKitWebSettings *webkitSettings)

Again you could remove the comment and simply say, getOptionEntriesFromWebKitWebSettings.

> Tools/GtkLauncher/main.c:296
> +    propertySpecs = g_object_class_list_properties
> +                        (G_OBJECT_GET_CLASS(webkitSettings), &numProperties);

It's okay for your lines to extend to about 120 characters before wrapping. I suggest doing that simply to match the style we've already established in WebKitGTK+.

> Tools/GtkLauncher/main.c:299
> +    if (optionEntriesArray) {

Here again I recommend an early return.

> Tools/GtkLauncher/main.c:302
> +            /* only for writable properties fill in structures */

Please make your comments complete sentences with a capital letter and period at the end.

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

You should also include support for floats.

> Tools/GtkLauncher/main.c:332
> +static void
> +transformStringToBoolean(const GValue *srcValue, GValue *destValue)

Please remove the extra newline.

> Tools/GtkLauncher/main.c:345
> +static void
> +transformStringToInt(const GValue *srcValue, GValue *destValue)

Ditto.

> Tools/GtkLauncher/main.c:366
> +/******************* additional options parsing ***********************************************/

Again, it makes more sense to move all this to a helper static function called parseAdditionalOptions rather than having this big comment.

> Tools/GtkLauncher/main.c:377
> +    GOptionGroup *webSettingsGroup = g_option_group_new("websettings",
> +                                        "WebKitWebSettings writable properties for default WebKitWebView",
> +                                        "WebKitWebSettings properties",
> +                                        webkitSettings, NULL);
> +    g_option_group_add_entries(webSettingsGroup, (GOptionEntry*) optionEntriesArray->data);

When indenting like this I prefer the arguments to all be lined up with the first one.

> Tools/GtkLauncher/main.c:380
> +    GOptionContext *context = g_option_context_new("[URL]");
> +    g_option_context_add_group(context, webSettingsGroup);

Why "[URL]" ?

> Tools/GtkLauncher/main.c:389
> +    GError *error = NULL;
> +    if (!g_option_context_parse(context, &argc, &argv, &error)) {
> +        g_print("Failed to parse arguments: %s\n", error->message);
> +        g_error_free(error);
> +        g_option_context_free(context);
> +        g_array_free(optionEntriesArray, TRUE);
> +        return 1;
> +    }

Excellent error handling!

> Tools/GtkLauncher/main.c:392
> +/***********************************************************************************************/

Please remove this and just move all the above code to a helper function.

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