Attachment #258407 for bug #147751

View Details Raw Unified Return to Bug 147751

| Differences between



2015-08-06  Myles C. Maxfield  <mmaxfield@apple.com>

        Make FontDescriptionKey sensitive to FontFeatureSettings
        https://bugs.webkit.org/show_bug.cgi?id=147751

        Reviewed by NOBODY (OOPS!).

        Just like how FontDescription hashes should be sensitive to locale, they should
        also be sensitive to font features.

        This patch also fixes operator== for FontDescriptionKey, which was previously
        comparing hashes for equality instead of the underlying data. Comparing hashes
        for equality is useless inside hashmaps.

        No new tests because there is no behavior change.

        * platform/graphics/FontCache.cpp:
        (WebCore::FontPlatformDataCacheKey::FontPlatformDataCacheKey):
        (WebCore::FontPlatformDataCacheKey::isHashTableDeletedValue):
        (WebCore::FontPlatformDataCacheKey::hashTableDeletedSize): Deleted.
        * platform/graphics/FontCache.h:
        (WebCore::FontDescriptionKey::FontDescriptionKey):
        (WebCore::FontDescriptionKey::operator==):
        (WebCore::FontDescriptionKey::operator!=):
        (WebCore::FontDescriptionKey::isHashTableDeletedValue):
        (WebCore::FontDescriptionKey::computeHash):
        * platform/graphics/FontFeatureSettings.cpp:
        (WebCore::FontFeature::hash):
        (WebCore::FontFeatureSettings::hash):
        * platform/graphics/FontFeatureSettings.h:

2015-08-06  Myles C. Maxfield  <mmaxfield@apple.com>

        Add comment to CSSParserString
        https://bugs.webkit.org/show_bug.cgi?id=147724


Lines 105-112 public: a/Source/WebCore/platform/graphics/FontCache.cpp_sec1
- a/Source/WebCore/platform/graphics/FontCache.cpp -5 / +5 lines
105	, m_family(family)	105	, m_family(family)
106	{ }	106	{ }
107		107
108	FontPlatformDataCacheKey(HashTableDeletedValueType) : m_fontDescriptionKey(hashTableDeletedSize()) { }	108	explicit FontPlatformDataCacheKey(HashTableDeletedValueType t)
109	bool isHashTableDeletedValue() const { return m_fontDescriptionKey.size == hashTableDeletedSize(); }	109	: m_fontDescriptionKey(t)
		110	{ }
		111
		112	bool isHashTableDeletedValue() const { return m_fontDescriptionKey.isHashTableDeletedValue(); }
110		113
111	bool operator==(const FontPlatformDataCacheKey& other) const	114	bool operator==(const FontPlatformDataCacheKey& other) const
112	{	115	{
Lines 115-123 public: a/Source/WebCore/platform/graphics/FontCache.cpp_sec2
115		118
116	FontDescriptionKey m_fontDescriptionKey;	119	FontDescriptionKey m_fontDescriptionKey;
117	AtomicString m_family;	120	AtomicString m_family;
118
119	private:
120	static unsigned hashTableDeletedSize() { return 0xFFFFFFFFU; }
121	};	121	};
122		122
123	struct FontPlatformDataCacheKeyHash {	123	struct FontPlatformDataCacheKeyHash {



#include <wtf/PassRefPtr.h>
#include <wtf/RefPtr.h>
#include <wtf/Vector.h>
#include <wtf/text/AtomicStringHash.h>
#include <wtf/text/WTFString.h>

#if PLATFORM(IOS)


// This key contains the FontDescription fields other than family that matter when fetching FontDatas (platform fonts).
struct FontDescriptionKey {
    FontDescriptionKey()
        : m_size(0)
        , m_weight(0)
        , m_flags(0)
        , m_featureSettings(0)
    { }

    FontDescriptionKey(const FontDescription& description)
        : m_size(description.computedPixelSize())
        , m_weight(description.weight())
        , m_flags(makeFlagKey(description))
        , m_locale(description.locale())
        , m_featureSettings(description.featureSettings())
    { }

    explicit FontDescriptionKey(WTF::HashTableDeletedValueType)
        : m_size(cHashTableDeletedSize)
    { }

    bool operator==(const FontDescriptionKey& other) const
    {
        return m_size == other.m_size && m_weight == other.m_weight && m_flags == other.m_flags && m_locale == other.m_locale
            && ((m_featureSettings == other.m_featureSettings) || (m_featureSettings && other.m_featureSettings && m_featureSettings.get() == other.m_featureSettings.get()));
    }

    bool operator!=(const FontDescriptionKey& other) const
    {
        return !(*this == other);
    }

    bool isHashTableDeletedValue() const { return m_size == cHashTableDeletedSize; }

    inline unsigned computeHash() const
    {
        unsigned result = WTF::intHash(m_size);
        result = WTF::pairIntHash(result, WTF::intHash(m_weight));
        result = WTF::pairIntHash(result, WTF::intHash(m_flags));
        result = WTF::pairIntHash(result, WTF::intHash(m_flags));
        result = WTF::PairHash<AtomicString, unsigned>::hash(std::make_pair(m_locale, result));
        result = WTF::pairIntHash(result, m_featureSettings ? m_featureSettings->hash() : 0);
        return result;
    }

private:
    static unsigned makeFlagKey(const FontDescription& description)
    {
        static_assert(USCRIPT_CODE_LIMIT < 0x1000, "Script code must fit in an unsigned along with the other flags");

            | static_cast<unsigned>(description.italic()) << 1
            | static_cast<unsigned>(description.renderingMode());
    }

    static const unsigned cHashTableDeletedSize = 0xFFFFFFFFU;

    unsigned m_size;
    unsigned m_weight;
    unsigned m_flags;
    AtomicString m_locale;
    RefPtr<FontFeatureSettings> m_featureSettings;
    inline unsigned computeHash() const
    {
        return StringHasher::hashMemory<sizeof(FontDescriptionKey)>(this);
    }
    unsigned size;
    unsigned weight;
    unsigned flags;
    unsigned localeHash; // FIXME: Here, and every client of us, makes hashes of hashes.
};

class FontCache {

Lines 26-31 a/Source/WebCore/platform/graphics/FontFeatureSettings.cpp_sec1
- a/Source/WebCore/platform/graphics/FontFeatureSettings.cpp +15 lines
26	#include "config.h"	26	#include "config.h"
27	#include "FontFeatureSettings.h"	27	#include "FontFeatureSettings.h"
28		28
		29	#include <wtf/text/AtomicStringHash.h>
		30
29	namespace WebCore {	31	namespace WebCore {
30		32
31	FontFeature::FontFeature(const AtomicString& tag, int value)	33	FontFeature::FontFeature(const AtomicString& tag, int value)
Lines 44-49 bool FontFeature::operator<(const FontFeature& other) const a/Source/WebCore/platform/graphics/FontFeatureSettings.cpp_sec2
44	return (m_tag.impl() < other.m_tag.impl()) \|\| (m_tag.impl() == other.m_tag.impl() && m_value < other.m_value);	46	return (m_tag.impl() < other.m_tag.impl()) \|\| (m_tag.impl() == other.m_tag.impl() && m_value < other.m_value);
45	}	47	}
46		48
		49	unsigned FontFeature::hash() const
		50	{
		51	return WTF::PairHash<AtomicString, unsigned>::hash(std::make_pair(m_tag, m_value));
		52	}
		53
47	Ref<FontFeatureSettings> FontFeatureSettings::create()	54	Ref<FontFeatureSettings> FontFeatureSettings::create()
48	{	55	{
49	return adoptRef(*new FontFeatureSettings);	56	return adoptRef(*new FontFeatureSettings);
Lines 60-63 void FontFeatureSettings::insert(FontFeature&& feature) a/Source/WebCore/platform/graphics/FontFeatureSettings.cpp_sec3
60	m_list.insert(i, WTF::move(feature));	67	m_list.insert(i, WTF::move(feature));
61	}	68	}
62		69
		70	unsigned FontFeatureSettings::hash() const
		71	{
		72	unsigned result = 0x5E787623U; // Chosen randomly
		73	for (size_t i = 0; i < size(); ++i)
		74	result = WTF::pairIntHash(result, at(i).hash());
		75	return result;
		76	}
		77
63	}	78	}

Lines 44-49 public: a/Source/WebCore/platform/graphics/FontFeatureSettings.h_sec1
- a/Source/WebCore/platform/graphics/FontFeatureSettings.h +4 lines
44	const AtomicString& tag() const { return m_tag; }	44	const AtomicString& tag() const { return m_tag; }
45	int value() const { return m_value; }	45	int value() const { return m_value; }
46		46
		47	unsigned hash() const;
		48
47	private:	49	private:
48	AtomicString m_tag;	50	AtomicString m_tag;
49	const int m_value { 0 };	51	const int m_value { 0 };
Lines 59-64 public: a/Source/WebCore/platform/graphics/FontFeatureSettings.h_sec2
59	const FontFeature& operator[](int index) const { return m_list[index]; }	61	const FontFeature& operator[](int index) const { return m_list[index]; }
60	const FontFeature& at(size_t index) const { return m_list.at(index); }	62	const FontFeature& at(size_t index) const { return m_list.at(index); }
61		63
		64	unsigned hash() const;
		65
62	private:	66	private:
63	FontFeatureSettings() { }	67	FontFeatureSettings() { }
64	Vector<FontFeature> m_list;	68	Vector<FontFeature> m_list;

Return to Bug 147751

Lines 1-5 a/Source/WebCore/ChangeLog_sec1
- a/Source/WebCore/ChangeLog +31 lines
		1	2015-08-06 Myles C. Maxfield <mmaxfield@apple.com>
		2
		3	Make FontDescriptionKey sensitive to FontFeatureSettings
		4	https://bugs.webkit.org/show_bug.cgi?id=147751
		5
		6	Reviewed by NOBODY (OOPS!).
		7
		8	Just like how FontDescription hashes should be sensitive to locale, they should
		9	also be sensitive to font features.
		10
		11	This patch also fixes operator== for FontDescriptionKey, which was previously
		12	comparing hashes for equality instead of the underlying data. Comparing hashes
		13	for equality is useless inside hashmaps.
		14
		15	No new tests because there is no behavior change.
		16
		17	* platform/graphics/FontCache.cpp:
		18	(WebCore::FontPlatformDataCacheKey::FontPlatformDataCacheKey):
		19	(WebCore::FontPlatformDataCacheKey::isHashTableDeletedValue):
		20	(WebCore::FontPlatformDataCacheKey::hashTableDeletedSize): Deleted.
		21	* platform/graphics/FontCache.h:
		22	(WebCore::FontDescriptionKey::FontDescriptionKey):
		23	(WebCore::FontDescriptionKey::operator==):
		24	(WebCore::FontDescriptionKey::operator!=):
		25	(WebCore::FontDescriptionKey::isHashTableDeletedValue):
		26	(WebCore::FontDescriptionKey::computeHash):
		27	* platform/graphics/FontFeatureSettings.cpp:
		28	(WebCore::FontFeature::hash):
		29	(WebCore::FontFeatureSettings::hash):
		30	* platform/graphics/FontFeatureSettings.h:
		31
1	2015-08-06 Myles C. Maxfield <mmaxfield@apple.com>	32	2015-08-06 Myles C. Maxfield <mmaxfield@apple.com>
2		33
3	Add comment to CSSParserString	34	Add comment to CSSParserString
4	https://bugs.webkit.org/show_bug.cgi?id=147724	35	https://bugs.webkit.org/show_bug.cgi?id=147724
5		36

Lines 37-42 a/Source/WebCore/platform/graphics/FontCache.h_sec1
- a/Source/WebCore/platform/graphics/FontCache.h -25 / +50 lines
37	#include <wtf/PassRefPtr.h>	37	#include <wtf/PassRefPtr.h>
38	#include <wtf/RefPtr.h>	38	#include <wtf/RefPtr.h>
39	#include <wtf/Vector.h>	39	#include <wtf/Vector.h>
		40	#include <wtf/text/AtomicStringHash.h>
40	#include <wtf/text/WTFString.h>	41	#include <wtf/text/WTFString.h>
41		42
42	#if PLATFORM(IOS)	43	#if PLATFORM(IOS)
Lines 67-84 typedef IMLangFontLink IMLangFontLinkType; a/Source/WebCore/platform/graphics/FontCache.h_sec2
67		68
68	// This key contains the FontDescription fields other than family that matter when fetching FontDatas (platform fonts).	69	// This key contains the FontDescription fields other than family that matter when fetching FontDatas (platform fonts).
69	struct FontDescriptionKey {	70	struct FontDescriptionKey {
70	explicit FontDescriptionKey(unsigned size = 0)	71	FontDescriptionKey()
71	: size(size)	72	: m_size(0)
72	, weight(0)	73	, m_weight(0)
73	, flags(0)	74	, m_flags(0)
74	, localeHash(0)	75	, m_featureSettings(0)
75	{ }	76	{ }
		77
76	FontDescriptionKey(const FontDescription& description)	78	FontDescriptionKey(const FontDescription& description)
77	: size(description.computedPixelSize())	79	: m_size(description.computedPixelSize())
78	, weight(description.weight())	80	, m_weight(description.weight())
79	, flags(makeFlagKey(description))	81	, m_flags(makeFlagKey(description))
80	, localeHash(description.locale().isNull() ? 0 : description.locale().impl()->existingHash())	82	, m_locale(description.locale())
		83	, m_featureSettings(description.featureSettings())
		84	{ }
		85
		86	explicit FontDescriptionKey(WTF::HashTableDeletedValueType)
		87	: m_size(cHashTableDeletedSize)
81	{ }	88	{ }
		89
		90	bool operator==(const FontDescriptionKey& other) const
		91	{
		92	return m_size == other.m_size && m_weight == other.m_weight && m_flags == other.m_flags && m_locale == other.m_locale
		93	&& ((m_featureSettings == other.m_featureSettings) \|\| (m_featureSettings && other.m_featureSettings && m_featureSettings.get() == other.m_featureSettings.get()));
		94	}
		95
		96	bool operator!=(const FontDescriptionKey& other) const
		97	{
		98	return !(*this == other);
		99	}
		100
		101	bool isHashTableDeletedValue() const { return m_size == cHashTableDeletedSize; }
		102
		103	inline unsigned computeHash() const
		104	{
		105	unsigned result = WTF::intHash(m_size);
		106	result = WTF::pairIntHash(result, WTF::intHash(m_weight));
		107	result = WTF::pairIntHash(result, WTF::intHash(m_flags));
		108	result = WTF::pairIntHash(result, WTF::intHash(m_flags));
		109	result = WTF::PairHash<AtomicString, unsigned>::hash(std::make_pair(m_locale, result));
		110	result = WTF::pairIntHash(result, m_featureSettings ? m_featureSettings->hash() : 0);
		111	return result;
		112	}
		113
		114	private:
82	static unsigned makeFlagKey(const FontDescription& description)	115	static unsigned makeFlagKey(const FontDescription& description)
83	{	116	{
84	static_assert(USCRIPT_CODE_LIMIT < 0x1000, "Script code must fit in an unsigned along with the other flags");	117	static_assert(USCRIPT_CODE_LIMIT < 0x1000, "Script code must fit in an unsigned along with the other flags");
Lines 91-112 struct FontDescriptionKey { a/Source/WebCore/platform/graphics/FontCache.h_sec3
91	\| static_cast<unsigned>(description.italic()) << 1	124	\| static_cast<unsigned>(description.italic()) << 1
92	\| static_cast<unsigned>(description.renderingMode());	125	\| static_cast<unsigned>(description.renderingMode());
93	}	126	}
94	bool operator==(const FontDescriptionKey& other) const	127
95	{	128	static const unsigned cHashTableDeletedSize = 0xFFFFFFFFU;
96	return size == other.size && weight == other.weight && flags == other.flags && localeHash == other.localeHash;	129
97	}	130	unsigned m_size;
98	bool operator!=(const FontDescriptionKey& other) const	131	unsigned m_weight;
99	{	132	unsigned m_flags;
100	return !(*this == other);	133	AtomicString m_locale;
101	}	134	RefPtr<FontFeatureSettings> m_featureSettings;
102	inline unsigned computeHash() const
103	{
104	return StringHasher::hashMemory<sizeof(FontDescriptionKey)>(this);
105	}
106	unsigned size;
107	unsigned weight;
108	unsigned flags;
109	unsigned localeHash; // FIXME: Here, and every client of us, makes hashes of hashes.
110	};	135	};
111		136
112	class FontCache {	137	class FontCache {