[Webkit-unassigned] [Bug 63060] [GTK] Use GOption to parse main arguments in GtkLauncher

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com>  2011-06-21 08:41:03 PST ---
(In reply to comment #2)
> (From update of attachment 97956 [details])
> 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.

Thanks for reviewing.

> > 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.

That's the usual way of using GOption, I'll try to use a local variable. 

> > 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.

Ok.

> > 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.

Ok.

> > Tools/GtkLauncher/main.c:265
> > +    /* Parse argument */
> 
> Can omit this comment entirely.

Ok.

> > Tools/GtkLauncher/main.c:273
> > +    }
> > +        break;
> 
> Why leave the break outside the block?

No reason, I'm simply used to do that, but for no particular reason.

> > 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.

Ok.

> > 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.

Ok.

> > Tools/GtkLauncher/main.c:295
> > +    }
> > +        break;
> 
> Again the break is outside the block. Does C require this?

No, I'll move it inside.

> > 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.

Ok.

> > 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?

Yes, I guess.

> > Tools/GtkLauncher/main.c:402
> > +    GOptionContext *context = g_option_context_new(NULL);
> 
> NULL -> 0 here.

Ok

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