Bug 55308 – [GTK] General mechanism for adjusting WebKitWebSettings in GtkLauncher.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Lukasz Slachciak]

Added possibility for GtkLauncher to parse command line options.
As a first available option - frame flattening. On some sites (e.g. www.expedia.com) turning on this option causes layout problems.

[Lukasz Slachciak]

Created attachment 83953 [details]
GtkLauncher frame flattening option

[Lukasz Slachciak]

And there is no compatibility break.
You can still use GtkLauncher in the "old style" i.e. putting just URL on the command line.

[Martin Robinson]

I think I'd rather see some more general mechanism for adjusting WebKitWebSettings in GtkLauncher. One in which you can pass in property name and value strings and it would do the right thing. CCing some other GTK+ guys here.

[Lukasz Slachciak]

I'll think about it. We will need to distinguish between different types (boolean/string etc,) of properties passed by commandline.

[Lukasz Slachciak]

Created attachment 84887 [details]
[GTK] General mechanism for adjusting WebKitWebSettings in GtkLauncher

Following Martin's comments I created general mechanism for parsing WebKitWebSettings options.
It fills available GtkLauncher --help-websettings options with WebKitWebSettings group.
All writable string/int/boolean properties will be listed there.
User can pass by commandline any combination of them:
e.g.:
--enable-frame-flattening=1 --minimum-font-size=30 http://xahlee.org/js/frame/0.html
--user-agent="SomeStrangeUA" http://www.useragentstring.com --enable-spatial-navigation=1 --minimum-font-size=20	

Due to the future extensibility I decided to use G_OPTION_ARG_CALLBACK as option entry arg field. Thanks to callback function we can extend parsing mechanism for float and others types.

[Lukasz Slachciak]

Created attachment 84893 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

Additional fix/improvement in boolean values processing

[Lukasz Slachciak]

Created attachment 84897 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

memory releasing fix when parsing error occurs

[Martin Robinson]

Comment on attachment 84897 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

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.

[Lukasz Slachciak]

Comment on attachment 84897 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

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

Thank you martin for review. And sorry for late answer too. I'm attaching fixed version of patch.

>> Tools/GtkLauncher/main.c:257
>> +  */
> 
> 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.

done

>> Tools/GtkLauncher/main.c:259
>> +                            gpointer data, GError **error)
> 
> The indentation seems off here. You can just put them all on one line.

ok

>> Tools/GtkLauncher/main.c:284
>> +}
> 
> 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.

I need to change my programing style :) Done.

>> Tools/GtkLauncher/main.c:290
>> +static GArray* getOptionEntries(WebKitWebSettings *webkitSettings)
> 
> Again you could remove the comment and simply say, getOptionEntriesFromWebKitWebSettings.

Done.

>> Tools/GtkLauncher/main.c:296
>> +                        (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+.

Ok, 120 fills my laptop width

>> Tools/GtkLauncher/main.c:299
>> +    if (optionEntriesArray) {
> 
> Here again I recommend an early return.

ok

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

ok

>> Tools/GtkLauncher/main.c:308
>> +                                || gParamType == G_TYPE_INT) {
> 
> You should also include support for floats.

support for floats added, although only --zoom-step property is writable float one and there is no way to use zooming in GtkLauncher

>> Tools/GtkLauncher/main.c:332
>> +transformStringToBoolean(const GValue *srcValue, GValue *destValue)
> 
> Please remove the extra newline.

ok

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

ok

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

moved

>> Tools/GtkLauncher/main.c:377
>> +    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.

ok

>> Tools/GtkLauncher/main.c:380
>> +    g_option_context_add_group(context, webSettingsGroup);
> 
> Why "[URL]" ?

string param of g_option_context_new(string) is displayed in the first line of --help output, after programname [OPTION...].
I thought that it is useful to say that URL loading is also supported.

>> Tools/GtkLauncher/main.c:392
>> +/***********************************************************************************************/
> 
> Please remove this and just move all the above code to a helper function.

ok

[Lukasz Slachciak]

Created attachment 90875 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

[Martin Robinson]

Comment on attachment 90875 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

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.

[Lukasz Slachciak]

Comment on attachment 90875 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

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

>> Tools/GtkLauncher/main.c:240
>> +    if (strlen(optionNameFull) <= 2 || !webkitSettings)
> 
> When can this be null. Would it be better to use an
> 
> ASSERT(webkitSettings);
> 
> ?

I used g_assert(webkitSettings);

>> Tools/GtkLauncher/main.c:244
>> +    const gchar *optionName = optionNameFull + 2;
> 
> Why do we end up with two -- entries?

I don't add them. This is what I get from GOptionEntry mechanism, even if I set name without "--" I think that it is done "by design" to comply with command line options standard for long names.
For short names we have one "-"

>> Tools/GtkLauncher/main.c:245
>> +    GParamSpec *spec = g_object_class_find_property(G_OBJECT_GET_CLASS(webkitSettings),
> 
> This can be one line.

done

>> Tools/GtkLauncher/main.c:285
>> +        if (param && param->flags & G_PARAM_WRITABLE) {
> 
> Please use an early return here.

done

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

ok

>> Tools/GtkLauncher/main.c:291
>> +                /* Create option entry. */
> 
> This comment seems extraneous. Please remove it.

ok

>> Tools/GtkLauncher/main.c:294
>> +                   optionEntry.short_name=*/
> 
> Instead of just stating that there is no short name, it would be better to explain why.

explained in comment.

>> Tools/GtkLauncher/main.c:296
>> +                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.

extra space fixed, but I don't see which part could go to a helper function. GOptionEntry filling?

>> 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));

ok

>> Tools/GtkLauncher/main.c:332
>> +    g_value_set_float(destValue, atof(str));
> 
> Ditto making this one line.

ok

[Lukasz Slachciak]

Created attachment 97698 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

[Lukasz Slachciak]

Created attachment 97699 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

[WebKit Review Bot]

Attachment 97698 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/GtkLauncher/main..." exit_code: 1

Tools/GtkLauncher/main.c:253:  Missing space after ,  [whitespace/comma] [3]
Tools/GtkLauncher/main.c:295:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 97699 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/GtkLauncher/main..." exit_code: 1

Tools/GtkLauncher/main.c:253:  Missing space after ,  [whitespace/comma] [3]
Tools/GtkLauncher/main.c:295:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lukasz Slachciak]

Created attachment 97700 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

[Lukasz Slachciak]

Martin, finally after long break I made fixes according to your comments.

[Martin Robinson]

Comment on attachment 97700 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

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

Nice work! I'll land this one.

> Tools/GtkLauncher/main.c:280
> +        return NULL;

You should use 0 here instead of NULL.

> Tools/GtkLauncher/main.c:285
> +        return NULL;

Ditto.

> Tools/GtkLauncher/main.c:359
> +    GError *error = NULL;

You should use 0 instead of NULL unless you are ending an argument list. I'll fix this and land.

[Martin Robinson]

Committed r89249: <http://trac.webkit.org/changeset/89249>

[Martin Robinson]

Comment on attachment 97700 [details]
General mechanism for adjusting WebKitWebSettings in GtkLauncher

Clearing flag on landed bug.

[Lukasz Slachciak]

(In reply to comment #19)
> (From update of attachment 97700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97700&action=review
> 
> Nice work! I'll land this one.
> 
> > Tools/GtkLauncher/main.c:280
> > +        return NULL;
> 
> You should use 0 here instead of NULL.
> 
> > Tools/GtkLauncher/main.c:285
> > +        return NULL;
> 
> Ditto.
> 
> > Tools/GtkLauncher/main.c:359
> > +    GError *error = NULL;
> 
> You should use 0 instead of NULL unless you are ending an argument list. I'll fix this and land.

Too much C in my life :)
Thank you Martin for review.