Bug 98601

Summary: [mac] WebKit clients cannot change the behavior of text-rendering: auto
Product: WebKit Reporter: mitz
Component: TextAssignee: mitz
Status: RESOLVED FIXED    
Severity: Normal CC: darin
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
Make the WebKitKerningAndLigaturesEnabledByDefault user defaults key control kerning and ligatures for text-rendering: auto darin: review+

Description mitz 2012-10-06 09:02:16 PDT
[mac] Kerning and ligatures are disabled by default
Comment 1 mitz 2012-10-06 09:05:32 PDT
<rdar://problem/12446507>
Comment 2 mitz 2012-10-06 09:16:46 PDT
Created attachment 167458 [details]
Make the WebKitKerningAndLigaturesEnabledByDefault user defaults key control kerning and ligatures for text-rendering: auto
Comment 3 Darin Adler 2012-10-06 09:58:37 PDT
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.
Comment 4 mitz 2012-10-06 10:03:23 PDT
(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!
Comment 5 mitz 2012-10-06 11:41:24 PDT
Fixed in <http://trac.webkit.org/r130585>.