FontCache probably needs to take @font-face feature arguments.
Created attachment 271950 [details] Reproduction
Created attachment 271961 [details] Patch
Created attachment 271965 [details] Patch
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 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).
Created attachment 271970 [details] Patch for committing
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
Committed r196969: <http://trac.webkit.org/changeset/196969>