[webkit-reviews] review denied: [Bug 63060] [GTK] Use GOption to parse main arguments in GtkLauncher : [Attachment 97956] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 21 08:14:24 PDT 2011
Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 63060: [GTK] Use GOption to parse main arguments in GtkLauncher
https://bugs.webkit.org/show_bug.cgi?id=63060
Attachment 97956: Patch
https://bugs.webkit.org/attachment.cgi?id=97956&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97956&action=review
This approach seems to be more code, but I prefer it since it has much better
error handling. Thanks for the cleanup. I've just a few small quibbles below.
> Tools/GtkLauncher/main.c:38
> +#ifndef WEBKIT2
> +static WebKitWebSettings *webkitSettings = 0;
> +#endif
I dislike the use of a global here. This could simply be defined in main and
passed as a double pointer to addWebSettingsGroupToContext.
> Tools/GtkLauncher/main.c:252
> + g_set_error(error, G_OPTION_ERROR, G_OPTION_ERROR_FAILED,
> + "Invalid option %s", optionNameFull);
This can be one line, as it's less than 120 characters.
> Tools/GtkLauncher/main.c:261
> + g_set_error(error, G_OPTION_ERROR, G_OPTION_ERROR_FAILED,
> + "Cannot find web settings for option %s",
optionNameFull);
Ditto.
> Tools/GtkLauncher/main.c:265
> + /* Parse argument */
Can omit this comment entirely.
> Tools/GtkLauncher/main.c:273
> + }
> + break;
Why leave the break outside the block?
> Tools/GtkLauncher/main.c:285
> + g_set_error(error, G_OPTION_ERROR, G_OPTION_ERROR_BAD_VALUE,
> + "Integer value '%s' for %s out of range", value,
optionNameFull);
No need to break.
> Tools/GtkLauncher/main.c:290
> + g_set_error(error, G_OPTION_ERROR, G_OPTION_ERROR_BAD_VALUE,
> + "Cannot parse integer value '%s' for %s", value,
optionNameFull);
Ditto.
> Tools/GtkLauncher/main.c:295
> + }
> + break;
Again the break is outside the block. Does C require this?
> Tools/GtkLauncher/main.c:304
> + g_set_error(error, G_OPTION_ERROR, G_OPTION_ERROR_BAD_VALUE,
> + "Float value '%s' for %s out of range", value,
optionNameFull);
Could be one line if less than 120 characters.
> Tools/GtkLauncher/main.c:314
> + }
> + break;
Same as above.
> Tools/GtkLauncher/main.c:391
> +static const GOptionEntry commandLineOptions[] =
> +{
> + { G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY, &uriArguments,
0, "[URL]" },
> + { 0, 0, 0, 0, 0, 0, 0 }
> +};
Could this be moved inside main as well to avoid having to make uriArguments a
global?
> Tools/GtkLauncher/main.c:402
> + GOptionContext *context = g_option_context_new(NULL);
NULL -> 0 here.
More information about the webkit-reviews
mailing list