Summary: | [GTK] API: WebKitWebSettings is not usable | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alp Toker <alp> | ||||||||||||||||||||||||
Component: | WebKitGTK | Assignee: | Christian Dywan <christian> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | alp, christian, lars, tofu.linden, zan | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||
Bug Blocks: | 14809 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Alp Toker
2007-11-30 20:38:09 PST
Created attachment 18385 [details]
Implement WebKitWebSettings
Created attachment 18386 [details]
Testing the new object with GtkLauncher
A little bit of testing and demonstration code
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.
Created attachment 18391 [details]
Testing with GtkLauncher (updated)
For the sake of completeness updated test code.
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.
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.
+ "scripts", priv->scripts, + "plugins", priv->plugins, How about "enable-scripts" and "enable-plugins"? Created attachment 18464 [details]
Updated according to discussion
Added "enable-" to "scripts" and "plugins".
Renamed "fixed" to "monospace".
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.
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.
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". (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. + 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. 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.
Created attachment 18526 [details]
Updated, better ChangeLog
(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 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.
Landed in r29723. We need to keep thinking about the defaults, prop names etc. They aren't necessarily final, and docs are still needed. Created attachment 24541 [details]
Proposition for simplifying use of WebKitWebSettings
That's the whole idea, discuss, please.
(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 "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. 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. |