[Cocoa] Clean up m_font inside FontPlatformData
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Jun 2015 16:50:38 +0000 (16:50 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Jun 2015 16:50:38 +0000 (16:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145634

Patch by Myles C. Maxfield <mmaxfield@apple.com> on 2015-06-04
Reviewed by Andreas Kling.

FontPlatformDatas are used as keys in a HashMap. This means that they need
to be able to represent a "deleted" value. Previously, this "deleted" value
was represented as setting the pointer value of m_font to -1, and guarding
all uses of m_font to make sure it wasn't -1 before dereferencing it.

This patch simplifies FontPlatformData to represent a "deleted" value using
a separate boolean member variable. This class is already big enough that
the increased space is negligable (the class already contains two CoreText
fonts in addition to a CoreGraphics font). Because of this simplification,
m_font can now be a RetainPtr, instead of being manually retained and
released.

There is still a long way to go before FontPlatformData is acceptably
clean and understandable. This patch improves one aspect of it, and more
improvements will eventually follow.

No new tests because there is no behavior change.

* platform/graphics/FontCache.cpp: Remove unused variable.
* platform/graphics/FontPlatformData.cpp:
(WebCore::FontPlatformData::FontPlatformData): Clean up all the PLATFORM
macros in favor of a single bool. Also, update to include new state.
(WebCore::FontPlatformData::operator=): Update to include new state.
* platform/graphics/FontPlatformData.h:
(WebCore::FontPlatformData::font): Update to account for RetainPtr.
(WebCore::FontPlatformData::nsFont): Ditto.
(WebCore::FontPlatformData::setNSFont): Ditto.
(WebCore::FontPlatformData::hash): Update to include new state.
(WebCore::FontPlatformData::operator==): Ditto.
(WebCore::FontPlatformData::isHashTableDeletedValue): Use new state.
(WebCore::FontPlatformData::hashTableDeletedFontValue): Deleted.
(WebCore::FontPlatformData::isValidCTFontRef): Deleted.
* platform/graphics/cocoa/FontPlatformDataCocoa.mm:
(WebCore::FontPlatformData::platformDataInit): No need for manual retain
and release.
(WebCore::FontPlatformData::platformDataAssign): Ditto.
(WebCore::FontPlatformData::platformIsEqual): Update to account for
RetanPtr.
(WebCore::FontPlatformData::setFont): No need for manual retain and
release.
(WebCore::FontPlatformData::FontPlatformData): Deleted.
(WebCore::FontPlatformData::~FontPlatformData): Deleted.
* platform/graphics/win/FontPlatformDataCairoWin.cpp:
(WebCore::FontPlatformData::~FontPlatformData): m_scaledFont is always
valid.
(WebCore::FontPlatformData::platformDataAssign): Ditto.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/FontCache.cpp
Source/WebCore/platform/graphics/FontPlatformData.cpp
Source/WebCore/platform/graphics/FontPlatformData.h
Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm
Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp

index 4809ad4..f9cc082 100644 (file)
@@ -1,3 +1,57 @@
+2015-06-04  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        [Cocoa] Clean up m_font inside FontPlatformData
+        https://bugs.webkit.org/show_bug.cgi?id=145634
+
+        Reviewed by Andreas Kling.
+
+        FontPlatformDatas are used as keys in a HashMap. This means that they need
+        to be able to represent a "deleted" value. Previously, this "deleted" value
+        was represented as setting the pointer value of m_font to -1, and guarding
+        all uses of m_font to make sure it wasn't -1 before dereferencing it.
+
+        This patch simplifies FontPlatformData to represent a "deleted" value using
+        a separate boolean member variable. This class is already big enough that
+        the increased space is negligable (the class already contains two CoreText
+        fonts in addition to a CoreGraphics font). Because of this simplification,
+        m_font can now be a RetainPtr, instead of being manually retained and
+        released.
+
+        There is still a long way to go before FontPlatformData is acceptably
+        clean and understandable. This patch improves one aspect of it, and more
+        improvements will eventually follow.
+
+        No new tests because there is no behavior change.
+
+        * platform/graphics/FontCache.cpp: Remove unused variable.
+        * platform/graphics/FontPlatformData.cpp:
+        (WebCore::FontPlatformData::FontPlatformData): Clean up all the PLATFORM
+        macros in favor of a single bool. Also, update to include new state.
+        (WebCore::FontPlatformData::operator=): Update to include new state.
+        * platform/graphics/FontPlatformData.h:
+        (WebCore::FontPlatformData::font): Update to account for RetainPtr.
+        (WebCore::FontPlatformData::nsFont): Ditto.
+        (WebCore::FontPlatformData::setNSFont): Ditto.
+        (WebCore::FontPlatformData::hash): Update to include new state.
+        (WebCore::FontPlatformData::operator==): Ditto.
+        (WebCore::FontPlatformData::isHashTableDeletedValue): Use new state.
+        (WebCore::FontPlatformData::hashTableDeletedFontValue): Deleted.
+        (WebCore::FontPlatformData::isValidCTFontRef): Deleted.
+        * platform/graphics/cocoa/FontPlatformDataCocoa.mm:
+        (WebCore::FontPlatformData::platformDataInit): No need for manual retain
+        and release.
+        (WebCore::FontPlatformData::platformDataAssign): Ditto.
+        (WebCore::FontPlatformData::platformIsEqual): Update to account for
+        RetanPtr.
+        (WebCore::FontPlatformData::setFont): No need for manual retain and
+        release.
+        (WebCore::FontPlatformData::FontPlatformData): Deleted.
+        (WebCore::FontPlatformData::~FontPlatformData): Deleted.
+        * platform/graphics/win/FontPlatformDataCairoWin.cpp:
+        (WebCore::FontPlatformData::~FontPlatformData): m_scaledFont is always
+        valid.
+        (WebCore::FontPlatformData::platformDataAssign): Ditto.
+
 2015-06-03  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [GTK] [Wayland] Build is broken on trunk
index 31b9cf6..a6c4049 100644 (file)
@@ -342,7 +342,6 @@ struct FontDataCacheKeyHash {
 
 struct FontDataCacheKeyTraits : WTF::GenericHashTraits<FontPlatformData> {
     static const bool emptyValueIsZero = true;
-    static const bool needsDestruction = true;
     static const FontPlatformData& emptyValue()
     {
         static NeverDestroyed<FontPlatformData> key(0.f, false, false);
index f90c02e..31664f4 100644 (file)
 namespace WebCore {
 
 FontPlatformData::FontPlatformData(WTF::HashTableDeletedValueType)
-#if PLATFORM(WIN)
-    : m_font(WTF::HashTableDeletedValue)
-#elif PLATFORM(COCOA)
-    : m_font(hashTableDeletedFontValue())
-#endif
+    : m_isHashTableDeletedValue(true)
 {
-#if USE(CAIRO)
-    m_scaledFont = hashTableDeletedFontValue();
-#endif
 }
 
 FontPlatformData::FontPlatformData()
@@ -70,6 +63,7 @@ FontPlatformData::FontPlatformData(CGFontRef cgFont, float size, bool syntheticB
 FontPlatformData::FontPlatformData(const FontPlatformData& source)
     : FontPlatformData(source.m_size, source.m_syntheticBold, source.m_syntheticOblique, source.m_orientation, source.m_widthVariant)
 {
+    m_isHashTableDeletedValue = source.m_isHashTableDeletedValue;
     m_isColorBitmapFont = source.m_isColorBitmapFont;
     m_isCompositeFontReference = source.m_isCompositeFontReference;
     platformDataInit(source);
@@ -81,6 +75,7 @@ const FontPlatformData& FontPlatformData::operator=(const FontPlatformData& othe
     if (this == &other)
         return *this;
 
+    m_isHashTableDeletedValue = other.m_isHashTableDeletedValue;
     m_syntheticBold = other.m_syntheticBold;
     m_syntheticOblique = other.m_syntheticOblique;
     m_orientation = other.m_orientation;
index a7142c5..9dbb4da 100644 (file)
@@ -107,7 +107,7 @@ public:
     HFONT hfont() const { return m_font ? m_font->get() : 0; }
     bool useGDI() const { return m_useGDI; }
 #elif PLATFORM(COCOA)
-    CTFontRef font() const { return m_font; }
+    CTFontRef font() const { return m_font.get(); }
     void setFont(CTFontRef);
 
     CTFontRef ctFont() const;
@@ -118,8 +118,8 @@ public:
 
 #if USE(APPKIT)
     // FIXME: Remove this when all NSFont usage is removed.
-    NSFont *nsFont() const { return (NSFont *)m_font; }
-    void setNSFont(NSFont *font) { setFont((CTFontRef)font); }
+    NSFont *nsFont() const { return reinterpret_cast<NSFont *>(const_cast<__CTFont*>(m_font.get())); }
+    void setNSFont(NSFont *font) { setFont(reinterpret_cast<CTFontRef>(font)); }
 #endif
 #endif
 
@@ -151,11 +151,11 @@ public:
 #elif OS(DARWIN)
 #if USE(APPKIT)
         ASSERT(m_font || !m_cgFont);
-        uintptr_t hashCodes[3] = { (uintptr_t)m_font, m_widthVariant, static_cast<uintptr_t>(m_orientation << 2 | m_syntheticBold << 1 | m_syntheticOblique) };
+        uintptr_t hashCodes[3] = { reinterpret_cast<uintptr_t>(const_cast<__CTFont*>(m_font.get())), m_widthVariant, static_cast<uintptr_t>(m_isHashTableDeletedValue << 3 | m_orientation << 2 | m_syntheticBold << 1 | m_syntheticOblique) };
 #else
         ASSERT(m_font || !m_cgFont || m_isEmoji);
-        uintptr_t hashCodes[3] = { static_cast<uintptr_t>(CFHash(m_font)), m_widthVariant, static_cast<uintptr_t>(m_isEmoji << 3 | m_orientation << 2 | m_syntheticBold << 1 | m_syntheticOblique) };
-#endif // !PLATFORM(IOS)
+        uintptr_t hashCodes[3] = { static_cast<uintptr_t>(CFHash(m_font.get())), m_widthVariant, static_cast<uintptr_t>(m_isHashTableDeletedValue << 4 | m_isEmoji << 3 | m_orientation << 2 | m_syntheticBold << 1 | m_syntheticOblique) };
+#endif
         return StringHasher::hashMemory<sizeof(hashCodes)>(hashCodes);
 #elif USE(CAIRO)
         return PtrHash<cairo_scaled_font_t*>::hash(m_scaledFont);
@@ -167,6 +167,7 @@ public:
     bool operator==(const FontPlatformData& other) const
     {
         return platformIsEqual(other)
+            && m_isHashTableDeletedValue == other.m_isHashTableDeletedValue
             && m_size == other.m_size
             && m_syntheticBold == other.m_syntheticBold
             && m_syntheticOblique == other.m_syntheticOblique
@@ -178,13 +179,7 @@ public:
 
     bool isHashTableDeletedValue() const
     {
-#if PLATFORM(WIN) && !USE(CAIRO)
-        return m_font.isHashTableDeletedValue();
-#elif PLATFORM(COCOA)
-        return m_font == hashTableDeletedFontValue();
-#elif USE(CAIRO)
-        return m_scaledFont == hashTableDeletedFontValue();
-#endif
+        return m_isHashTableDeletedValue;
     }
 
 #if PLATFORM(COCOA) || PLATFORM(WIN)
@@ -200,16 +195,11 @@ private:
     void platformDataInit(const FontPlatformData&);
     const FontPlatformData& platformDataAssign(const FontPlatformData&);
 #if PLATFORM(COCOA)
-    static CTFontRef hashTableDeletedFontValue() { return reinterpret_cast<CTFontRef>(-1); }
-    static bool isValidCTFontRef(CTFontRef font) { return font && font != hashTableDeletedFontValue(); }
     CGFloat ctFontSize() const;
 #endif
 #if PLATFORM(WIN)
     void platformDataInit(HFONT, float size, HDC, WCHAR* faceName);
 #endif
-#if USE(CAIRO)
-    static cairo_scaled_font_t* hashTableDeletedFontValue() { return reinterpret_cast<cairo_scaled_font_t*>(-1); }
-#endif
 
 public:
     bool m_syntheticBold { false };
@@ -224,7 +214,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 };
+    RetainPtr<CTFontRef> m_font;
     mutable RetainPtr<CTFontRef> m_ctFont;
 #elif PLATFORM(WIN)
     RefPtr<SharedGDIObject<HFONT>> m_font;
@@ -239,6 +229,7 @@ private:
 
     bool m_isColorBitmapFont { false };
     bool m_isCompositeFontReference { false };
+    bool m_isHashTableDeletedValue { false };
 
 #if PLATFORM(WIN)
     bool m_useGDI { false };
index c39063e..5682f44 100644 (file)
@@ -46,20 +46,17 @@ FontPlatformData::FontPlatformData(CTFontRef font, float size, bool syntheticBol
 {
     ASSERT_ARG(font, font);
     m_font = font;
-    CFRetain(m_font);
     m_isColorBitmapFont = CTFontGetSymbolicTraits(font) & kCTFontTraitColorGlyphs;
     m_isCompositeFontReference = CTFontGetSymbolicTraits(font) & kCTFontCompositeTrait;
 }
 
 FontPlatformData::~FontPlatformData()
 {
-    if (isValidCTFontRef(m_font))
-        CFRelease(m_font);
 }
 
 void FontPlatformData::platformDataInit(const FontPlatformData& f)
 {
-    m_font = isValidCTFontRef(f.m_font) ? static_cast<CTFontRef>(const_cast<void *>(CFRetain(f.m_font))) : f.m_font;
+    m_font = f.m_font;
 
 #if PLATFORM(IOS)
     m_isEmoji = f.m_isEmoji;
@@ -74,12 +71,8 @@ const FontPlatformData& FontPlatformData::platformDataAssign(const FontPlatformD
 #if PLATFORM(IOS)
     m_isEmoji = f.m_isEmoji;
 #endif
-    if (isValidCTFontRef(m_font) && isValidCTFontRef(f.m_font) && CFEqual(m_font, f.m_font))
+    if (m_font && f.m_font && CFEqual(m_font.get(), f.m_font.get()))
         return *this;
-    if (isValidCTFontRef(f.m_font))
-        CFRetain(f.m_font);
-    if (isValidCTFontRef(m_font))
-        CFRelease(m_font);
     m_font = f.m_font;
     m_ctFont = f.m_ctFont;
 
@@ -91,14 +84,14 @@ bool FontPlatformData::platformIsEqual(const FontPlatformData& other) const
     bool result = false;
     if (m_font || other.m_font) {
 #if PLATFORM(IOS)
-        result = isValidCTFontRef(m_font) && isValidCTFontRef(other.m_font) && CFEqual(m_font, other.m_font);
+        result = m_font && other.m_font && CFEqual(m_font.get(), other.m_font.get());
 #if !ASSERT_DISABLED
         if (result)
             ASSERT(m_isEmoji == other.m_isEmoji);
 #endif
 #else
         result = m_font == other.m_font;
-#endif // PLATFORM(IOS)
+#endif
         return result;
     }
 #if PLATFORM(IOS) && !ASSERT_DISABLED
@@ -111,19 +104,15 @@ bool FontPlatformData::platformIsEqual(const FontPlatformData& other) const
 void FontPlatformData::setFont(CTFontRef font)
 {
     ASSERT_ARG(font, font);
-    ASSERT(m_font != reinterpret_cast<CTFontRef>(-1));
 
     if (m_font == font)
         return;
 
-    CFRetain(font);
-    if (m_font)
-        CFRelease(m_font);
     m_font = font;
     m_size = CTFontGetSize(font);
     m_cgFont = adoptCF(CTFontCopyGraphicsFont(font, nullptr));
 
-    CTFontSymbolicTraits traits = CTFontGetSymbolicTraits(m_font);
+    CTFontSymbolicTraits traits = CTFontGetSymbolicTraits(m_font.get());
     m_isColorBitmapFont = traits & kCTFontTraitColorGlyphs;
     m_isCompositeFontReference = traits & kCTFontCompositeTrait;
     
index 339c14d..aaa7aa6 100644 (file)
@@ -91,7 +91,7 @@ FontPlatformData::FontPlatformData(GDIObject<HFONT> font, cairo_font_face_t* fon
 
 FontPlatformData::~FontPlatformData()
 {
-    if (m_scaledFont && m_scaledFont != hashTableDeletedFontValue())
+    if (m_scaledFont)
         cairo_scaled_font_destroy(m_scaledFont);
 }
 
@@ -110,7 +110,7 @@ const FontPlatformData& FontPlatformData::platformDataAssign(const FontPlatformD
     m_font = other.m_font;
     m_useGDI = other.m_useGDI;
 
-    if (m_scaledFont && m_scaledFont != hashTableDeletedFontValue())
+    if (m_scaledFont)
         cairo_scaled_font_destroy(m_scaledFont);
 
     m_scaledFont = cairo_scaled_font_reference(other.m_scaledFont);