869762 - Use Core Foundation notifications instead of ATS for font changes on OS X

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

Code within gfxMacPlatformFontList.mm still uses deprecated ATS (Apple Type Services) calls.  These should be replaced with a mixture of calls to CoreText APIs to avoid problems when the deprecated API is removed in future versions of OSX (10.9 is slated to be released this summer).

The ATS notification and ATSGetGeneration can be replaced with kCTFontManagerRegisteredFontsChangedNotification notification, this is what Webkit code does:

http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/mac/FontCacheMac.mm#L55

Comment 1

[Josh Aas]

Part of the word described here is done in bug 925241.

Comment 2

[Josh Aas]

Attachment: Fix v1.0

Please review carefully, this compiles but I did not test it since I don't know much about what all of this code is supposed to be doing.

Comment 3

[Jonathan Kew [:jfkthame]]

A brief test sequence:
(1) Verify that Zapfino is present in the font list in Preferences/Content. Close the Preferences window.
(2) Open a simple testcase such as
      data:text/html,<div style="font-family:zapfino">Hello world
    and confirm that it shows the expected font.
(3) Open Font Book.app, find Zapfino in the list, and Disable it (Shift-Cmd-D). The testcase should immediately revert to displaying the browser's default font.
(4) Go back to Preferences/Content and check that Zapfino is no longer in the list. Close Preferences again.
(5) Go back to Font Book and re-enable Zapfino (Shift-Cmd-D again). The testcase should again display in Zapfino, and it should reappear in the Preferences/Content font list.

Comment 4

[Josh Aas]

One thing in particular that needs review, that I'm not sure is covered by the test case in comment 3, is that I removed the font generation tracking code, which was supposedly there for some reason related to downloadable fonts. I don't think CT has a replacement, but maybe I'm wrong.

Comment 5

[John Daggett (:jtd)]

(In reply to Josh Aas (Mozilla Corporation) from comment #4)
> One thing in particular that needs review, that I'm not sure is covered by
> the test case in comment 3, is that I removed the font generation tracking
> code, which was supposedly there for some reason related to downloadable
> fonts. I don't think CT has a replacement, but maybe I'm wrong.

The generation stuff was used to deal with downloadable fonts.  We *don't* want to rebuild the list of platform fonts when a downloadable font is loaded.  The platform font list should only change when changes to the system font list changes (e.g. enable/disable a given face in FontBook).

I'll test whether the current patch works in both situations.

Comment 6

[John Daggett (:jtd)]

Comment on attachment 815528 [details] [diff] [review]
Fix v1.0

Ok, tested this with downloadable fonts and they don't cause a
notification to occur.

Patch looks fine, except one small nit:

> +    static void FontCacheChangedNotificationCallback(CFNotificationCenterRef center,

Describing this as "FontCache..." isn't so great. Since the
notification is labeled with kCTFontManagerRegisteredFontsChangedNotification,
I'd suggest following that pattern.  How about "RegisteredFontsChanged"?

r+ with that fixed.

Comment 7

[Josh Aas]

Attachment: Fix v1.1

Fixed the nit from jdaggett and removed Carbon includes which are no longer necessary.

Comment 8

[Josh Aas]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c62ad7dd57cd

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c62ad7dd57cd