Bug 100863 – Make defaultTypesettingFeatures a proper setting

• Status: NEW
• Component: Platform

[Allan Sandfeld Jensen]

While exporting the option to set better default typesetting features to Qt, I ran into the issue that the setting for default typesetting features is a static setting in Font.h rather than a proper page setting. This means its effect is global rather than localized to the current page or context, and that changing it at runtime will not update the rendering automatically.

I solved the issue in a patch for bug #100106, but have decided it would be better to land that part of the patch as a separate bug.

[Allan Sandfeld Jensen]

Created attachment 171669 [details]
Patch

Comment on attachment 171669 [details]
Patch

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

This breaks Font::setDefaultTypesettingFeatures().

> Source/WebCore/platform/graphics/FontDescription.h:76
> +    enum KerningState { AutoKerning, NormalKerning, NoneKerning };

This renaming appears unrelated to what this patch is trying to do, so it doesn’t belong in this patch.

> Source/WebCore/platform/graphics/FontDescription.h:238
> +    unsigned m_defaultTypesettingFeatures : 2; // Used to control what auto kerning and normal ligatures implies.

What makes this a property of a FontDescription?

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (From update of attachment 171669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171669&action=review
> 
> This breaks Font::setDefaultTypesettingFeatures().
> 
No, but it does deprecate it. The default value used in Settings is Font::defaultTypesettingFeatures. It is not pretty, but it should work in the existing cases. The patch in bug #100106 also ports WK2 to stop using Font::setDefaultTypesettingFeatures(), which just leaves Mac WK1 which I didn't touch because it is in ObjC.

> > Source/WebCore/platform/graphics/FontDescription.h:76
> > +    enum KerningState { AutoKerning, NormalKerning, NoneKerning };
> 
> This renaming appears unrelated to what this patch is trying to do, so it doesn’t belong in this patch.
> 
Without it you will get namespace clashes. The problem could also be solved by qualifying Kerning with full namespace, but it made more sense to me to just fix the underlying problem, rather than commiting odd code that break the coding style.

> > Source/WebCore/platform/graphics/FontDescription.h:238
> > +    unsigned m_defaultTypesettingFeatures : 2; // Used to control what auto kerning and normal ligatures implies.
> 
> What makes this a property of a FontDescription?

That other properties of FontDescription depend on it to be fully resolved, see FontDescription::typesettingFeatures(). 

Secondly saving this in FontDescription fits how RenderingMode and usePrinterFont is passed to FontDescription. Both of these are also page settings. And all in all they these settings take up a lot less space than saving a pointer in there instead.

[Allan Sandfeld Jensen]

Created attachment 171682 [details]
Patch

Avoid rename and get completely rid of Font::setDefaultTypesettingFeatures. Beware that the objC part has not been tested.

[Build Bot]

Comment on attachment 171682 [details]
Patch

Attachment 171682 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14561306

[WebKit Review Bot]

Comment on attachment 171682 [details]
Patch

Attachment 171682 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14656350

New failing tests:
inspector-protocol/debugger-pause-dedicated-worker.html

[Allan Sandfeld Jensen]

Created attachment 171809 [details]
Patch

[Build Bot]

Comment on attachment 171809 [details]
Patch

Attachment 171809 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14678497

[Allan Sandfeld Jensen]

Created attachment 171815 [details]
Patch

Update exported symbols for Mac

[Sam Weinig]

What is the usecase for making this configurable at runtime?  This seems very compile time type thing.

[Allan Sandfeld Jensen]

(In reply to comment #10)
> What is the usecase for making this configurable at runtime?  This seems very compile time type thing.

Printing, and an option for users with fast machines. We could not compile it in for Qt because it would be too slow for a lot of hardware, but we can offer it as a runtime setting.

(In reply to comment #11)
> (In reply to comment #10)
> > What is the usecase for making this configurable at runtime?  This seems very compile time type thing.
> 
> Printing

I don’t see how this setting allows a differentiation between printing and screen display. The same Settings are used by the page in both cases. A user or user-agent style sheet for print media that set the font-kerning and font-variant-ligatures inheritable properties on the root element might be a better way to achieve this.

> and an option for users with fast machines

This use case is satisfied by the current global and doesn’t require a per-page(group), dynamic setting.

[Allan Sandfeld Jensen]

(In reply to comment #12)
> > and an option for users with fast machines
> 
> This use case is satisfied by the current global and doesn’t require a per-page(group), dynamic setting.

Perhaps, but we have no settings system for user-agent wide settings, we only have the (page) settings. Since there is no system for them, each static settings has their own special API and special quirks and forces us to making special cases all the way up through the all the layers, if want to export them.

Take the example of WebKit2, by making this a setting, exporting it to higher layers comes almost free and it requires no special cases, where having it as a static variable means we need extra parameters to set it at process creation, extra IPC methods to control it at runtime, and extra wrapper methods in both WebProcess, WebProcessProxy and all of the ports APIs individually.

Even if we don't need it to be a runtime setting, making it one makes the code cleaner and simpler, by using the existing settings system and logic. As I said to begin with, this was originally part of the patch for bug #100106, where it made the patch as a whole simpler and cleaner.

(In reply to comment #13)
> (In reply to comment #12)
> > > and an option for users with fast machines
> > 
> > This use case is satisfied by the current global and doesn’t require a per-page(group), dynamic setting.
> 
> Perhaps, but we have no settings system for user-agent wide settings, we only have the (page) settings. Since there is no system for them, each static settings has their own special API and special quirks and forces us to making special cases all the way up through the all the layers, if want to export them.
> 
> Take the example of WebKit2, by making this a setting, exporting it to higher layers comes almost free and it requires no special cases, where having it as a static variable means we need extra parameters to set it at process creation, extra IPC methods to control it at runtime,

There doesn’t appear to be a use case for controlling this at runtime.

> Even if we don't need it to be a runtime setting, making it one makes the code cleaner and simpler, by using the existing settings system and logic. As I said to begin with, this was originally part of the patch for bug #100106, where it made the patch as a whole simpler and cleaner.

The behavior needs to be set globally so that it clients of the text system that aren’t tied to a Page (both inside WebCore and in WebKit) will respect it. Making this a setting breaks those clients. Perhaps your patch looks simpler and cleaner to you because it doesn’t address this issue, or because it doesn’t expose the setting via the preferences API.

[Anders Carlsson]

Comment on attachment 171815 [details]
Patch

I don't think Sam meant to r+ this.

[Anders Carlsson]

Comment on attachment 171815 [details]
Patch

Sam said in person that he meant to r- this patch.

[Allan Sandfeld Jensen]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > > and an option for users with fast machines
> > > 
> > > This use case is satisfied by the current global and doesn’t require a per-page(group), dynamic setting.
> > 
> > Perhaps, but we have no settings system for user-agent wide settings, we only have the (page) settings. Since there is no system for them, each static settings has their own special API and special quirks and forces us to making special cases all the way up through the all the layers, if want to export them.
> > 
> > Take the example of WebKit2, by making this a setting, exporting it to higher layers comes almost free and it requires no special cases, where having it as a static variable means we need extra parameters to set it at process creation, extra IPC methods to control it at runtime,
> 
> There doesn’t appear to be a use case for controlling this at runtime.
> 
Then how else whould you change the setting, then? Ask the user to reboot?

> > Even if we don't need it to be a runtime setting, making it one makes the code cleaner and simpler, by using the existing settings system and logic. As I said to begin with, this was originally part of the patch for bug #100106, where it made the patch as a whole simpler and cleaner.
> 
> The behavior needs to be set globally so that it clients of the text system that aren’t tied to a Page (both inside WebCore and in WebKit) will respect it. Making this a setting breaks those clients. Perhaps your patch looks simpler and cleaner to you because it doesn’t address this issue, or because it doesn’t expose the setting via the preferences API.

That clients of the text system do you have that isn't tied to a Page? And my patch does expose the setting via the preferences API, that is the entire point of the patch.

[Allan Sandfeld Jensen]

(In reply to comment #17)
> (In reply to comment #14)
> > (In reply to comment #13)
> > There doesn’t appear to be a use case for controlling this at runtime.
> > 
> Then how else whould you change the setting, then? Ask the user to reboot?
> 
Allow me to elaborate on the use-case.

A user might want to enable kerning and live with the performance impact. He enables the feature and immediately sees both the benefit and the performance impact, and can choose whether he wants to keep it or disable. He might also later encounter a longer page with more text and experience the performance impact getting worse and disable it at that point.

So while it is not an option that will be changed a lot, providing it at runtime is essential to the user experience.

[Sam Weinig]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > There doesn’t appear to be a use case for controlling this at runtime.
> > > 
> > Then how else whould you change the setting, then? Ask the user to reboot?
> > 
> Allow me to elaborate on the use-case.
> 
> A user might want to enable kerning and live with the performance impact. He enables the feature and immediately sees both the benefit and the performance impact, and can choose whether he wants to keep it or disable. He might also later encounter a longer page with more text and experience the performance impact getting worse and disable it at that point.
> 
> So while it is not an option that will be changed a lot, providing it at runtime is essential to the user experience.

I don't think that is compelling enough a reason to complicate the code here.

(In reply to comment #17)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > > and an option for users with fast machines
> > > > 
> > > > This use case is satisfied by the current global and doesn’t require a per-page(group), dynamic setting.
> > > 
> > > Perhaps, but we have no settings system for user-agent wide settings, we only have the (page) settings. Since there is no system for them, each static settings has their own special API and special quirks and forces us to making special cases all the way up through the all the layers, if want to export them.
> > > 
> > > Take the example of WebKit2, by making this a setting, exporting it to higher layers comes almost free and it requires no special cases, where having it as a static variable means we need extra parameters to set it at process creation, extra IPC methods to control it at runtime,
> > 
> > There doesn’t appear to be a use case for controlling this at runtime.
> > 
> Then how else whould you change the setting, then? Ask the user to reboot?

Starting a new WebProcess instance would probably suffice, if we wanted to support changing this at runtime.

> 
> > > Even if we don't need it to be a runtime setting, making it one makes the code cleaner and simpler, by using the existing settings system and logic. As I said to begin with, this was originally part of the patch for bug #100106, where it made the patch as a whole simpler and cleaner.
> > 
> > The behavior needs to be set globally so that it clients of the text system that aren’t tied to a Page (both inside WebCore and in WebKit) will respect it. Making this a setting breaks those clients. Perhaps your patch looks simpler and cleaner to you because it doesn’t address this issue, or because it doesn’t expose the setting via the preferences API.
> 
> That clients of the text system do you have that isn't tied to a Page?

There are two classes. Within WebCore, there is Theme code that is not Page-aware and there is some Canvas code that operates without a Page. Outside WebCore there is WebKit text API that is not tied to a Page.

> And my patch does expose the setting via the preferences API, that is the entire point of the patch.

The preferences API is the WebKit1 WebPreferences class and the WebKit2 WKPreferences object.

[Allan Sandfeld Jensen]

(In reply to comment #20)
> The preferences API is the WebKit1 WebPreferences class and the WebKit2 WKPreferences object.

Yes? Take a look at the patch for bug #100106

[Allan Sandfeld Jensen]

(In reply to comment #20)
> (In reply to comment #17)
> > What clients of the text system do you have that isn't tied to a Page?
> 
> There are two classes. Within WebCore, there is Theme code that is not Page-aware and there is some Canvas code that operates without a Page. Outside WebCore there is WebKit text API that is not tied to a Page.
> 
Interesting, I was not aware of that. That would indeed invalidate my patch. I will take a look at it.(In reply to comment #21)
> (In reply to comment #20)
> > The preferences API is the WebKit1 WebPreferences class and the WebKit2 WKPreferences object.
> 
> Yes? Take a look at the patch for bug #100106

Just in case you missed it. The preferences are in both WebKit and WebKit2 mapped to page settings. Therefore a global setting can not be exported as a preference. We have no design for global settings, just page settings aka preferences.