Bug 14809 - [gtk] Implement settings as properties of WebKitGtkPage
Summary: [gtk] Implement settings as properties of WebKitGtkPage
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 523.x (Safari 3)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 16219
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-29 15:47 PDT by Christian Dywan
Modified: 2009-01-12 15:37 PST (History)
2 users (show)

See Also:


Attachments
An attempt at gobject properties (5.37 KB, patch)
2007-10-20 17:57 PDT, Christian Dywan
no flags Details | Formatted Diff | Diff
Many more properties, style cleanup (14.11 KB, patch)
2007-10-25 18:47 PDT, Christian Dywan
no flags Details | Formatted Diff | Diff
wrong file (2.95 KB, patch)
2007-11-15 18:35 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
g_object_prefs set and get (14.32 KB, text/x-patch)
2007-12-03 01:16 PST, Salvatore De Paolis
no flags Details
webkit: set_property and get_property (13.80 KB, patch)
2007-12-03 10:41 PST, Salvatore De Paolis
mrowe: review-
Details | Formatted Diff | Diff
Updated (17.36 KB, patch)
2007-12-30 10:41 PST, Christian Dywan
alp: review-
Details | Formatted Diff | Diff
Site using CSS system colors in Safari 3.0.4 (OK) (31.48 KB, image/jpeg)
2008-02-13 05:25 PST, Adam Strzelecki
no flags Details
Site using CSS system colors in latest WebKit r30153 (BAD, most of elements are BLACK) (26.75 KB, image/jpeg)
2008-02-13 05:26 PST, Adam Strzelecki
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Dywan 2007-07-29 15:47:54 PDT
We don't currently have a way of retrieving or modifying settings (fonts, colors, etc.). The proper way would be implementing a property of WebKitGtkPage for every value, along with get and set functions in the style webkit_gtk_set_variable_font.
Comment 1 Christian Dywan 2007-10-20 17:57:29 PDT
Created attachment 16749 [details]
An attempt at gobject properties

This is a first attempt at implementing WebCore:Settings as properties of WebKitPage. This is intentionally only six selected properties because I would like someone to look at it before repeating bad code very often.

I plan to implement every value of WebCore::Settings. I don't know wether there should be getters and setters for all of them, just a few for the most common values or none at all.

Comments are welcome.
Comment 2 Christian Dywan 2007-10-25 18:47:21 PDT
Created attachment 16870 [details]
Many more properties, style cleanup

This revision of the patch implements many more properties. Further more the code was cleaned up style wise.
Comment 3 Christian Dywan 2007-11-15 18:35:23 PST
Created attachment 17308 [details]
wrong file

This changes the behavior of the script-prompt to return NULL when the dialog is cancelled. This matches other browsers, tested with Firefox and Opera on a number of websites.

Besides I changed a bit of the dialog code to avoid compiler warnings regarding uninitialized values.

At last I modifed the console message default to prevent linkification in terminals to include the line number in the link.
Comment 4 Christian Dywan 2007-11-16 19:52:30 PST
(In reply to comment #3)
> Created an attachment (id=17308) [edit]
> Correct script prompt return value and avoid warrings.
> [snip]

Nevermind my mistake, this patch doesn't belong to this bug.
Comment 5 Xan Lopez 2007-11-25 07:34:22 PST
(In reply to comment #2)
> Created an attachment (id=16870) [edit]
> Many more properties, style cleanup
> 
> This revision of the patch implements many more properties. Further more the
> code was cleaned up style wise.
> 

Some comments:

- PROP_0 is generally used instead of PROP_NULL.
- Unneeded blank lines in the switch constructs.
- The first parameter of the param_spec functions is the canonical name of the property, which has to follow some syntactic rules. The second is the "nickname", which is generally the canonical name with s/-/ / and properly capitalized. The third one is a human readable description explaining what this property is about.
- You use lots of magic numbers :|

Other than that, looks good to me.
Comment 6 Salvatore De Paolis 2007-12-03 01:16:01 PST
Created attachment 17668 [details]
g_object_prefs set and get

This is the Christian's patch adapted to new API and xan advices.
About comments, I wish they could be enhance by someone else.
Comment 7 Salvatore De Paolis 2007-12-03 10:41:06 PST
Created attachment 17679 [details]
webkit: set_property and get_property
Comment 8 Mark Rowe (bdash) 2007-12-03 10:51:31 PST
Comment on attachment 17679 [details]
webkit: set_property and get_property

I think PROP_IMAGES_LOAD_AUTOMATICALLY and "auto-images-load" should be PROP_AUTO_LOAD_IMAGES and "auto-load-images" respectively.  "auto-images-load" doesn't make sense as-is.

The default fonts you have specified, amongst other things, do not appear to match those used in webkit_web_view_init.  Is there a reason for this?

In webkit_web_view_class_init  there needs to be a blank line before the property PROP_FONT_SIZE_DEFAULT_FIXED is created to be consistent.
Comment 9 Christian Dywan 2007-12-30 10:41:11 PST
Created attachment 18194 [details]
Updated

I would like to move on with this, so I'm updating the patch. Opinions welcome.
Comment 10 Maciej Stachowiak 2008-01-01 22:53:29 PST
The code itself looks good to me (though I'm not fresh on Gtk+ programming) but I think there's a needed API design decision here, whether to copy the Mac port's separate settings object, or expose GObject properties right on the view, or both.
Comment 11 Alp Toker 2008-01-01 23:01:37 PST
Comment on attachment 18194 [details]
Updated

Thanks Christian for getting this patch back in action.

There's been some good discussion over the merits of having settings on WebView vs. a separate settings object and I think we should go for the latter now.

So it's best to have these properties on WebKitWebSettings.

As you suggested, we can look into having them on both objects later on. There are a couple of techniques we can use to do this, but I don't think we should do it just yet.
Comment 12 Adam Strzelecki 2008-02-13 05:25:11 PST
Created attachment 19110 [details]
Site using CSS system colors in Safari 3.0.4 (OK)
Comment 13 Adam Strzelecki 2008-02-13 05:26:06 PST
Created attachment 19111 [details]
Site using CSS system colors in latest WebKit r30153 (BAD, most of elements are BLACK)
Comment 14 Adam Strzelecki 2008-02-13 05:28:23 PST
Shit... sorry stupid buzilla moved myself to next bug, I wanted create attachments.... sorry again.
Comment 15 Adam Strzelecki 2008-02-13 05:31:56 PST
Comment on attachment 19111 [details]
Site using CSS system colors in latest WebKit r30153 (BAD, most of elements are BLACK)

Sorry wrong bug, Bugzilla is moving me to next bug without asking so... wrrr
Comment 16 Adam Strzelecki 2008-02-13 05:32:12 PST
Comment on attachment 19110 [details]
Site using CSS system colors in Safari 3.0.4 (OK)

Sorry wrong bug, Bugzilla is moving me to next bug without asking so... wrrr
Comment 17 Gustavo Noronha (kov) 2009-01-12 15:37:34 PST
I believe this no longer applies, since we now have WebKitWebSettings.