Bug 16219 – [GTK] API: WebKitWebSettings is not usable

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

[Alp Toker]

The necessary entry points to use WebKitWebSettings are not implemented. We need to decide if we want to expose settings like this or if we want to adapt the idea to something more suited for GTK+ developers.

Christian originally raised this question but hasn't yet written up the bug so
I went ahead and did so.

[Christian Dywan]

Created attachment 18385 [details]
Implement WebKitWebSettings

[Christian Dywan]

Created attachment 18386 [details]
Testing the new object with GtkLauncher

A little bit of testing and demonstration code

[Christian Dywan]

Created attachment 18390 [details]
Updated implementation of WebKitWebSettings

Removing unneeded header includes.

Adding default Cursive and Fantasy fonts based on font usage surveys.

Implementing webkit_web_settings_copy.

[Christian Dywan]

Created attachment 18391 [details]
Testing with GtkLauncher (updated)

For the sake of completeness updated test code.

[Alp Toker]

Comment on attachment 18390 [details]
Updated implementation of WebKitWebSettings

This looks great. A few comments:

There's whitespace before open parentheses in some places (enough for me to ask for you to re-submit before landing).

I'd like Xan to look over this as well before landing.

Eventually I hope we'll use Pango objects and data types for some of the font stuff but we can change that later, better not to complicate things now.

[Christian Dywan]

Created attachment 18435 [details]
Removed whitespace, dpi, user-stylesheet

I removed whitespace in different places and updated the code to retrieve the screen resolution from gdk.

And I renamed user-stylesheet-path to just user-stylesheet since it really is about only one file and not a folder. Incidentally I would find an actual user-stylesheet-path referring to a folder worth a thought, but that's out of the scope of this bug.

[Alp Toker]

+                 "scripts", priv->scripts,
+                 "plugins", priv->plugins,

How about "enable-scripts" and "enable-plugins"?

[Christian Dywan]

Created attachment 18464 [details]
Updated according to discussion

Added "enable-" to "scripts" and "plugins".

Renamed "fixed" to "monospace".

[Christian Dywan]

Created attachment 18469 [details]
Updated, aware of screen changes

Updated to use the default screen in case the widget is not mapped and connect to screen-changed to update font sizes.

[Mark Rowe (bdash)]

Comment on attachment 18469 [details]
Updated, aware of screen changes

There are a few things which need improved here:

Using UTF-8 as the default value for the encoding has compatibility implications.  WebKit on Mac and Windows defaults to ISO-8859-1 (latin-1).  What is the reason for differing from Mac and Windows?

Using "Sans" as the default font family has compatibility implications.  WebKit on Mac and Windows default to Times, which is a serif font family.  What is the reason for using a sans-serif family here?

I haven't looked closely but I think several of the other defaults also differ from Mac and Windows.  It'd be worth looking over these and providing reasoning as to why they should differ for Gtk.

I think the description and possibly the name of "user-stylesheet" is unclear.  The setting is for the URI of the user stylesheet, some clients may expect this to be the CSS data or similar.

In webkit_web_settings_get_property the switch statement has the { on the incorrect line.

webkit_web_settings_copy sets its web_settings argument to the return value.  That doesn't seem correct as it has no effect.

The *-placement of the gchar* declarations in webkit_web_view_update_settings is not correct.  We don't typically declare more than one variable per line, but in this case you're declaring 8 in one go so I'm not sure what the best approach is.

1088     settings->setDOMPasteAllowed(true);

This is most definitely not desirable.  Allowing access to the DOM clipboard can be a security issue, and it should be off by default.  Specific applications, such as DumpRenderTree, may want to enable it in some contexts so it should be possible for a client application to enable it, but web browsers will rarely want to do this.

There are coding style issues in webkit_web_view_settings_notify, particularly the lack of white space in "if(".  The sheer number of chained ifs feels icky to me too.

[Christian Dywan]

Created attachment 18500 [details]
Updated according to Mark's comments

(In reply to comment #10)
> (From update of attachment 18469 [details] [edit])

> Using UTF-8 as the default value for the encoding has compatibility
> implications.  WebKit on Mac and Windows defaults to ISO-8859-1 (latin-1). 
> What is the reason for differing from Mac and Windows?
> 
> Using "Sans" as the default font family has compatibility implications.
> WebKit on Mac and Windows default to Times, which is a serif font family.
> What is the reason for using a sans-serif family here?
> 
> I haven't looked closely but I think several of the other defaults also
> differ from Mac and Windows.  It'd be worth looking over these and providing
> reasoning as to why they should differ for Gtk.

The reason is that WebKit is not meant to be used in a web browser exclusively but as a general purpose view. It should be suitable for various use cases, most of which won't ever display arbitrary websites. The conclusion is that the defaults should be native values whereas the client still has the option to use 'compatible' values as desired.

> The *-placement of the gchar* declarations in webkit_web_view_update_settings
> is not correct.  We don't typically declare more than one variable per line,
> but in this case you're declaring 8 in one go so I'm not sure what the best
> approach is.

No idea how to improve that really. Declaring those in single lines would be overkill.

> I think the description and possibly the name of "user-stylesheet" is
> unclear. The setting is for the URI of the user stylesheet, some clients
> may expect this to be the CSS data or similar.

After a little chat with Mark I am going for "user-stylesheet-uri".

[Darin Adler]

(In reply to comment #11)
> The reason is that WebKit is not meant to be used in a web browser exclusively
> but as a general purpose view. It should be suitable for various use cases,
> most of which won't ever display arbitrary websites. The conclusion is that the
> defaults should be native values whereas the client still has the option to use
> 'compatible' values as desired.

OK, but the same situation exists on Mac; I think you should consider having the compatible values be the defaults as we chose to do on Mac OS X.

[Xan Lopez]

+    GParamFlags flags = (GParamFlags)(G_PARAM_READWRITE | G_PARAM_CONSTRUCT);

Add G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB ?

+    g_object_class_install_property(G_OBJECT_CLASS(klass),

You already have gobject_class declared above

+    WebKitWebSettingsPrivate* priv = WEBKIT_WEB_SETTINGS_GET_PRIVATE(web_settings);
+
+    priv->default_encoding = NULL;
+    priv->cursive_font_family = NULL;
+    priv->default_font_family = NULL;
+    priv->fantasy_font_family = NULL;
+    priv->monospace_font_family = NULL;
+    priv->sans_serif_font_family = NULL;
+    priv->serif_font_family = NULL;
+    priv->user_stylesheet_uri = NULL;

Private data is zeroed already.

+    WebKitWebSettingsPrivate* priv = WEBKIT_WEB_SETTINGS_GET_PRIVATE(web_settings);

Use the priv pointer you are already declaring in the instance struct instead of GET_PRIVATE all the time, it's much faster (remember to initialize the pointer on _init though!). g_type_class_add_private was meant to be used in the cases where you can't modify the instance struct, but if you can it's good practice to have the priv pointer there and use it in the implementation. How you implement that is not really relevant, I use g_type_class_add_private usually too.

+    WebKitWebSettings* web_settings = WEBKIT_WEB_SETTINGS(g_object_new(WEBKIT_TYPE_WEB_SETTINGS, NULL));
+
+    return web_settings;

return g_object_new(WEBKIT_TYPE_WEB_SETTINGS, NULL); ? :)

As the function is returning WebKitWebSettings* the result of g_object_new will be casted for you already AFAIK.

+    GObject parent;

GObject parent_instance;

+                                                        WEBKIT_PARAM_READWRITE));

Hey, w00t, we have this already. You should use it in the settings file.

+    guint DPI = gdk_screen_get_resolution(screen);

This can return -1, does it make sense to check?

+static void webkit_web_view_update_settings(WebKitWebView* webView)
+{
+    g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));
+

In general static functions shouldn't have type checks. Check in the API entry point.

[Christian Dywan]

Created attachment 18524 [details]
Updated according to Xan's comments

Updated according to Xan's comments and discussion on IRC.

I made gdk_screen_get_resolution conditional since it is >= 2.10.

[Christian Dywan]

Created attachment 18526 [details]
Updated, better ChangeLog

[Alp Toker]

(In reply to comment #15)
> Created an attachment (id=18526) [edit]
> Updated, better ChangeLog
> 

Some issues bdash pointed out:

there are still some issues with default values for the settings

missing documentation for the new API methods

minor coding style issues in webkitwebsettings.cpp

{-placement on the struct + enum at the top of the file

redundancy in webkit_web_settings_new

[Alp Toker]

Comment on attachment 18526 [details]
Updated, better ChangeLog

r=me

I'll fix up the issues.

The prop strings in at least one of the props are the wrong way round.

I hope this gets more testing and documentation once landed.

[Alp Toker]

Landed in r29723. We need to keep thinking about the defaults, prop names etc. They aren't necessarily final, and docs are still needed.

[Zan Dobersek]

Created attachment 24541 [details]
Proposition for simplifying use of WebKitWebSettings

That's the whole idea, discuss, please.

[Christian Dywan]

(In reply to comment #19)
> Created an attachment (id=24541) [edit]
> Proposition for simplifying use of WebKitWebSettings
> 
> That's the whole idea, discuss, please.

I'm not sure what you are referring to with "the whole idea". In any case I don't think this useful. WebKitWebSettings is in a way a specialized relative of GtkSettings. The latter does not provide any accessors (disregarding the gtk_settings_set_* functions that are not supposed to be used).

If all you want is type checking, check out Gnome bug #541569 [1].

[1] http://bugzilla.gnome.org/show_bug.cgi?id=541569

[Zan Dobersek]

"the whole idea" was ment as simplification of use of WebKitWebSettings.
And after a quick rethink I came to conclusion this wouldn't be much of a simplification and am withdrawing from this idea.

Still, WebKitWebSettings are missing properties that are useful in some way and should be added, such as whether JavaScript can access clipboard et cetera.

[Christian Dywan]

Comment on attachment 24541 [details]
Proposition for simplifying use of WebKitWebSettings

(In reply to comment #21)
> "the whole idea" was ment as simplification of use of WebKitWebSettings.
> And after a quick rethink I came to conclusion this wouldn't be much of a
> simplification and am withdrawing from this idea.
> 
> Still, WebKitWebSettings are missing properties that are useful in some way and
> should be added, such as whether JavaScript can access clipboard et cetera.
> 

Clearing the review flag then to reflect the decision to drop the idea.

Please file a new bug or even individual bugs if appropriate for settings you need, ideally with a use case.