[webkit-reviews] review denied: [Bug 16219] [GTK] API: WebKitWebSettings is not usable : [Attachment 18469] Updated, aware of screen changes

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Christian Dywan
<christian at imendio.com>'s request for review:
Bug 16219: [GTK] API: WebKitWebSettings is not usable
http://bugs.webkit.org/show_bug.cgi?id=16219

Attachment 18469: Updated, aware of screen changes
http://bugs.webkit.org/attachment.cgi?id=18469&action=edit

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
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.