[webkit-reviews] review granted: [Bug 98601] [mac] WebKit clients cannot change the behavior of text-rendering: auto : [Attachment 167458] Make the WebKitKerningAndLigaturesEnabledByDefault user defaults key control kerning and ligatures for text-rendering: auto

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org <mitz at webkit.org>'s
request for review:
Bug 98601: [mac] WebKit clients cannot change the behavior of text-rendering:
auto
https://bugs.webkit.org/show_bug.cgi?id=98601

Attachment 167458: Make the WebKitKerningAndLigaturesEnabledByDefault user
defaults key control kerning and ligatures for text-rendering: auto
https://bugs.webkit.org/attachment.cgi?id=167458&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=167458&action=review


> Source/WebCore/platform/graphics/Font.h:127
> -	   TypesettingFeatures features = textRenderingMode ==
OptimizeLegibility || textRenderingMode == GeometricPrecision ? Kerning |
Ligatures : 0;
> +	   TypesettingFeatures features = textRenderingMode ==
OptimizeLegibility || textRenderingMode == GeometricPrecision ? Kerning |
Ligatures : s_defaultTypesettingFeatures;

I think an or would be slightly better.

    TypesettingFeatures features = s_defaultTypesettingFeatures |
(textRenderingMode == OptimizeLegibility || textRenderingMode ==
GeometricPrecision ? Kerning | Ligatures);

The idea being that the text rendering mode turns on features rather than
entirely overriding the default. I also think we should use a switch statement
for the text rendering mode the way we do for the states in the font
description below to make the code read more clearly.

    TypesettingFeatures features = s_defaultTypesettingFeatures;

    switch (textRenderingMode) {
    case GeometricPrecision:
    case OptimizeLegibility:
	features |= Kerning | Ligatures;
	break;
    /* list other cases here */
	break;
    }

> Source/WebCore/platform/graphics/Font.h:239
> +    static TypesettingFeatures s_defaultTypesettingFeatures;

Can this data member be private?

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:102
> +    parameters.shouldEnableKerningAndLigaturesByDefault = [[NSUserDefaults
standardUserDefaults] boolForKey:@"WebKitKerningAndLigaturesEnabledByDefault"];


It’s a little strange to have the phrase “by default” in the name of “a
default”. I think you could just omit it, although I suppose it makes it a
little more clear that if this is false you can still get kerning and
ligatures.