1170986 - Use Core Text APIs instead of Cocoa font manager for font enumeration

Status: RESOLVED FIXED

(Core :: Graphics: Text, task)

Description

[(not currently active) Ted Mielczarek]

The bulk of the changes here are in gfxMacPlatformFontList because the UIKit font listing APIs are completely different from Cocoa's:
 src/nsDeviceContext.cpp          |    4 -
 thebes/gfxCoreTextShaper.cpp     |    5 +
 thebes/gfxCoreTextShaper.h       |    4 +
 thebes/gfxFontUtils.cpp          |    2 
 thebes/gfxMacFont.h              |    4 +
 thebes/gfxMacPlatformFontList.h  |    3 +
 thebes/gfxMacPlatformFontList.mm |  103 ++++++++++++++++++++++++++++++++++-----
 thebes/gfxPlatform.cpp           |    8 +--
 thebes/gfxPlatformFontList.cpp   |    2 
 thebes/gfxPlatformFontList.h     |    2 
 thebes/gfxPlatformMac.cpp        |   32 ++++++++++--
 thebes/gfxQuartzSurface.h        |    4 +
 thebes/moz.build                 |    4 -
 13 files changed, 148 insertions(+), 29 deletions(-)

The rest of the patch is mostly a combination of swapping some #ifdef XP_MACOSX for XP_DARWIN, and wrapping #ifdef MOZ_WIDGET_COCOA around stuff that should be Mac-only and not iOS.

Comment 1

[Jonathan Kew [:jfkthame]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0)
> The bulk of the changes here are in gfxMacPlatformFontList because the UIKit
> font listing APIs are completely different from Cocoa's:

Without having actually seen your patch, I wonder.... can't we have a common font-list implementation based on iterating over the list returned by CTFontCollectionCreateFromAvailableFonts()? That should be available to both OS X and iOS.

Comment 2

[(not currently active) Ted Mielczarek]

That's a great question, I don't know! I'd love to not have two separate codepaths, certainly. My current patch is here:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/gecko-ios/rev/652dc1a7d9e0

The biggest changes are in gfxMacFontFamily::FindStyleVariations:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/gecko-ios/diff/652dc1a7d9e0/gfx/thebes/gfxMacPlatformFontList.mm#l1.118

and gfxMacPlatformFontList::InitFontList:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/gecko-ios/diff/652dc1a7d9e0/gfx/thebes/gfxMacPlatformFontList.mm#l1.316

If we could unify that code that'd be great. I don't really know much about CoreText, I just read the Apple docs and hack stuff 'till it works.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 1170986 - Make gfx build for iOS. r?jfkthame

bug 1170986 - Make gfx build for iOS. r?jfkthame

Comment 4

[(not currently active) Ted Mielczarek]

Putting this patch up for review just to get some eyes on it. I'm happy to look into the changes you've suggested.

Comment 5

[John Daggett (:jtd)]

Comment on attachment 8614788 [details]
MozReview Request: bug 1170986 - Make gfx build for iOS. r?jfkthame

I think we need to be a little more careful about not littering OSX code with awkward #ifdef's. I think we can also switch to using CoreText in some cases that would eliminate the need to do something different under iOS.

> +++ b/gfx/thebes/gfxMacFont.h	Tue Apr 07 07:09:22 2015 -0400
> @@ -4,17 +4,21 @@
> +#ifdef MOZ_WIDGET_COCOA
>  #include <ApplicationServices/ApplicationServices.h>
> +#else
> +#include <CoreText/CoreText.h>
> +#endif

Guessing we don't need the ApplicationServices.h include within the header. If possible, please eliminate the need for an #ifdef here.

> +++ b/gfx/thebes/gfxMacPlatformFontList.h	Tue Apr 07 07:09:22 2015 -0400
> @@ -2,16 +2,19 @@
> +#ifdef MOZ_WIDGET_UIKIT
> +#include <CoreText/CoreText.h>
> +#endif

No #ifdef please.

> +++ b/gfx/thebes/gfxMacPlatformFontList.mm	Tue Apr 07 07:09:22 2015 -0400
> @@ -37,33 +37,41 @@
> +#ifdef MOZ_WIDGET_COCOA
>  #import <AppKit/AppKit.h>
> +#else
> +#import <Foundation/Foundation.h>
> +#import <CoreGraphics/CoreGraphics.h>
> +#import <UIKit/UIKit.h>
> +#endif

Can we make this just AppKit.h vs. UIKit.h?

> @@ -427,25 +437,27 @@ gfxMacFontFamily::LocalizedName(nsAStrin
> +#ifdef MOZ_WIDGET_COCOA
>      NSString *family = GetNSStringForString(mName);
>      NSString *localized = [sFontManager
>                             localizedNameForFamily:family
>                                               face:nil];
>  
>      if (localized) {
>          GetStringForNSString(localized, aLocalizedName);
>          return;
>      }
> +#endif

This needs a comment, anything within UI will never get localized font names. Something like "xxx - ignoring localized fontnames on iOS for now"?

> @@ -465,42 +477,65 @@ gfxMacFontFamily::FindStyleVariations(Fo
>  {
>      if (mHasStyles)
>          return;
>  
>      nsAutoreleasePool localPool;
>  
>      NSString *family = GetNSStringForString(mName);
>  
> +#ifdef MOZ_WIDGET_COCOA
>      // create a font entry for each face
>      NSArray *fontfaces = [sFontManager
>                            availableMembersOfFontFamily:family];  // returns an array of [psname, style name, weight, traits] elements, goofy api
>      int faceCount = [fontfaces count];
>      int faceIndex;
>  
> +    nsAutoString postscriptFontName;
>      for (faceIndex = 0; faceIndex < faceCount; faceIndex++) {
>          NSArray *face = [fontfaces objectAtIndex:faceIndex];
>          NSString *psname = [face objectAtIndex:INDEX_FONT_POSTSCRIPT_NAME];
> +        GetStringForNSString(psname, postscriptFontName);
>          int32_t appKitWeight = [[face objectAtIndex:INDEX_FONT_WEIGHT] unsignedIntValue];
>          uint32_t macTraits = [[face objectAtIndex:INDEX_FONT_TRAITS] unsignedIntValue];
>          NSString *facename = [face objectAtIndex:INDEX_FONT_FACE_NAME];
> +#else  // UIKit
> +    UIFont* systemFont = [UIFont systemFontOfSize:[UIFont systemFontSize]];
> +    NSArray* fontNames;
> +    if ([family isEqualToString:[systemFont familyName]]) {
> +        // For some reason fontNamesForFamilyName doesn't work
> +        // for the default system font.
> +        NSMutableArray* names = [[NSMutableArray alloc] initWithCapacity:1];
> +        [names addObject:[systemFont fontName]];
> +        fontNames = names;
> +    } else {
> +        fontNames = [UIFont fontNamesForFamilyName:family];
> +    }
> +    for (NSString* psname in fontNames) {
> +        // make a nsString
> +        nsAutoString postscriptFontName;
> +        GetStringForNSString(psname, postscriptFontName);
> +
> +        CTFontRef ctFont = ::CTFontCreateWithName((CFStringRef)psname, 10, nullptr);
> +        CFDictionaryRef traits = ::CTFontCopyTraits(ctFont);
> +        NSString* facename = (NSString*)CTFontCopyName(ctFont, kCTFontStyleNameKey);
> +        CFNumberRef weight = (CFNumberRef)::CFDictionaryGetValue(traits, kCTFontWeightTrait);
> +        double appKitWeight;
> +        CFNumberGetValue(weight, kCFNumberDoubleType, &appKitWeight);
> +#endif

Two things:

We should *not* be using this sort of #ifdef style, it's brutal to try and maintain.

  #ifdef MOZ_WIDGET_COCOA
    for (xxx) {
      // OSX way
  #else 
    for (xxx)
      // iOS way
  #endif
      // common code
    }

Better to abstract out whatever iOS/OSX specific code there is into separate static functions.

In this case, I think we can skip the use of AppKit and just use CoreText API's instead. The tricky part is that the values for font weight/width/slant are harder to interpret correctly using the CoreText attribute values. But I think we could unify the iOS/OSX code via CoreText here.

The other thing is you shouldn't be using CTFont objects here but rather CTFontDescriptor objects. Something like this:

    CFMutableDictionaryRef attr =
      CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks);
    CFDictionaryAddValue(attr, kCTFontFamilyNameAttribute, family);
    CTFontDescriptorRef fd = CTFontDescriptorCreateWithAttributes(attr);
    CFArrayRef matchingFonts = CTFontDescriptorCreateMatchingFontDescriptors(fd, NULL);

    // iterate over faces in the family
    int f, numFaces = (int) CFArrayGetCount(matchingFonts);
    for (f = 0; f < numFaces; f++) {
      CTFontDescriptorRef faceDesc = (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f);
      if (!faceDesc) {
        continue;
      }
      CFStringRef faceName = (CFStringRef) CTFontDescriptorCopyAttribute(faceDesc, kCTFontNameAttribute);
      CFDictionaryRef fontTraits = (CFDictionaryRef) CTFontDescriptorCopyAttribute(faceDesc, kCTFontTraitsAttribute);
      CFNumberRef num;
      CGFloat weight, width, slant;
      int32_t symbolic;
      num = (CFNumberRef) CFDictionaryGetValue(fontTraits, kCTFontWeightTrait);
      CFNumberGetValue(num, kCFNumberCGFloatType, &weight);
      num = (CFNumberRef) CFDictionaryGetValue(fontTraits, kCTFontWidthTrait);
      CFNumberGetValue(num, kCFNumberCGFloatType, &width);
      num = (CFNumberRef) CFDictionaryGetValue(fontTraits, kCTFontSlantTrait);
      CFNumberGetValue(num, kCFNumberCGFloatType, &slant);
      num = (CFNumberRef) CFDictionaryGetValue(fontTraits, kCTFontSymbolicTrait);
      CFNumberGetValue(num, kCFNumberIntType, &symbolic);
    }

> @@ -674,19 +735,19 @@ gfxMacPlatformFontList::InitFontList()
>      // iterate over available families
> -
> +#ifdef MOZ_WIDGET_COCOA
>      CFArrayRef familyNames = CTFontManagerCopyAvailableFontFamilyNames();
>  
>      // iterate over families
>      uint32_t i, numFamilies;
>  
>      numFamilies = CFArrayGetCount(familyNames);
>      for (i = 0; i < numFamilies; i++) {

Here too, the weird #ifdef is terrible. Qt code handles this quite nicely:

http://code.woboq.org/qt5/qtbase/src/platformsupport/fontdatabases/mac/qcoretextfontdatabase.mm.html

static CFArrayRef availableFamilyNames()
{
#if defined(Q_OS_OSX)
  return CTFontManagerCopyAvailableFontFamilyNames();
#elif defined(Q_OS_IOS)
  return (CFArrayRef) [[UIFont familyNames] retain];
#endif
}

> +    [fonts addObject:[[UIFont systemFontOfSize:[UIFont systemFontSize]] familyName]];

So [UIFont familyNames] doesn't return hidden system font names? I think you're going to be adding double entries here as the system font may or may not be hidden (e.g. iOS used Helvetica Neue so I'm guessing that wasn't hidden). And the actual font may vary with localization (i.e. en-US font will not be the same as the ja-JP font).

> @@ -1035,20 +1108,24 @@ public:
>      virtual void Load() {
>          nsAutoreleasePool localPool;
> +#ifdef MOZ_WIDGET_COCOA
>          // bug 975460 - async font loader crashes sometimes under 10.6, disable
>          if (nsCocoaFeatures::OnLionOrLater()) {
>              FontInfoData::Load();
>          }
> +#else
> +        FontInfoData::Load();
> +#endif

Put the test logic in a macro instead please.

>  gfxPlatformMac::UseAcceleratedCanvas()
>  {
> -  // Lion or later is required
> -  return nsCocoaFeatures::OnLionOrLater() && Preferences::GetBool("gfx.canvas.azure.accelerated", false);
> +    bool pref = Preferences::GetBool("gfx.canvas.azure.accelerated", false);
> +#ifdef MOZ_WIDGET_COCOA
> +    // Lion or later is required
> +    return nsCocoaFeatures::OnLionOrLater() && pref;
> +#else
> +    return pref;
> +#endif
>  }

Ditto.

> bool
>  gfxPlatformMac::UseProgressivePaint()
>  {
> +#ifdef MOZ_WIDGET_COCOA
>    // Progressive painting requires cross-process mutexes, which don't work so
>    // well on OS X 10.6 so we disable there.
>    return nsCocoaFeatures::OnLionOrLater() && gfxPlatform::UseProgressivePaint();
> +#else
> +  return gfxPlatform::UseProgressivePaint();
> +#endif
>  }

Ditto.

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to John Daggett (:jtd) from comment #5)
> Comment on attachment 8614788 [details]
> MozReview Request: bug 1170986 - Make gfx build for iOS. r?jfkthame
> 
> I think we need to be a little more careful about not littering OSX code
> with awkward #ifdef's. I think we can also switch to using CoreText in some
> cases that would eliminate the need to do something different under iOS.

Yeah, jfkthame's suggestion seems sane. Thanks for looking at the patch! It was a pretty hack-and-slash approach to getting things working.

> > +++ b/gfx/thebes/gfxMacPlatformFontList.mm	Tue Apr 07 07:09:22 2015 -0400
> > @@ -37,33 +37,41 @@
> > +#ifdef MOZ_WIDGET_COCOA
> >  #import <AppKit/AppKit.h>
> > +#else
> > +#import <Foundation/Foundation.h>
> > +#import <CoreGraphics/CoreGraphics.h>
> > +#import <UIKit/UIKit.h>
> > +#endif
> 
> Can we make this just AppKit.h vs. UIKit.h?

Yeah, I think AppKit.h pulls all that in for you (but UIKit.h doesn't, maybe?)




> Here too, the weird #ifdef is terrible. Qt code handles this quite nicely:
> 
> http://code.woboq.org/qt5/qtbase/src/platformsupport/fontdatabases/mac/
> qcoretextfontdatabase.mm.html

Thanks, I agree this code is awful as written.

> 
> static CFArrayRef availableFamilyNames()
> {
> #if defined(Q_OS_OSX)
>   return CTFontManagerCopyAvailableFontFamilyNames();
> #elif defined(Q_OS_IOS)
>   return (CFArrayRef) [[UIFont familyNames] retain];
> #endif
> }
> 
> > +    [fonts addObject:[[UIFont systemFontOfSize:[UIFont systemFontSize]] familyName]];
> 
> So [UIFont familyNames] doesn't return hidden system font names? I think
> you're going to be adding double entries here as the system font may or may
> not be hidden (e.g. iOS used Helvetica Neue so I'm guessing that wasn't
> hidden). And the actual font may vary with localization (i.e. en-US font
> will not be the same as the ja-JP font).

The default system font on my iPhone 6 running iOS 8 is ".Helvetica Neue Interface". (It's trivial to check: http://www.quora.com/What-is-the-system-font-in-iOS-8 )

This was quite a hassle for me to figure out, maybe using the CoreText APIs directly will make this less awful.

Thanks for the comments, I'll try to get the patch cleaned up.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Use Core Text APIs instead of Cocoa font manager wherever possible in gfxMacPlatformFontList

I spent a bit of time last night looking through gfxMacPlatformFontList to see if it could be moved off the Cocoa font manager and onto purely Core Text APIs instead. The attached patch seems to work for me locally in simple testing (I'll push it to try in a moment, as I expect it'll break some reftests, at least -- I have not fine-tuned the attribute mappings, etc), and should come much closer to being iOS-compatible than current trunk code. I don't have an iOS build env set up, so have not attempted to actually compile this; there'll be at least a bit of header #ifdef-ing still required, but almost all the Cocoa-specific stuff has been excised.

Comment 8

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ee8869312f

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Use Core Text APIs instead of Cocoa font manager wherever possible in gfxMacPlatformFontList

Here's a better version of the patch; let's see if tryserver likes it any better.

Comment 10

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af8f0020ee8d

Comment 11

[Jonathan Kew [:jfkthame]]

Ah, drat; CTFontCollectionCopyFontAttribute() isn't available on 10.6; it was new in 10.7.

Ted, could you check if it's usable on iOS? The online docs seem to be rather incomplete -- they describe the "Option bits for use with CTFontCollectionCopyFontAttribute(s)", but fail to mention the functions themselves!

Comment 12

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=243c798f7c38

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Use Core Text APIs instead of Cocoa font manager wherever possible in gfxMacPlatformFontList

This patch seems to very nearly pass on tryserver, but there's some kind of spacing/metrics discrepancy with the system font on OS X 10.10. I'm not currently running 10.10, which makes it hard to do the tracing/debugging to figure out what's going wrong there.... are you on 10.10 by any chance, John? If so, could you take a look?

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 8614788 [details]
MozReview Request: bug 1170986 - Make gfx build for iOS. r?jfkthame

bug 1170986 - Make gfx build for iOS. r?jfkthame

Comment 15

[(not currently active) Ted Mielczarek]

I'm going to hand this bug over to you and file a new one for the rest of my patch. Thanks for taking this on! I still need the rest of my patch, but it's a lot less awful with yours applied.

Comment 16

[John Daggett (:jtd)]

As I suspected, this patch leads to InitFontList taking much longer to iterate over all fonts. Running on my system in a debug build (keep in mind the system code is optimized), it goes from 47ms to just under 150ms. I've added logging for this and pushed a couple trybuilds so we can do an A/B comparison (and see how this affects ts_paint).

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #16)
> As I suspected, this patch leads to InitFontList taking much longer to
> iterate over all fonts. Running on my system in a debug build (keep in mind
> the system code is optimized), it goes from 47ms to just under 150ms. I've
> added logging for this and pushed a couple trybuilds so we can do an A/B
> comparison (and see how this affects ts_paint).

Yes, I'm not surprised it takes longer, but it's not clear to me how serious that will be... testing obviously needed.

Note that it will be worst on 10.6, where CTFontCollectionCopyFontAttribute isn't available and so the patch fakes it in a way that I'd expect to be significantly more expensive. I'm more interested in the impact on 10.7 and later.

Comment 18

[John Daggett (:jtd)]

optimized trybuild times for iterating over font families in InitFontList
OSX 10.8, 438 families, 1252 fonts

existing trunk code - 8.00ms, 7.71ms
with iOS Coretext fontlist patch - 62.22ms, 58.83ms

So the patch is introducing an extra 50ms at startup.

Within gfxMacPlatformFontList::InitFontList():

> -    CFArrayRef familyNames = CTFontManagerCopyAvailableFontFamilyNames();
> +    // get a list of families from the collection of available fonts
> +    CTFontCollectionRef collection =
> +        CTFontCollectionCreateFromAvailableFonts(nullptr);

The problem we have is that CTFontManagerCopyAvailableFontFamilyNames isn't available on iOS (why?!?). So we have to iterate over fonts to pull out the family names which is ugly. My suggestion would be to simply bracket the call to CTFontManagerCopyAvailableFontFamilyNames with an OSX #ifdef and for the iOS case create a static method that uses CTFontCollectionCreateFromAvailableFonts to synthesize the array of names returned by CTFontManagerCopyAvailableFontFamilyNames. That will remove the perf impact that the new code has on startup.

I suspect using CoreText instead of AppKit within gfxMacFontFamily::FindStyleVariations will produce minor changes in terms of the weight/width/slope values for fonts within families. We need to do a comparison of family composition across [10.6 ... 10.10]. Or we can just use the AppKit code for OSX and only use the CoreText code for iOS, that would make it easier to land this more quickly.

Comment 19

[(not currently active) Ted Mielczarek]

I can definitely live with some iOS #ifdefs as long as they're not as bad as my original patch. I'm sure nobody wants to eat a 50ms startup regression on desktop!

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #18)
> optimized trybuild times for iterating over font families in InitFontList
> OSX 10.8, 438 families, 1252 fonts
> 
> existing trunk code - 8.00ms, 7.71ms
> with iOS Coretext fontlist patch - 62.22ms, 58.83ms
> 
> So the patch is introducing an extra 50ms at startup.
> 
> Within gfxMacPlatformFontList::InitFontList():
> 
> > -    CFArrayRef familyNames = CTFontManagerCopyAvailableFontFamilyNames();
> > +    // get a list of families from the collection of available fonts
> > +    CTFontCollectionRef collection =
> > +        CTFontCollectionCreateFromAvailableFonts(nullptr);
> 
> The problem we have is that CTFontManagerCopyAvailableFontFamilyNames isn't
> available on iOS (why?!?). So we have to iterate over fonts to pull out the
> family names which is ugly. My suggestion would be to simply bracket the
> call to CTFontManagerCopyAvailableFontFamilyNames with an OSX #ifdef and for
> the iOS case create a static method that uses
> CTFontCollectionCreateFromAvailableFonts to synthesize the array of names
> returned by CTFontManagerCopyAvailableFontFamilyNames. That will remove the
> perf impact that the new code has on startup.

Yeah, if we just need to #ifdef that bit, it seems OK.

My other concern is with the test failures that showed up on my try run (logs no longer available, unfortunately). IIRC, there seemed to be an issue with the system font on 10.10, which I'm not in a position to investigate.

Comment 21

[John Daggett (:jtd)]

To support system fonts correctly under 10.11 we're going to need to revamp system font handling a bit. I think that will allow us to clean up the "hidden" font handling a bit here.

https://bugzilla.mozilla.org/show_bug.cgi?id=1201318#c18

Comment 22

[(not currently active) Ted Mielczarek]

(In reply to Jonathan Kew (:jfkthame) from comment #11)
> Ah, drat; CTFontCollectionCopyFontAttribute() isn't available on 10.6; it
> was new in 10.7.
> 
> Ted, could you check if it's usable on iOS? The online docs seem to be
> rather incomplete -- they describe the "Option bits for use with
> CTFontCollectionCopyFontAttribute(s)", but fail to mention the functions
> themselves!

Humorously I just updated to Xcode 7 and building with the iOS 9 SDK shows that this function has been removed from the iOS SDK.

Comment 23

[(not currently active) Ted Mielczarek]

Wasn't that hard to work around that though:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/gecko-ios/rev/e49cc11d3a56#l2.16

Comment 24

[(not currently active) Ted Mielczarek]

What can I do to help get this patch landed? It's one of the last remaining bits for iOS support on trunk.

Comment 25

[John Daggett (:jtd)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #24)
> What can I do to help get this patch landed? It's one of the last remaining
> bits for iOS support on trunk.

The patch needs to be reworked for a few reasons. The system font changes done on bug 1201318 will affect how this patch is structured. The patch introduces a 50ms regression at startup under OSX. Using CoreText calls to determine font attributes under OSX will introduce regressions I think, I think it would be easier to use that code for iOS only and leave the current OSX code as is.

I also think figuring out how to enumerate system font families is still a bit tricky and we need to confirm that what we're implementing works well under iOS 9.0 where the complicated San Francisco family is used as the system font family.

Steps:

1. Isolate family name enumeration, something along the lines of what Qt does:

http://code.woboq.org/qt5/qtbase/src/platformsupport/fontdatabases/mac/qcoretextfontdatabase.mm.html

  static CFArrayRef availableFamilyNames()
  {
  #if defined(Q_OS_OSX)
      return CTFontManagerCopyAvailableFontFamilyNames();
  #elif defined(Q_OS_IOS)
      return (CFArrayRef) [[UIFont familyNames] retain];
  #endif
  }
  
2. Figure out an iOS version of gfxMacPlatformFontList::InitSystemFonts

  a. How can we populate the mSystemFontFamilies hashtable?

  b. Is there a single system font family used across system font calls?

  c. Under iOS 9.0, does the San Francisco work the same way, with a different family used for different sizes? (Guess: yes)

3. Make a separate iOS font family object, gfxIOSFontFamily, and use the code Jonathan put together for looking up style attributes of individual fonts within a family.

Note that CoreText contains a private set of weight cutoffs that we should use as a reference:

  CGFloat gCTFontWeights[9] = {
      -0.800000, // 100
      -0.500000, // 200
      -0.400000, // 300
      0.000000,  // 400
      0.230000,  // 500
      0.300000,  // 600
      0.400000,  // 700
      0.560000,  // 800
      0.620000   // 900
  };
  
(from: nm -a /System/Library/Frameworks/CoreText.framework/CoreText | grep kCTFontWeight)

Comment 26

[John Daggett (:jtd)]

Looking over the iOS font class docs, it looks like the iOS equivalent of doing per-face enumeration is [UIFont fontNamesForFamilyName:]. This will return an array of postscript names for a given family. I think it would be helpful to see if we can flush out the system font families this way:

  UIFont* sys = [UIFont systemFontOfSize: 8.0];
  NSString* familyName = [sys familyName];
  for (auto f in [UIFont fontNamesForFamilyName: familyName]) {
    CTFontRef fontRef = (CTFontRef)[UIFont fontWithName: f size: 8.0];
    // look up attributes
  }

  UIFont* sys = [UIFont systemFontOfSize: 128.0];
  NSString* familyName = [sys familyName];
  for (auto f in [UIFont fontNamesForFamilyName: familyName]) {
    CTFontRef fontRef = (CTFontRef)[UIFont fontWithName: f size: 128.0];
    // look up attributes
  }

Comment 27

[(not currently active) Ted Mielczarek]

Thanks, I'll take a crack at that.

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use CoreFont to enumerate fonts on iOS

I've split the CF stuff into a separate file for now. I think having NSFont and CF in the same file is pretty
confusing, and avoids the regression on Mac.

I haven't had time to look at the suggestions in comment #26, but maybe we can land this first and refine later?

Comment 29

[John Daggett (:jtd)]

Comment on attachment 8681342 [details] [diff] [review]
Use CoreFont to enumerate fonts on iOS

Review of attachment 8681342 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to James Willcox (:snorp) ([e-mail]) from comment #28)
> Created attachment 8681342 [details] [diff] [review]
> Use CoreFont to enumerate fonts on iOS
> 
> I've split the CF stuff into a separate file for now. I think having NSFont
> and CF in the same file is pretty
> confusing, and avoids the regression on Mac.
> 
> I haven't had time to look at the suggestions in comment #26, but maybe we
> can land this first and refine later?

Sorry but I don't think duping the entire file is a good idea, that leads to code maintenance headaches. The patch you have here is already missing two important changes to gfxMacPlatformFontList.mm since your forked version.

The changes noted in comment 26 are one of the reasons for this I think, the code logic for system fonts was changed so that a generic name is passed down for the "system font" which is translated into the right system font family without the need to enumerate hidden system fonts. For iOS we need to tweak the contents of InitSystemFonts() to populate mSystemFontFamilies.

The changes in the patch that are switching from NSString to CFString aren't necessary. The localized name routines for gfxSingleFaceMacFontFamily aren't needed on iOS.

So here are the changes I would propose:

- patch to restructure code to isolate NSFont-specific code similar
  to what you're doing in your patch

- patch to #ifdef OSX/iOS versions of routines

Is the plan for iOS to use all the OSX prefs in all.js?

Comment 30

[Jonathan Kew [:jfkthame]]

Another possible implementation strategy, perhaps avoiding much of the #ifdef ugliness, would be to have a shared class (maybe gfxCoreTextFontList?) to handle everything that's common to OS X and iOS (most of the current gfxMacPlatformFontList code, I think), and then small OS X- and iOS-specific subclasses of that, which would provide the separate implementations of things like initializing the font list.

Comment 31

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #30)
> Another possible implementation strategy, perhaps avoiding much of the
> #ifdef ugliness, would be to have a shared class (maybe
> gfxCoreTextFontList?) to handle everything that's common to OS X and iOS
> (most of the current gfxMacPlatformFontList code, I think), and then small
> OS X- and iOS-specific subclasses of that, which would provide the separate
> implementations of things like initializing the font list.

Yup, I think that's probably a cleaner way to do this.

I have a feeling we need to set up a separate pref font list for iOS. I don't thing the OSX one is appropriate.

Comment 32

[Jonathan Kew [:jfkthame]]

Attachment: Refactor CG/CoreText font-list code to provide OS X and iOS implementations with a common base

Here's a patch that aims to do something like I suggested in comment 30. It builds and appears to work fine on OS X; I haven't got an iOS build going yet, so that side of things is completely untested.

Comment 33

[Jonathan Kew [:jfkthame]]

Comment on attachment 8697716 [details] [diff] [review]
Refactor CG/CoreText font-list code to provide OS X and iOS implementations with a common base

Bearing in mind that this hasn't been compiled on iOS at all yet, I'm sure there will be issues of some kind, but feedback on the overall patch is welcome.

Brief overview:

- We introduce a CoreTextFontList abstract class that contains most of the old gfxMacPlatformFontList

- gfxMacPlatformFontList then becomes a small subclass of the CoreTextFontList that uses Cocoa to fill in the missing pieces

- and there's a new (untested) gfxIOSPlatformFontList class that does the same for iOS using UIKit

(gfxMacFontFamily gets similar treatment, as it needs per-platform implementations of FindStyleVariations; the CT-based code that should be both OS X and iOS-compatible unfortunately doesn't work with hidden system fonts on OS X, AFAICT.)

Looking at the patch here, I can see it wants some general cleanup before actual review, but if one of you with a working iOS build would like to try it there (and fix any glaring errors!), that would be awesome.

Comment 34

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #33)
> (gfxMacFontFamily gets similar treatment, as it needs per-platform
> implementations of FindStyleVariations; the CT-based code that should be
> both OS X and iOS-compatible unfortunately doesn't work with hidden system
> fonts on OS X, AFAICT.)

I think we can probably sniff out the system fonts by (1) using the [UIFont sysFontForSize:] methods (2) getting the underlying "real" family name via the font descriptor (rather than the weird meta name from [NSFont familyName]) and then (3) pulling the associated non-Latin system fonts via the fallback cascade.

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8697716 [details] [diff] [review]
Refactor CG/CoreText font-list code to provide OS X and iOS implementations with a common base

Review of attachment 8697716 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't ever had a chance to try this, but I will if I ever get back to iOS stuff.

Comment 36

[Andrey Bienkowski]

https://bugzilla.mozilla.org/show_bug.cgi?id=1548706

Comment 37

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1170986 - patch 0 - Reformat gfxMacPlatformFontList.mm with clang-format as though it were a .cpp file.

No functional change. The issue here is that we're going to move a lot of the contents of this file
to a new .cpp source file; but we have different line-length specifications for .mm vs .cpp files,
and so that results in clang-format making a lot of whitespace-only changes to adjust the lines.

To try and simplify review, this preparatory patch is created by running clang-format on the file
as if it were a .cpp source, so that the whitespace adjustments get done. Then the following patch,
which moves much of the content to CoreTextFontList.cpp, will already see the whitespace in the form
it expects.

Comment 38

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1170986 - patch 1 - Refactor gfxMacPlatformFontList to separate out an abstract, Cocoa-independent CoreTextFontList class, and a small Cocoa-specific gfxMacPlatformFontList implementation.

This should not change user-visible behavior, in principle (though it's possible there will be
minor changes due to using more Core Text APIs in place of Cocoa to enumerate available fonts,
and they may interpret font attributes slightly differently).

AFAICS current macOS tests continue to pass with this refactoring.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1170986 - patch 1.1 - Reformat the remains of gfxMacPlatformFontList.mm back to Obj-C++ style.

No functional change; just applying clang-format to what remains in the Obj-C++ file here,
after factoring out the common Core Text part to a separate .cpp source file.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86e965ddf6c1
patch 0 - Reformat gfxMacPlatformFontList.mm with clang-format as though it were a .cpp file. r=gfx-reviewers,nical
https://hg.mozilla.org/integration/autoland/rev/398cc87d7091
patch 1 - Refactor gfxMacPlatformFontList to separate out an abstract, Cocoa-independent CoreTextFontList class, and a small Cocoa-specific gfxMacPlatformFontList implementation. r=gfx-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/22701f8e759e
patch 1.1 - Reformat the remains of gfxMacPlatformFontList.mm back to Obj-C++ style. r=gfx-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/42ff0ef2bc5d
apply code formatting via Lando

Comment 41

[Cosmin Sabou [:CosminS]]

Backed out for causing osx accessible failures on browser_text.js.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Crunning%2Cpending%2Crunnable&fromchange=b31464975bb47ae4e77397f563a686b9ad30ca00&searchStr=os%2Cx%2C10.15%2Cwebrender%2Cdebug%2Cmochitests%2Ctest-macosx1015-64-qr%2Fdebug-mochitest-browser-a11y%2Cba&tochange=33d1c42b026e21617602c6006491a09d2380fce2&selectedTaskRun=e8cIH0hQTriB6xbOPgIa0Q.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=427555911&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/165164f7cde3e6fd6f1eccdc8b63aff86bc87b88

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31bf68247e6e
patch 0 - Reformat gfxMacPlatformFontList.mm with clang-format as though it were a .cpp file. r=gfx-reviewers,nical
https://hg.mozilla.org/integration/autoland/rev/2c9991086e3c
patch 1 - Refactor gfxMacPlatformFontList to separate out an abstract, Cocoa-independent CoreTextFontList class, and a small Cocoa-specific gfxMacPlatformFontList implementation. r=gfx-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/64ceb33533a4
patch 1.1 - Reformat the remains of gfxMacPlatformFontList.mm back to Obj-C++ style. r=gfx-reviewers,bradwerth

Comment 43

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/31bf68247e6e
https://hg.mozilla.org/mozilla-central/rev/2c9991086e3c
https://hg.mozilla.org/mozilla-central/rev/64ceb33533a4