[webkit-changes] [185200] trunk/Source/WebCore

Thu, 04 Jun 2015 09:51:29 -0700

Title: [185200] trunk/Source/WebCore
Revision
185200
Author
mmaxfi...@apple.com
Date
2015-06-04 09:50:38 -0700 (Thu, 04 Jun 2015)

Log Message

[Cocoa] Clean up m_font inside FontPlatformData
https://bugs.webkit.org/show_bug.cgi?id=145634

Patch by Myles C. Maxfield <mmaxfi...@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.

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (185199 => 185200)


--- trunk/Source/WebCore/ChangeLog	2015-06-04 14:25:50 UTC (rev 185199)
+++ trunk/Source/WebCore/ChangeLog	2015-06-04 16:50:38 UTC (rev 185200)
@@ -1,3 +1,57 @@
+2015-06-04  Myles C. Maxfield  <mmaxfi...@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  <clo...@igalia.com>
 
         [GTK] [Wayland] Build is broken on trunk

Modified: trunk/Source/WebCore/platform/graphics/FontCache.cpp (185199 => 185200)


--- trunk/Source/WebCore/platform/graphics/FontCache.cpp	2015-06-04 14:25:50 UTC (rev 185199)
+++ trunk/Source/WebCore/platform/graphics/FontCache.cpp	2015-06-04 16:50:38 UTC (rev 185200)
@@ -342,7 +342,6 @@
 
 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);

Modified: trunk/Source/WebCore/platform/graphics/FontPlatformData.cpp (185199 => 185200)


--- trunk/Source/WebCore/platform/graphics/FontPlatformData.cpp	2015-06-04 14:25:50 UTC (rev 185199)
+++ trunk/Source/WebCore/platform/graphics/FontPlatformData.cpp	2015-06-04 16:50:38 UTC (rev 185200)
@@ -35,15 +35,8 @@
 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(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 @@
     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;

Modified: trunk/Source/WebCore/platform/graphics/FontPlatformData.h (185199 => 185200)


--- trunk/Source/WebCore/platform/graphics/FontPlatformData.h	2015-06-04 14:25:50 UTC (rev 185199)
+++ trunk/Source/WebCore/platform/graphics/FontPlatformData.h	2015-06-04 16:50:38 UTC (rev 185200)
@@ -107,7 +107,7 @@
     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 @@
 
 #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 @@
 #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 @@
     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 @@
 
     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 @@
     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 @@
 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 @@
 
     bool m_isColorBitmapFont { false };
     bool m_isCompositeFontReference { false };
+    bool m_isHashTableDeletedValue { false };
 
 #if PLATFORM(WIN)
     bool m_useGDI { false };

Modified: trunk/Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm (185199 => 185200)


--- trunk/Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm	2015-06-04 14:25:50 UTC (rev 185199)
+++ trunk/Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm	2015-06-04 16:50:38 UTC (rev 185200)
@@ -46,20 +46,17 @@
 {
     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 @@
 #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 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 @@
 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;

Modified: trunk/Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp (185199 => 185200)


--- trunk/Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp	2015-06-04 14:25:50 UTC (rev 185199)
+++ trunk/Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp	2015-06-04 16:50:38 UTC (rev 185200)
@@ -91,7 +91,7 @@
 
 FontPlatformData::~FontPlatformData()
 {
-    if (m_scaledFont && m_scaledFont != hashTableDeletedFontValue())
+    if (m_scaledFont)
         cairo_scaled_font_destroy(m_scaledFont);
 }
 
@@ -110,7 +110,7 @@
     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);

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to