[webkit-reviews] review granted: [Bug 137539] [Mac] Spending too much time mapping desired font families to available ones : [Attachment 239513] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 137539: [Mac] Spending too much time mapping desired font families to
available ones
https://bugs.webkit.org/show_bug.cgi?id=137539

Attachment 239513: Patch
https://bugs.webkit.org/attachment.cgi?id=239513&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239513&action=review


OK to land as is. A few ideas for improvements.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:57
> +    [WebFontCache invalidate];

I think it’s a little indirect that WebFontCache is only properly invalidated
if WebCore::FontCache is allocated. I suppose that in practice this is not a
problem, but it’s sort of an indirect dependency that could possibly be made
slightly more direct. I could imagine adding a call to fontCache() to the
desiredFamilyToAvailableFamilyDictionary() function, for example, with a
comment explaining the reason and the side effect.

> Source/WebCore/platform/mac/WebFontCache.mm:132
> +    if ([desiredFamilyToAvailableFamilyDictionary() count] == maxCacheSize)
{

Might be faster to put the dictionary into a local variable rather than calling
the function at least two times.

> Source/WebCore/platform/mac/WebFontCache.mm:134
> +	   NSEnumerator *keyEnumerator =
[desiredFamilyToAvailableFamilyDictionary() keyEnumerator];
> +	   [desiredFamilyToAvailableFamilyDictionary() removeObjectForKey:
[keyEnumerator nextObject]];

Fast enumeration would probably be more efficient and I think reads a little
better.

    for (NSString *key in desiredFamilyToAvailableFamilyDictionary()) {
	[desiredFamilyToAvailableFamilyDictionary() removeObjectForKey:key];
	break;
    }

> Source/WebCore/platform/mac/WebFontCache.mm:139
> +    if (availableFamily)
> +	   [desiredFamilyToAvailableFamilyDictionary()
setObject:availableFamily forKey:desiredFamily];
> +    else
> +	   [desiredFamilyToAvailableFamilyDictionary() setObject:@""
forKey:desiredFamily];

Seems a little wordy this way. Might read better like this:

    [desiredFamilyToAvailableFamilyDictionary() setObject:availableFamily ?:
@"" forKey:desiredFamily];

Or like this:

    NSString *value = availableFamily ? availableFamily : @"";
    [desiredFamilyToAvailableFamilyDictionary() setObject:value
forKey:desiredFamily];

Could also consider representing null in the dictionary as NSNull rather than
with an empty string. Downside is you’d have to use id rather than NSString *
when getting values out of the dictionary. Upside is that NSNull is a more
logical direct mapping to/from nil than the empty string is.

> Source/WebCore/platform/mac/WebFontCache.mm:194
>      NSFontManager *fontManager = [NSFontManager sharedFontManager];

I’d put this down where it’s used in the non-cached case; seems a little ugly
to do this even in the cached case.

And on reflection, I think this function would be more readable broken into
three pieces:

1) A function that maps a desired family name to an available family name,
without caching.
2) A function that finds a desired family name to an available family name,
with caching, that calls (1) in the "not yet in cache" case.
3) A function that finds an NSFont, which calls (2).

I won’t insist on this refactoring, but I do think it could make the code
easier to read.

> Source/WebCore/platform/mac/WebFontCache.mm:200
> +	       // Already tried mapping the desired font to an available one in
the past and failed, so
> +	       // don't try again.

It's good to have a comment here, but this one seems a little too wordy. The
idea that we cache failures as well as successes is a normal part of the
memoization pattern; I wish there was a more straightforward way of separating
“it was cached” from “the cached value happens to be null” that makes this look
less like a special case, and more like “decoding empty string and turning it
into nil”.

> Source/WebCore/platform/mac/WebFontCache.mm:208
> +	   NSEnumerator *e = [[fontManager availableFontFamilies]
objectEnumerator];
> +	   while ((availableFamily = [e nextObject])) {

Could use fast enumeration:

    for (availableFamily in [fontManager availableFontFamilies]) {

> Source/WebCore/platform/mac/WebFontCache.mm:219
> +	       NSEnumerator *availableFonts = [[fontManager availableFonts]
objectEnumerator];
> +	       NSString *availableFont;
> +	       NSFont *nameMatchedFont = nil;
> +	       NSFontTraitMask desiredTraitsForNameMatch = desiredTraits |
(desiredWeight >= 7 ? NSBoldFontMask : 0);
> +	       while ((availableFont = [availableFonts nextObject])) {

Could use fast enumeration:

    for (NSString *availableFont in [fontManager availableFonts]) {