[Cocoa] System fonts do not get correct tracking
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Apr 2015 01:08:15 +0000 (01:08 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Apr 2015 01:08:15 +0000 (01:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=143395

Reviewed by Ryosuke Niwa.

Inside FontPlatformData, we have two CTFonts. If the user has specified
-webkit-system-font, we will pass in a CTFont, and the FontPlatformData
will wrap it. However, we will then roundtrip through CGFont in order
to create a second CTFont. We were basing our tracking and system
font knowledge off of this round-tripped font instead of the original font.

Note that this design is terrible and needs to be overhauled.
FontPlatformData should only have a single platform font inside it.

This patch also caches whether or not a font is a system font.

No new tests because it is impossible to test the tracking of the
system font in a robust way.

* platform/graphics/Font.cpp:
(WebCore::Font::Font): Rearrange member variables.
* platform/graphics/Font.h: Move member variables around for better
packing, and cache whether or not the font is a system font.
* platform/graphics/FontData.h: Add comment
* platform/graphics/cocoa/FontCocoa.mm:
(WebCore::Font::platformInit): Cache whether or not the font is a system
font.
(WebCore::hasCustomTracking): Use cached value.
(WebCore::canUseFastGlyphAdvanceGetter):
(WebCore::Font::platformWidthForGlyph):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@182512 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/Font.cpp
Source/WebCore/platform/graphics/Font.h
Source/WebCore/platform/graphics/FontPlatformData.h
Source/WebCore/platform/graphics/cocoa/FontCocoa.mm

index 4ca6f45..ee349b6 100644 (file)
@@ -1,3 +1,36 @@
+2015-04-07  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        [Cocoa] System fonts do not get correct tracking
+        https://bugs.webkit.org/show_bug.cgi?id=143395
+
+        Reviewed by Ryosuke Niwa.
+
+        Inside FontPlatformData, we have two CTFonts. If the user has specified
+        -webkit-system-font, we will pass in a CTFont, and the FontPlatformData
+        will wrap it. However, we will then roundtrip through CGFont in order
+        to create a second CTFont. We were basing our tracking and system
+        font knowledge off of this round-tripped font instead of the original font.
+
+        Note that this design is terrible and needs to be overhauled.
+        FontPlatformData should only have a single platform font inside it.
+
+        This patch also caches whether or not a font is a system font.
+
+        No new tests because it is impossible to test the tracking of the
+        system font in a robust way.
+
+        * platform/graphics/Font.cpp:
+        (WebCore::Font::Font): Rearrange member variables.
+        * platform/graphics/Font.h: Move member variables around for better
+        packing, and cache whether or not the font is a system font.
+        * platform/graphics/FontData.h: Add comment
+        * platform/graphics/cocoa/FontCocoa.mm:
+        (WebCore::Font::platformInit): Cache whether or not the font is a system
+        font.
+        (WebCore::hasCustomTracking): Use cached value.
+        (WebCore::canUseFastGlyphAdvanceGetter):
+        (WebCore::Font::platformWidthForGlyph):
+
 2015-04-07  Alex Christensen  <achristensen@webkit.org>
 
         Block popups from content extensions.
index 151ed34..4a477fd 100644 (file)
@@ -50,20 +50,30 @@ unsigned GlyphPage::s_count = 0;
 const float smallCapsFontSizeMultiplier = 0.7f;
 const float emphasisMarkFontSizeMultiplier = 0.5f;
 
-Font::Font(const FontPlatformData& platformData, bool isCustomFont, bool isLoading, bool isTextOrientationFallback)
+Font::Font(const FontPlatformData& platformData, std::unique_ptr<SVGData>&& svgData, bool isCustomFont, bool isLoading, bool isTextOrientationFallback)
     : m_maxCharWidth(-1)
     , m_avgCharWidth(-1)
     , m_platformData(platformData)
+    , m_svgData(WTF::move(svgData))
+    , m_mathData(nullptr)
     , m_treatAsFixedPitch(false)
     , m_isCustomFont(isCustomFont)
     , m_isLoading(isLoading)
     , m_isTextOrientationFallback(isTextOrientationFallback)
     , m_isBrokenIdeographFallback(false)
-    , m_mathData(nullptr)
-#if ENABLE(OPENTYPE_VERTICAL)
-    , m_verticalData(0)
-#endif
     , m_hasVerticalGlyphs(false)
+    , m_isUsedInSystemFallbackCache(false)
+#if PLATFORM(COCOA) || PLATFORM(WIN)
+    , m_isSystemFont(false)
+#endif
+#if PLATFORM(IOS)
+    , m_shouldNotBeUsedForArabic(false)
+#endif
+{
+}
+
+Font::Font(const FontPlatformData& platformData, bool isCustomFont, bool isLoading, bool isTextOrientationFallback)
+    : Font(platformData, std::unique_ptr<SVGData>(), isCustomFont, isLoading, isTextOrientationFallback)
 {
     platformInit();
     platformGlyphInit();
@@ -77,21 +87,7 @@ Font::Font(const FontPlatformData& platformData, bool isCustomFont, bool isLoadi
 }
 
 Font::Font(std::unique_ptr<SVGData> svgData, float fontSize, bool syntheticBold, bool syntheticItalic)
-    : m_platformData(FontPlatformData(fontSize, syntheticBold, syntheticItalic))
-    , m_svgData(WTF::move(svgData))
-    , m_treatAsFixedPitch(false)
-    , m_isCustomFont(true)
-    , m_isLoading(false)
-    , m_isTextOrientationFallback(false)
-    , m_isBrokenIdeographFallback(false)
-    , m_mathData(nullptr)
-#if ENABLE(OPENTYPE_VERTICAL)
-    , m_verticalData(0)
-#endif
-    , m_hasVerticalGlyphs(false)
-#if PLATFORM(IOS)
-    , m_shouldNotBeUsedForArabic(false)
-#endif
+    : Font(FontPlatformData(fontSize, syntheticBold, syntheticItalic), WTF::move(svgData), true, false, false)
 {
     m_svgData->initializeFont(this, fontSize);
 }
index dca5b51..f4f31b4 100644 (file)
@@ -203,8 +203,11 @@ public:
 
     bool applyTransforms(GlyphBufferGlyph*, GlyphBufferAdvance*, size_t glyphCount, TypesettingFeatures) const;
 
-#if PLATFORM(WIN)
+#if PLATFORM(COCOA) || PLATFORM(WIN)
     bool isSystemFont() const { return m_isSystemFont; }
+#endif
+
+#if PLATFORM(WIN)
     SCRIPT_FONTPROPERTIES* scriptFontProperties() const;
     SCRIPT_CACHE* scriptCache() const { return &m_scriptCache; }
     static void setShouldApplyMacAscentHack(bool);
@@ -217,6 +220,8 @@ private:
 
     Font(std::unique_ptr<SVGData>, float fontSize, bool syntheticBold, bool syntheticItalic);
 
+    Font(const FontPlatformData&, std::unique_ptr<SVGData>&&, bool isCustomFont = false, bool isLoading = false, bool isTextOrientationFallback = false);
+
     void platformInit();
     void platformGlyphInit();
     void platformCharWidthInit();
@@ -248,20 +253,10 @@ private:
     mutable std::unique_ptr<GlyphMetricsMap<FloatRect>> m_glyphToBoundsMap;
     mutable GlyphMetricsMap<float> m_glyphToWidthMap;
 
-    bool m_treatAsFixedPitch;
-    bool m_isCustomFont; // Whether or not we are custom font loaded via @font-face
-    bool m_isLoading; // Whether or not this custom font is still in the act of loading.
-
-    bool m_isTextOrientationFallback;
-    bool m_isBrokenIdeographFallback;
-
-    bool m_isUsedInSystemFallbackCache { false };
-
     mutable RefPtr<OpenTypeMathData> m_mathData;
 #if ENABLE(OPENTYPE_VERTICAL)
     RefPtr<OpenTypeVerticalData> m_verticalData;
 #endif
-    bool m_hasVerticalGlyphs;
 
     Glyph m_spaceGlyph;
     float m_spaceWidth;
@@ -304,12 +299,25 @@ private:
 #endif
 
 #if PLATFORM(WIN)
-    bool m_isSystemFont;
     mutable SCRIPT_CACHE m_scriptCache;
     mutable SCRIPT_FONTPROPERTIES* m_scriptFontProperties;
 #endif
+
+    unsigned m_treatAsFixedPitch : 1;
+    unsigned m_isCustomFont : 1; // Whether or not we are custom font loaded via @font-face
+    unsigned m_isLoading : 1; // Whether or not this custom font is still in the act of loading.
+
+    unsigned m_isTextOrientationFallback : 1;
+    unsigned m_isBrokenIdeographFallback : 1;
+    unsigned m_hasVerticalGlyphs : 1;
+
+    unsigned m_isUsedInSystemFallbackCache : 1;
+
+#if PLATFORM(COCOA) || PLATFORM(WIN)
+    unsigned m_isSystemFont : 1;
+#endif
 #if PLATFORM(IOS)
-    bool m_shouldNotBeUsedForArabic;
+    unsigned m_shouldNotBeUsedForArabic : 1;
 #endif
 };
 
index a800d05..fd0cd8e 100644 (file)
@@ -226,6 +226,7 @@ public:
 
 private:
 #if PLATFORM(COCOA)
+    // FIXME: Get rid of one of these. These two fonts are subtly different, and it is not obvious which one to use where.
     CTFontRef m_font { nullptr };
     mutable RetainPtr<CTFontRef> m_ctFont;
 #elif PLATFORM(WIN)
index 4e0126f..2c266d0 100644 (file)
@@ -267,15 +267,16 @@ void Font::platformInit()
     if (platformData().orientation() == Vertical && !isTextOrientationFallback())
         m_hasVerticalGlyphs = fontHasVerticalGlyphs(m_platformData.ctFont());
 
-    if (!m_platformData.m_isEmoji)
-        return;
-
-    int thirdOfSize = m_platformData.size() / 3;
-    m_fontMetrics.setAscent(thirdOfSize);
-    m_fontMetrics.setDescent(thirdOfSize);
-    m_fontMetrics.setLineGap(thirdOfSize);
-    m_fontMetrics.setLineSpacing(0);
+    if (m_platformData.m_isEmoji) {
+        int thirdOfSize = m_platformData.size() / 3;
+        m_fontMetrics.setAscent(thirdOfSize);
+        m_fontMetrics.setDescent(thirdOfSize);
+        m_fontMetrics.setLineGap(thirdOfSize);
+        m_fontMetrics.setLineSpacing(0);
+    }
 #endif
+
+    m_isSystemFont = CTFontDescriptorIsSystemUIFont(adoptCF(CTFontCopyFontDescriptor(m_platformData.font())).get());
 }
 
 void Font::platformCharWidthInit()
@@ -454,9 +455,9 @@ static inline bool advanceForColorBitmapFont(const FontPlatformData& platformDat
 #endif
 }
 
-static bool hasCustomTracking(CTFontRef font)
+static inline bool hasCustomTracking(const Font& font)
 {
-    return CTFontDescriptorIsSystemUIFont(adoptCF(CTFontCopyFontDescriptor(font)).get());
+    return font.isSystemFont();
 }
 
 static inline bool isEmoji(const FontPlatformData& platformData)
@@ -469,10 +470,11 @@ static inline bool isEmoji(const FontPlatformData& platformData)
 #endif
 }
 
-static inline bool canUseFastGlyphAdvanceGetter(const FontPlatformData& platformData, Glyph glyph, CGSize& advance, bool& populatedAdvance)
+static inline bool canUseFastGlyphAdvanceGetter(const Font& font, Glyph glyph, CGSize& advance, bool& populatedAdvance)
 {
+    const FontPlatformData& platformData = font.platformData();
     // Fast getter doesn't take custom tracking into account
-    if (hasCustomTracking(platformData.ctFont()))
+    if (hasCustomTracking(font))
         return false;
     // Fast getter doesn't work for emoji
     if (isEmoji(platformData))
@@ -490,7 +492,7 @@ float Font::platformWidthForGlyph(Glyph glyph) const
     CGSize advance = CGSizeZero;
     bool horizontal = platformData().orientation() == Horizontal;
     bool populatedAdvance = false;
-    if ((horizontal || m_isBrokenIdeographFallback) && canUseFastGlyphAdvanceGetter(platformData(), glyph, advance, populatedAdvance)) {
+    if ((horizontal || m_isBrokenIdeographFallback) && canUseFastGlyphAdvanceGetter(*this, glyph, advance, populatedAdvance)) {
         float pointSize = platformData().m_size;
         CGAffineTransform m = CGAffineTransformMakeScale(pointSize, pointSize);
         if (!CGFontGetGlyphAdvancesForStyle(platformData().cgFont(), &m, renderingStyle(platformData()), &glyph, 1, &advance)) {
@@ -498,8 +500,15 @@ float Font::platformWidthForGlyph(Glyph glyph) const
             LOG_ERROR("Unable to cache glyph widths for %@ %f", fullName.get(), pointSize);
             advance.width = 0;
         }
-    } else if (!populatedAdvance)
-        CTFontGetAdvancesForGlyphs(m_platformData.ctFont(), horizontal ? kCTFontOrientationHorizontal : kCTFontOrientationVertical, &glyph, &advance, 1);
+    } else if (!populatedAdvance) {
+        // m_platformData.font() returns the original font that was passed into the FontPlatformData constructor.\10 In the case of fonts that have custom tracking,
+        // the custom tracking does not survive the transformation to either m_platformData.cgFont() nor m_platformData.ctFont(), so we must use the original
+        // font() that was passed in. However, for web fonts, m_platformData.font() is null, so we must use m_platformData.ctFont() for those cases.
+        if (hasCustomTracking(*this))
+            CTFontGetAdvancesForGlyphs(m_platformData.font(), horizontal ? kCTFontOrientationHorizontal : kCTFontOrientationVertical, &glyph, &advance, 1);
+        else
+            CTFontGetAdvancesForGlyphs(m_platformData.ctFont(), horizontal ? kCTFontOrientationHorizontal : kCTFontOrientationVertical, &glyph, &advance, 1);
+    }
 
     return advance.width + m_syntheticBoldOffset;
 }