Bug 154554 – Font features specified in @font-face blocks don't apply to local() families

• Component: Text
• Status: RESOLVED FIXED

[Myles C. Maxfield]

FontCache probably needs to take @font-face feature arguments.

[Myles C. Maxfield]

Created attachment 271950 [details]
Reproduction

[Myles C. Maxfield]

Created attachment 271961 [details]
Patch

[Myles C. Maxfield]

Created attachment 271965 [details]
Patch

[Dean Jackson]

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.

[Myles C. Maxfield]

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).

[Myles C. Maxfield]

Created attachment 271970 [details]
Patch for committing

[WebKit Commit Bot]

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

[Myles C. Maxfield]

Committed r196969: <http://trac.webkit.org/changeset/196969>