[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 #9 from Lukasz Slachciak <l.slachciak at samsung.com>  2011-04-23 23:28:23 PST ---
(From update of attachment 84897)
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

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