Font lookups are flakey due to caching in fontWithFamily(). Radar: <rdar://problem/21012406>
Created attachment 253395 [details] Patch
Comment on attachment 253395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253395&action=review > Source/WebCore/ChangeLog:16 > + This patch now uses a std::pair as key containing both the "desired > + family" and the "desired traits" for correctness. I also updated the > + cache to use WTF types instead of NS types. So now looking up a name in the cache requires converting an AtomicString to an NSString every time? That doesn’t seem like an improvement! > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:159 > + familyMapping.add(std::make_pair(desiredFamily, desiredTraits), value); Old code did the equivalent of HashMap::set, not HashMap::add. Is this a desirable change?
Comment on attachment 253395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253395&action=review >> Source/WebCore/ChangeLog:16 >> + cache to use WTF types instead of NS types. > > So now looking up a name in the cache requires converting an AtomicString to an NSString every time? That doesn’t seem like an improvement! Converting an AtomicString into a NSString is relatively cheap. It ends up calling StringImpl::createCFString() which constructs a CFStringRef (later casted to a NSString*) *without* copying the characters. The cost of converting an AtomicString into a NSString* is thus really small compared to calling [fontManager availableFontFamilies] + [fontManager availableFonts] and doing linear searches on those 2 lists. There is a slight overhead when saving a mapping into the cache though because we need to construct an AtomicString for the NSString* of the availableFamily. However, we already get the desiredFamily as an AtomicString so we no longer need to convert it to an NSString* when the cache is successful (I will re-upload an iteration with this optimization). Therefore, it is a win there and also HashMap usually performs better than NSDictionary. >> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:159 >> + familyMapping.add(std::make_pair(desiredFamily, desiredTraits), value); > > Old code did the equivalent of HashMap::set, not HashMap::add. Is this a desirable change? We only call this function if the key is not already present so add() is slightly faster here.
Created attachment 253400 [details] Patch
Created attachment 253401 [details] Patch
Comment on attachment 253401 [details] Patch Clearing flags on attachment: 253401 Committed r184599: <http://trac.webkit.org/changeset/184599>
All reviewed patches have been landed. Closing bug.