Bug 73773 – [WK2][GTK] WebSettings support in MiniBrowser

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

[Philippe Normand]

Would be nice to be able to pass settings via the command line like in the wk1 launcher.

[Philippe Normand]

Created attachment 117791 [details]
WebSettings support in MiniBrowser

[Martin Robinson]

Related bug: https://bugs.webkit.org/show_bug.cgi?id=71568

[WebKit Review Bot]

Comment on attachment 117791 [details]
WebSettings support in MiniBrowser

Attachment 117791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10689591

New failing tests:
svg/custom/linking-uri-01-b.svg

[Philippe Normand]

(In reply to comment #2)
> Related bug: https://bugs.webkit.org/show_bug.cgi?id=71568

Argh! Seems like they're "complementary" though. I like the command line stuff but also understand a UI is nice to have.

[Carlos Garcia Campos]

Comment on attachment 117791 [details]
WebSettings support in MiniBrowser

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

I know this has been copied from GtkLauncher, so maybe can land this, and then fix the possible issus in GtkLauncher and MiniBrowser at the same time.

> Tools/MiniBrowser/gtk/main.c:87
> +        if (value && g_ascii_strcasecmp(value, "true") && strcmp(value, "1"))

I don't undertand this line g_ascii_strcasecmp(value, "true") && strcmp(value, "1"), I don't know what the intention is, but the value can't be "true" and "1".

> Tools/MiniBrowser/gtk/main.c:144
> +        return 0;

We can use NULL in C.

> Tools/MiniBrowser/gtk/main.c:152
> +        if (!param || !(param->flags & G_PARAM_WRITABLE))

I'm not sure we need this, all properties in webkit settings are READWRITE. But if we leave the check we should skip also construct only properties, since you can't do g_object_set on them.

> Tools/MiniBrowser/gtk/main.c:157
> +        if (gParamType == G_TYPE_BOOLEAN || gParamType == G_TYPE_STRING || gParamType == G_TYPE_INT
> +            || gParamType == G_TYPE_FLOAT) {

I would use a help function here, and continue if the type is not valid, 

if (!paramTypeIsSupported(gParamType))
    continue;

> Tools/MiniBrowser/gtk/main.c:177
> +static gboolean addSettingsGroupToContext(GOptionContext *context, WebKitSettings* webkitSettings)

WebKitSettings* webkitSettings -> WebKitSettings *webkitSettings

> Tools/MiniBrowser/gtk/main.c:208
> +        webkitSettings = 0;

We can use NULL in C.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 117791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117791&action=review
> 
> I know this has been copied from GtkLauncher, so maybe can land this, and then fix the possible issus in GtkLauncher and MiniBrowser at the same time.

or even better, fix GtkLauncher first and then land the right patch for MiniBrowser

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #2)
> > Related bug: https://bugs.webkit.org/show_bug.cgi?id=71568
> 
> Argh! Seems like they're "complementary" though. I like the command line stuff but also understand a UI is nice to have.

Yes, they are indeed complementary, I had in mind to write this patch when the UI one lands :-)

[Philippe Normand]

Comment on attachment 117791 [details]
WebSettings support in MiniBrowser

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

>> Tools/MiniBrowser/gtk/main.c:87
>> +        if (value && g_ascii_strcasecmp(value, "true") && strcmp(value, "1"))
> 
> I don't undertand this line g_ascii_strcasecmp(value, "true") && strcmp(value, "1"), I don't know what the intention is, but the value can't be "true" and "1".

IIUC, this tests if value != true and value != 1

[Carlos Garcia Campos]

(In reply to comment #8)
> (From update of attachment 117791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117791&action=review
> 
> >> Tools/MiniBrowser/gtk/main.c:87
> >> +        if (value && g_ascii_strcasecmp(value, "true") && strcmp(value, "1"))
> > 
> > I don't undertand this line g_ascii_strcasecmp(value, "true") && strcmp(value, "1"), I don't know what the intention is, but the value can't be "true" and "1".
> 
> IIUC, this tests if value != true and value != 1

right!, I always forget !strcmp(a,b) mean a == b :-P that's why I always use == 0, but it's not allowed by webkit coding style, so nevermind, sorry for the noise.

[Philippe Normand]

I'll update this one when the UI patch lands.

[Philippe Normand]

Created attachment 120887 [details]
WebSettings support in MiniBrowser

[Martin Robinson]

Comment on attachment 120887 [details]
WebSettings support in MiniBrowser

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

Looks good, but please consider the following cleanup before landing...

> Tools/MiniBrowser/gtk/main.c:30
>  #include <webkit2/webkit2.h>

I know it was wrong before, but do you mind making sure that this is in alphabetical order now?

> Tools/MiniBrowser/gtk/main.c:45
> +static void loadURI(const gchar *uri, WebKitSettings *webkitSettings)

Please rename this to something like createBrowserWindow.

> Tools/MiniBrowser/gtk/main.c:88
> +        gboolean propertyValue = TRUE;
> +        if (value && g_ascii_strcasecmp(value, "true") && strcmp(value, "1"))
> +            propertyValue = FALSE;

This can just be: 

gboolean propertyValue = !(value && g_ascii_strcasecmp(value, "true") && strcmp(value, "1"));

[Philippe Normand]

Committed r103928: <http://trac.webkit.org/changeset/103928>