Bug 149775 - FontCascade::typesettingFeatures() is not privy to font-variant-* nor font-feature-settings
Summary: FontCascade::typesettingFeatures() is not privy to font-variant-* nor font-fe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 150074
Blocks: 149779
  Show dependency treegraph
 
Reported: 2015-10-02 16:56 PDT by Myles C. Maxfield
Modified: 2015-10-20 11:07 PDT (History)
3 users (show)

See Also:


Attachments
Patch (28.08 KB, patch)
2015-10-17 16:47 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (28.41 KB, patch)
2015-10-18 09:40 PDT, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff
Patch for committing (28.34 KB, patch)
2015-10-19 20:02 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2015-10-02 16:56:44 PDT
It needs to be updated to include this additional information.
Comment 1 Radar WebKit Bug Importer 2015-10-02 17:06:57 PDT
<rdar://problem/22959756>
Comment 2 Myles C. Maxfield 2015-10-12 14:07:02 PDT
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
Comment 3 Myles C. Maxfield 2015-10-12 14:07:46 PDT
WidthIterator::supportsTypesettingFeatures() can just be deleted.
Comment 4 Myles C. Maxfield 2015-10-12 15:43:19 PDT
Related: https://bugs.webkit.org/show_bug.cgi?id=150051

This related bug removes the 4th use (above) of FontCascade::typesettingFeatures().
Comment 5 Myles C. Maxfield 2015-10-12 16:14:45 PDT
The design for this should be to add a few more bits to TypesettingFeatures.

s_defaultTypesettingFeatures will have to be updated.
Comment 6 Myles C. Maxfield 2015-10-12 16:58:36 PDT
(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.
Comment 7 Myles C. Maxfield 2015-10-12 17:01:26 PDT
(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
Comment 8 Myles C. Maxfield 2015-10-12 17:08:00 PDT
s_defaultTypesettingFeatures gets set by the WebKitKerningAndLigaturesEnabledByDefault default which defaults to YES.
Comment 9 Myles C. Maxfield 2015-10-17 16:47:48 PDT
Created attachment 263391 [details]
Patch
Comment 10 Myles C. Maxfield 2015-10-17 17:03:42 PDT
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.
Comment 11 Myles C. Maxfield 2015-10-17 17:28:01 PDT
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.
Comment 12 Myles C. Maxfield 2015-10-18 09:40:27 PDT
Created attachment 263424 [details]
Patch
Comment 13 Darin Adler 2015-10-18 15:42:08 PDT
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?
Comment 14 Myles C. Maxfield 2015-10-19 20:02:38 PDT
Created attachment 263547 [details]
Patch for committing
Comment 15 WebKit Commit Bot 2015-10-19 21:56:07 PDT
Comment on attachment 263547 [details]
Patch for committing

Clearing flags on attachment: 263547

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