Bug 100106 - [Qt] Make default typesettings configurable
Summary: [Qt] Make default typesettings configurable
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks: 100863
  Show dependency treegraph
 
Reported: 2012-10-23 04:04 PDT by Allan Sandfeld Jensen
Modified: 2014-02-03 03:22 PST (History)
9 users (show)

See Also:


Attachments
Patch (3.69 KB, patch)
2012-10-23 04:06 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (14.29 KB, patch)
2012-10-23 06:36 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (14.84 KB, patch)
2012-10-23 07:16 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (18.24 KB, patch)
2012-10-23 09:07 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (18.15 KB, patch)
2012-10-29 04:20 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (33.11 KB, patch)
2012-10-31 07:43 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.49 KB, patch)
2012-11-15 04:46 PST, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-10-23 04:04:03 PDT
WebCore has the ability to draw fonts with kerning and common ligatures, and we support that in the Qt backend. It is however disabled by default and only enabled when websites explicitly enable it (the settings can basically be auto, on, off, and we let auto == off).

Safari already allows this setting to be user-configurable, and I believe we should give users the same option. We might later also enable it by default for printing.
Comment 1 Allan Sandfeld Jensen 2012-10-23 04:06:21 PDT
Created attachment 170114 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2012-10-23 06:36:50 PDT
Created attachment 170147 [details]
Patch

Made it configurable in WebKit2 by making the Mac-only setting generally available
Comment 3 Build Bot 2012-10-23 06:56:10 PDT
Comment on attachment 170147 [details]
Patch

Attachment 170147 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14487876
Comment 4 Allan Sandfeld Jensen 2012-10-23 07:16:50 PDT
Created attachment 170157 [details]
Patch
Comment 5 Allan Sandfeld Jensen 2012-10-23 09:07:55 PDT
Created attachment 170176 [details]
Patch

Expose setting in QtTestBrowser for visually testing
Comment 6 Pierre Rossi 2012-10-26 07:49:51 PDT
Comment on attachment 170176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=170176&action=review

> Source/WebKit/qt/Api/qwebsettings.cpp:492
> +    \value KerningAndLigaturesEnabledByDefault This setting enables font kerning and common ligatures everywhere they have not been explicitly disabled. This setting is disabled by default.

I think it's fine to expose this for people using QtWebKit to do things other than web browsers. As long as it is off by default it shouldn't hurt, but since it's gonna bring performance down quite dramatically when turned on, maybe there could be a mention of the implications setting this to true has.
The CSS way with "text-rendering: optimizeLegibility" seems more suited for people who have control over their content.
Maybe the docs should mention something about those trade-offs ?
Comment 7 Allan Sandfeld Jensen 2012-10-29 03:24:26 PDT
(In reply to comment #6)
> (From update of attachment 170176 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170176&action=review
> 
> > Source/WebKit/qt/Api/qwebsettings.cpp:492
> > +    \value KerningAndLigaturesEnabledByDefault This setting enables font kerning and common ligatures everywhere they have not been explicitly disabled. This setting is disabled by default.
> 
> I think it's fine to expose this for people using QtWebKit to do things other than web browsers. As long as it is off by default it shouldn't hurt, but since it's gonna bring performance down quite dramatically when turned on, maybe there could be a mention of the implications setting this to true has.
> The CSS way with "text-rendering: optimizeLegibility" seems more suited for people who have control over their content.

Maybe, but I don't think we support custum UA or even user stylesheets, and this is controlling the default when text-rendering mode is in default.

> Maybe the docs should mention something about those trade-offs ?

Good idea, I will add that.
Comment 8 Allan Sandfeld Jensen 2012-10-29 04:20:38 PDT
Created attachment 171201 [details]
Patch

Rebased and improved documentation
Comment 9 Pierre Rossi 2012-10-29 09:43:56 PDT
Comment on attachment 171201 [details]
Patch

Looks good to me, but I'm not a reviewer ;)
Comment 10 Jocelyn Turcotte 2012-10-31 04:28:05 PDT
Comment on attachment 171201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171201&action=review

> Source/WebKit/qt/Api/qwebsettings.h:86
> +        KerningAndLigaturesEnabledByDefault

I would leave the "ByDefault" part out for consistency with other parameters. The doc you added explains it well.

> Source/WebKit2/Shared/WebProcessCreationParameters.cpp:40
> +    , shouldEnableKerningAndLigaturesByDefault(false)

This patch should normally either not touch the WebKit2 part, or do something with this bool and expose it in QWebPreferences.

> Source/WebKit2/UIProcess/WebContext.cpp:390
> +    parameters.shouldEnableKerningAndLigaturesByDefault = m_shouldEnableKerningAndLigaturesByDefault;

We should probably remove that part from WebContextMac.mm
Comment 11 Allan Sandfeld Jensen 2012-10-31 05:02:25 PDT
(In reply to comment #10)
> (From update of attachment 171201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171201&action=review
> 
> > Source/WebKit/qt/Api/qwebsettings.h:86
> > +        KerningAndLigaturesEnabledByDefault
> 
> I would leave the "ByDefault" part out for consistency with other parameters. The doc you added explains it well.
> 
The problem is that it only changes the default. In most cases that means it will enable it, but web-sites can still override the default. I originally used a different name, but changed it to match the name of the Mac settings. 

> > Source/WebKit2/UIProcess/WebContext.cpp:390
> > +    parameters.shouldEnableKerningAndLigaturesByDefault = m_shouldEnableKerningAndLigaturesByDefault;
> 
> We should probably remove that part from WebContextMac.mm

What do you mean? The part that sets this in WebContextMac, overrides this setting and is needed to apply the Mac settings.

Mac will not be able to apply the setting at runtime with this system, but it will continue to work as well as it already does, and they are free to migrate to using the platform-independent settings later.

You are probably right the WebKit2 settings are not ready yet.
Comment 12 Allan Sandfeld Jensen 2012-10-31 07:43:22 PDT
Created attachment 171658 [details]
Patch

Moves the default typesetting features into page/Setting, and exports it qwebpreferences
Comment 13 WebKit Review Bot 2012-10-31 07:47:05 PDT
Attachment 171658 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2200:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Allan Sandfeld Jensen 2012-10-31 08:40:49 PDT
Comment on attachment 171658 [details]
Patch

I am going to split this patch in two.
Comment 15 Simon Hausmann 2012-11-02 02:21:08 PDT
In general I like the idea a lot.

While I understand the motivation of disabling kerning and ligatures for western languages by default in a browser, I'm personally a big fan of those features and I totally see that for example in an email reader application it is desirable to enable them by default (where the text layout performance is less critical). This is a decision that should be possible to control by the app developer.
Comment 16 Allan Sandfeld Jensen 2012-11-04 03:16:14 PST
(In reply to comment #15)
> In general I like the idea a lot.
> 
> While I understand the motivation of disabling kerning and ligatures for western languages by default in a browser, I'm personally a big fan of those features and I totally see that for example in an email reader application it is desirable to enable them by default (where the text layout performance is less critical). This is a decision that should be possible to control by the app developer.

We may need a separate API for it though. Apple are not going to allow us to make it a preference, because they have APIs using the text system without a Page and thus without Settings.

The question is then how we would do that in Qt. A global setting is essentially something set on QApplication level, but kerning is usually enabled by default in Qt, and a WebKit specific setting in QApplication would be a mess.

We may end up having to do it using user style-sheets though that is more error-prone.
Comment 17 Allan Sandfeld Jensen 2012-11-15 04:46:59 PST
Created attachment 174405 [details]
Patch

Changed the kerning setting to be global, since making it a real runtime setting has some opposition
Comment 18 Jocelyn Turcotte 2014-02-03 03:22:59 PST
=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.