Bug 154554

Summary: Font features specified in @font-face blocks don't apply to local() families
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: TextAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, jonlee, mmaxfield, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149779    
Attachments:
Description Flags
Reproduction
none
Patch
none
Patch
dino: review+
Patch for committing commit-queue: commit-queue-

Description Myles C. Maxfield 2016-02-22 13:27:01 PST
FontCache probably needs to take @font-face feature arguments.
Comment 1 Myles C. Maxfield 2016-02-22 13:28:01 PST
Created attachment 271950 [details]
Reproduction
Comment 2 Myles C. Maxfield 2016-02-22 15:55:29 PST
Created attachment 271961 [details]
Patch
Comment 3 Myles C. Maxfield 2016-02-22 16:21:38 PST
Created attachment 271965 [details]
Patch
Comment 4 Dean Jackson 2016-02-22 17:15:53 PST
Comment on attachment 271965 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.cpp:118
> +        if (m_fontDescriptionKey != other.m_fontDescriptionKey || m_fontFaceFeatures != other.m_fontFaceFeatures || m_fontFaceVariantSettings != other.m_fontFaceVariantSettings)

We typically do all these on single lines when we have more than one.

> Source/WebCore/platform/graphics/FontCache.cpp:375
> +RefPtr<Font> FontCache::fontForFamily(const FontDescription& fontDescription, const AtomicString& family, const FontFeatureSettings* fontFaceFeatures, const FontVariantSettings* fontFaceVariantSettings, bool checkingAlternateName)

Through this whole patch you call the FontFeatureSettings parameter fontFaceFeatures, and the FontVariantSettings parameter fontFaceVariantSettings. You should use consistent naming, whether that is postfixing with Settings or not (and using Face or not).

> Source/WebCore/platform/graphics/FontCache.h:229
> +    std::unique_ptr<FontPlatformData> createFontPlatformData(const FontDescription&, const AtomicString& family, const FontFeatureSettings* fontFaceFeatures, const FontVariantSettings* fontFaceVariantSettings);

I guess technically you don't need the parameter names here.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:137
> +    return *fontForFamily(fontDescription, timesStr, nullptr, nullptr, false);

nullptr, nullptr, false are the default values. No need to include them here.

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:123
> +    return *fontForFamily(fontDescription, AtomicString(".PhoneFallback", AtomicString::ConstructFromLiteral), nullptr, nullptr, false);

Or here.

> Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:289
> +                    runFont = fontCache.fontForFamily(m_font.fontDescription(), fontName.get(), nullptr, nullptr, false).get();

Or here. Unless you want to :)

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:398
> +    if (RefPtr<Font> font = fontForFamily(fontDescription, AtomicString("Times", AtomicString::ConstructFromLiteral), nullptr, nullptr, false))

ditto.

> Source/WebCore/platform/text/TextFlags.h:237
> +    bool operator!=(const FontVariantSettings& other) const
> +    {
> +        return !(*this == other);
> +    }

Style nit. The other new instances of this are on a single line.
Comment 5 Myles C. Maxfield 2016-02-22 17:21:34 PST
Comment on attachment 271965 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontCache.cpp:375
>> +RefPtr<Font> FontCache::fontForFamily(const FontDescription& fontDescription, const AtomicString& family, const FontFeatureSettings* fontFaceFeatures, const FontVariantSettings* fontFaceVariantSettings, bool checkingAlternateName)
> 
> Through this whole patch you call the FontFeatureSettings parameter fontFaceFeatures, and the FontVariantSettings parameter fontFaceVariantSettings. You should use consistent naming, whether that is postfixing with Settings or not (and using Face or not).

Maybe I should rename fontFaceVariantSettings to fontFaceVariants, but I think that's out of scope for this patch (these names are use in many more places than just in this patch).
Comment 6 Myles C. Maxfield 2016-02-22 17:21:41 PST
Created attachment 271970 [details]
Patch for committing
Comment 7 WebKit Commit Bot 2016-02-22 23:08:02 PST
Comment on attachment 271970 [details]
Patch for committing

Rejecting attachment 271970 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 271970, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/870805
Comment 8 Myles C. Maxfield 2016-02-22 23:17:25 PST
Committed r196969: <http://trac.webkit.org/changeset/196969>