[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





--- Comment #12 from Lukasz Slachciak <l.slachciak at samsung.com>  2011-06-18 05:41:06 PST ---
(From update of attachment 90875)
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

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