Bug 98601 - [mac] WebKit clients cannot change the behavior of text-rendering: auto
Summary: [mac] WebKit clients cannot change the behavior of text-rendering: auto
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-06 09:02 PDT by mitz
Modified: 2012-10-06 11:41 PDT (History)
1 user (show)

See Also:

Attachments
Make the WebKitKerningAndLigaturesEnabledByDefault user defaults key control kerning and ligatures for text-rendering: auto (11.02 KB, patch)
2012-10-06 09:16 PDT, mitz 		darin: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>.