Make FontDescriptionKey sensitive to FontFeatureSettings
Created attachment 258407 [details] Patch
Created attachment 258408 [details] Patch
Comment on attachment 258408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258408&action=review > Source/WebCore/ChangeLog:14 > + I should probably also say that this patch is in preparation for implementing font-feature-settings.
Comment on attachment 258407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258407&action=review > Source/WebCore/platform/graphics/FontCache.h:75 > + , m_featureSettings(0) Is this needed? m_featureSettings will be default contracted to null. > Source/WebCore/platform/graphics/FontCache.h:132 > + unsigned m_size; > + unsigned m_weight; > + unsigned m_flags; Can we default these to 0 here? > Source/WebCore/platform/graphics/FontFeatureSettings.cpp:72 > + unsigned result = 0x5E787623U; // Chosen randomly Why do we need a random number?
Comment on attachment 258408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258408&action=review > Source/WebCore/platform/graphics/FontCache.h:110 > + 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); The way we usually do this to create an array of unsigned and pass it to StringHasher::hashMemory. See SecurityOriginHash.h for an example. We really need to add a hash_combine like thing (http://www.boost.org/doc/libs/1_55_0/doc/html/hash/combine.html).
Comment on attachment 258408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258408&action=review > Source/WebCore/platform/graphics/FontCache.h:72 > + : m_size(0) What about > Source/WebCore/platform/graphics/FontCache.h:131 > + unsigned m_weight; You could initialize these like so: unsigned m_size {0}; and remove the default constructor entirely.
Comment on attachment 258408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258408&action=review >> Source/WebCore/platform/graphics/FontCache.h:110 >> + result = WTF::pairIntHash(result, m_featureSettings ? m_featureSettings->hash() : 0); > > The way we usually do this to create an array of unsigned and pass it to StringHasher::hashMemory. See SecurityOriginHash.h for an example. We really need to add a hash_combine like thing (http://www.boost.org/doc/libs/1_55_0/doc/html/hash/combine.html). Ah, yes, I've seen that pattern in PlatformFontData. I'll use that.
Comment on attachment 258407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258407&action=review >> Source/WebCore/platform/graphics/FontCache.h:75 >> + , m_featureSettings(0) > > Is this needed? m_featureSettings will be default contracted to null. Nope! >> Source/WebCore/platform/graphics/FontCache.h:132 >> + unsigned m_flags; > > Can we default these to 0 here? Yep! >> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:72 >> + unsigned result = 0x5E787623U; // Chosen randomly > > Why do we need a random number? Because this is the input to a hash function, it seems like a better idea than just 0 for increasing entropy.
Comment on attachment 258408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258408&action=review > Source/WebCore/platform/graphics/FontCache.h:109 > + 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)); Wow, I completely messed this up
Created attachment 258409 [details] Patch
Created attachment 258410 [details] Patch
Created attachment 258412 [details] Patch
Committed r188088: <http://trac.webkit.org/changeset/188088>
Comment on attachment 258412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258412&action=review > Source/WebCore/platform/graphics/FontFeatureSettings.cpp:75 > + unsigned result = 0; > + for (size_t i = 0; i < size(); ++i) > + result = WTF::pairIntHash(result, at(i).hash()); > + return result; Is this really a good way to hash a vector? I think there’s a better approach where we accumulate with a single hash rather than hashing the old hash with a new hash value every time. Likely more efficient as well. There must be some good example elsewhere where we do this.
Comment on attachment 258412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258412&action=review >> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:75 >> + return result; > > Is this really a good way to hash a vector? I think there’s a better approach where we accumulate with a single hash rather than hashing the old hash with a new hash value every time. Likely more efficient as well. There must be some good example elsewhere where we do this. Hmm, I see you discussing this with Sam earlier, but I am not really sure about where you ended up here. I’d like to understand so I can do it right in the future.
(In reply to comment #15) > Comment on attachment 258412 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258412&action=review > > >> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:75 > >> + return result; > > > > Is this really a good way to hash a vector? I think there’s a better approach where we accumulate with a single hash rather than hashing the old hash with a new hash value every time. Likely more efficient as well. There must be some good example elsewhere where we do this. > > Hmm, I see you discussing this with Sam earlier, but I am not really sure > about where you ended up here. I’d like to understand so I can do it right > in the future. It looks like StringHasher has an interface where you can push data into it willy-nilly. I'll use that.
(In reply to comment #16) > (In reply to comment #15) > > Comment on attachment 258412 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=258412&action=review > > > > >> Source/WebCore/platform/graphics/FontFeatureSettings.cpp:75 > > >> + return result; > > > > > > Is this really a good way to hash a vector? I think there’s a better approach where we accumulate with a single hash rather than hashing the old hash with a new hash value every time. Likely more efficient as well. There must be some good example elsewhere where we do this. > > > > Hmm, I see you discussing this with Sam earlier, but I am not really sure > > about where you ended up here. I’d like to understand so I can do it right > > in the future. > > It looks like StringHasher has an interface where you can push data into it > willy-nilly. I'll use that. https://bugs.webkit.org/show_bug.cgi?id=147866