Bug 148218 – [OS X] Remove dead code from FontCache::systemFallbackForCharacters()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[OS X] Remove dead code from FontCache::systemFallbackForCharacters()

[Myles C. Maxfield]

Created attachment 259460 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 259460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259460&action=review

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:-565
> -        if (NSFont *bestVariation = [fontManager fontWithFamily:[substituteFont familyName] traits:traits weight:weight size:size]) {

No need for the "size" variable

[Myles C. Maxfield]

Created attachment 259567 [details]
Patch

[Daniel Bates]

Comment on attachment 259567 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259567&action=review

> Source/WebCore/ChangeLog:8
> +        CTFontCreateForCharactersWithLanguage() will always return the best

For your consideration, I suggest writing this sentence in terms of FontCache::lookupCTFont() (which calls CTFontCreateForCharactersWithLanguage() at the time of writing) instead of CTFontCreateForCharactersWithLanguage() since FontCache::systemFallbackForCharacters() does not explicitly call CTFontCreateForCharactersWithLanguage().

> Source/WebCore/ChangeLog:10
> +        font. Also, all fonts that will be created on WebKit's behalf are
> +        already printer fonts.

You may want to consider adding a comment to the code to explain that FontCache::lookupCTFont()/CTFontCreateForCharactersWithLanguage() will return a printer-friendly font.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:-543
> -    CGFloat size;

With the removal of this size variable and associated logic the comment above fontManager is now out-of-date. Please update the comment above fontManager.

[Myles C. Maxfield]

Comment on attachment 259567 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259567&action=review

>> Source/WebCore/ChangeLog:10
>> +        already printer fonts.
> 
> You may want to consider adding a comment to the code to explain that FontCache::lookupCTFont()/CTFontCreateForCharactersWithLanguage() will return a printer-friendly font.

Going forward, the concept of a "printer font" will become less and less relevant. I don't think it's worth paying it any more lip service.

[Myles C. Maxfield]

Committed r188802: <http://trac.webkit.org/changeset/188802>