Bug 149775 – FontCascade::typesettingFeatures() is not privy to font-variant-* nor font-feature-settings

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

It needs to be updated to include this additional information.

[Radar WebKit Bug Importer]

<rdar://problem/22959756>

[Myles C. Maxfield]

It is computed from text-rendering, font-kerning, and font-variant-ligatures.

It is used in a few places:
1. In the simple text code path, we don't run shaping if K&L is disabled
2. We use a more sophisticated space width measuring algorithm if kerning is enabled, in both RenderBlockLineLayout and BreakingContext
3. In the complex text code path, we create the CTLine/CTTypesetter with options to reflect K&L
4. If the font has a typesetting feature other than kerning/ligatures (of which there are none), we fall to the complex text codepath

[Myles C. Maxfield]

WidthIterator::supportsTypesettingFeatures() can just be deleted.

[Myles C. Maxfield]

Related: https://bugs.webkit.org/show_bug.cgi?id=150051

This related bug removes the 4th use (above) of FontCascade::typesettingFeatures().

[Myles C. Maxfield]

The design for this should be to add a few more bits to TypesettingFeatures.

s_defaultTypesettingFeatures will have to be updated.

[Myles C. Maxfield]

(In reply to comment #5)
> The design for this should be to add a few more bits to TypesettingFeatures.
> 
> s_defaultTypesettingFeatures will have to be updated.

This doesn't seem right. Adding bits to TypesettingFeatures for every value of font-variant-* would make the type redundant with FontVariantSettings.

[Myles C. Maxfield]

(In reply to comment #2)
> It is computed from text-rendering, font-kerning, and font-variant-ligatures.
> 
> It is used in a few places:
> 1. In the simple text code path, we don't run shaping if K&L is disabled
> 2. We use a more sophisticated space width measuring algorithm if kerning is
> enabled, in both RenderBlockLineLayout and BreakingContext
> 3. In the complex text code path, we create the CTLine/CTTypesetter with
> options to reflect K&L
> 4. If the font has a typesetting feature other than kerning/ligatures (of
> which there are none), we fall to the complex text codepath

5. FontCascade uses it to determine if we should use the simple vs complex code path for various operations

[Myles C. Maxfield]

s_defaultTypesettingFeatures gets set by the WebKitKerningAndLigaturesEnabledByDefault default which defaults to YES.

[Myles C. Maxfield]

Created attachment 263391 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 263391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263391&action=review

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:333
> +    // Step 4: Font-variant

font-kerning should be applied here.

[Myles C. Maxfield]

Comment on attachment 263391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263391&action=review

> Source/WebCore/platform/graphics/Font.cpp:382
>      UNUSED_PARAM(glyphs);

This else block needs to be updated.

[Myles C. Maxfield]

Created attachment 263424 [details]
Patch

[Darin Adler]

Comment on attachment 263424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263424&action=review

> Source/WebCore/platform/graphics/FontCascade.cpp:596
> +    // Font::applyTransforms() doesn't know which features to enable/disable.

Comment should be more specific. I believe you mean something like “Because Font::applyTransforms() doesn't know which features to enable/disable it the simple code path can’t properly handle feature or variant settings.” But maybe there’s a shorter and even clearer way to say that.

> Source/WebCore/platform/graphics/FontCascade.h:324
> +    // This is only used for the simple text codepath.

An interesting statement, but not sure exactly why it’s relevant. Typically, comments that state how a function is used become obsolete as callers use them in new ways. If this stated *why* this is suitable only for the simple text code path, then it would probably be more useful.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:329
> +    // Step 1: CoreText handles our defaults

What does “our defaults” mean? Specifically, what is “our”?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:331
> +    // Steps 2-3: FIXME: Consult with @font-face

Would be clearer if it said:

    // Steps 2-3: Consult with @font-face
    // FIXME: This is not yet implemented.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:343
> +        // We may want to turn off more stuff here.

Oblique comment. More useful would be a comment explaining where the list above came from; particularly, what is the criteria for being in this list?

[Myles C. Maxfield]

Created attachment 263547 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 263547 [details]
Patch for committing

Clearing flags on attachment: 263547

Committed r191331: <http://trac.webkit.org/changeset/191331>