Bug 98601 – [mac] WebKit clients cannot change the behavior of text-rendering: auto

• Component: Text
• Status: RESOLVED FIXED

[mac] Kerning and ligatures are disabled by default

<rdar://problem/12446507>

Created attachment 167458 [details]
Make the WebKitKerningAndLigaturesEnabledByDefault user defaults key control kerning and ligatures for text-rendering: auto

[Darin Adler]

Comment on attachment 167458 [details]
Make the WebKitKerningAndLigaturesEnabledByDefault user defaults key control kerning and ligatures for text-rendering: auto

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.

(In reply to comment #3)
> (From update of attachment 167458 [details])
> 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;
>     }

I’ll change to use a switch statement.

> 
> > Source/WebCore/platform/graphics/Font.h:239
> > +    static TypesettingFeatures s_defaultTypesettingFeatures;
> 
> Can this data member be private?

Yes, I’ll change it.

> > 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.

Yes, I think it’s worth the clarity. It’s similar to the WebKitDefaultTextEncodingName default and a few other “default defaults” in WebKit.

Thanks for the review!

Fixed in <http://trac.webkit.org/r130585>.