WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
154554
Font features specified in @font-face blocks don't apply to local() families
https://bugs.webkit.org/show_bug.cgi?id=154554
Summary
Font features specified in @font-face blocks don't apply to local() families
Myles C. Maxfield
Reported
2016-02-22 13:27:01 PST
FontCache probably needs to take @font-face feature arguments.
Attachments
Reproduction
(1.19 KB, text/html)
2016-02-22 13:28 PST
,
Myles C. Maxfield
no flags
Details
Patch
(22.12 KB, patch)
2016-02-22 15:55 PST
,
Myles C. Maxfield
no flags
Details
Formatted Diff
Diff
Patch
(24.55 KB, patch)
2016-02-22 16:21 PST
,
Myles C. Maxfield
dino
: review+
Details
Formatted Diff
Diff
Patch for committing
(25.32 KB, patch)
2016-02-22 17:21 PST
,
Myles C. Maxfield
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2016-02-22 13:28:01 PST
Created
attachment 271950
[details]
Reproduction
Myles C. Maxfield
Comment 2
2016-02-22 15:55:29 PST
Created
attachment 271961
[details]
Patch
Myles C. Maxfield
Comment 3
2016-02-22 16:21:38 PST
Created
attachment 271965
[details]
Patch
Dean Jackson
Comment 4
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.
Myles C. Maxfield
Comment 5
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).
Myles C. Maxfield
Comment 6
2016-02-22 17:21:41 PST
Created
attachment 271970
[details]
Patch for committing
WebKit Commit Bot
Comment 7
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
Myles C. Maxfield
Comment 8
2016-02-22 23:17:25 PST
Committed
r196969
: <
http://trac.webkit.org/changeset/196969
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug