Summary: | [mac] WebKit clients cannot change the behavior of text-rendering: auto | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||
Component: | Text | Assignee: | mitz | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Mac | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
mitz
2012-10-06 09:02:16 PDT
Created attachment 167458 [details]
Make the WebKitKerningAndLigaturesEnabledByDefault user defaults key control kerning and ligatures for text-rendering: auto
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>. |