Bug 73773 - [WK2][GTK] WebSettings support in MiniBrowser
Summary: [WK2][GTK] WebSettings support in MiniBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-04 09:06 PST by Philippe Normand
Modified: 2012-01-03 00:11 PST (History)
4 users (show)

See Also:


Attachments
WebSettings support in MiniBrowser (8.27 KB, patch)
2011-12-04 09:09 PST, Philippe Normand
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
WebSettings support in MiniBrowser (8.65 KB, patch)
2012-01-02 09:12 PST, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2011-12-04 09:06:54 PST
Would be nice to be able to pass settings via the command line like in the wk1 launcher.
Comment 1 Philippe Normand 2011-12-04 09:09:13 PST
Created attachment 117791 [details]
WebSettings support in MiniBrowser
Comment 2 Martin Robinson 2011-12-04 09:40:59 PST
Related bug: https://bugs.webkit.org/show_bug.cgi?id=71568
Comment 3 WebKit Review Bot 2011-12-04 09:45:26 PST
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
Comment 4 Philippe Normand 2011-12-04 09:46:18 PST
(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.
Comment 5 Carlos Garcia Campos 2011-12-16 01:41:31 PST
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.
Comment 6 Carlos Garcia Campos 2011-12-16 01:42:17 PST
(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
Comment 7 Carlos Garcia Campos 2011-12-16 01:43:39 PST
(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 :-)
Comment 8 Philippe Normand 2011-12-16 08:04:58 PST
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
Comment 9 Carlos Garcia Campos 2011-12-22 04:12:32 PST
(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.
Comment 10 Philippe Normand 2012-01-02 04:27:13 PST
I'll update this one when the UI patch lands.
Comment 11 Philippe Normand 2012-01-02 09:12:17 PST
Created attachment 120887 [details]
WebSettings support in MiniBrowser
Comment 12 Martin Robinson 2012-01-02 10:16:48 PST
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"));
Comment 13 Philippe Normand 2012-01-03 00:11:05 PST
Committed r103928: <http://trac.webkit.org/changeset/103928>