Bug 63060 – [GTK] Use GOption to parse main arguments in GtkLauncher

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Now that we are using GOption for the websettings options, we could use it also for the main arguments. It will make easier to support passing more than one uri from the command line like MiniBrowser does.

[Carlos Garcia Campos]

Created attachment 97956 [details]
Patch

It also simplifies and improves a bit the code to parse the websettings arguments, see changelog for more details.

[Martin Robinson]

Comment on attachment 97956 [details]
Patch

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.

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

Created attachment 98144 [details]
Updated patch according to review

[Martin Robinson]

Comment on attachment 98144 [details]
Updated patch according to review

View in context: https://bugs.webkit.org/attachment.cgi?id=98144&action=review

Great stuff. it doesn't seem like settings will "stick" for other WebViews opened unfortunatelyi. I guess we can fix that in a followup patch since it's sort of unrelated to this.

> Tools/GtkLauncher/main.c:379
> +    static WebKitWebSettings *webkitSettings = 0;
> +#endif
> +    static const gchar **uriArguments = 0;
> +    static const GOptionEntry commandLineOptions[] =

If I'm not mistaken these do not have to be static. Please check before landing.

[Carlos Garcia Campos]

Committed r89432: <http://trac.webkit.org/changeset/89432>

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 98144 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98144&action=review
> 
> Great stuff. it doesn't seem like settings will "stick" for other WebViews opened unfortunatelyi. I guess we can fix that in a followup patch since it's sort of unrelated to this.

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

> > Tools/GtkLauncher/main.c:379
> > +    static WebKitWebSettings *webkitSettings = 0;
> > +#endif
> > +    static const gchar **uriArguments = 0;
> > +    static const GOptionEntry commandLineOptions[] =
> 
> If I'm not mistaken these do not have to be static. Please check before landing.

Right, not necessarily, removed already. Thanks!