Closed Bug 869762 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics: Text, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jtd, Assigned: jaas)

Details

Attachments

(2 files)

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
QA Contact: joshmoz
Assignee: jdaggett → joshmoz
QA Contact: joshmoz
Part of the word described here is done in bug 925241.
Summary: replace calls to ATS with calls to CoreText APIs → Use Core Foundation notifications instead of ATS for font changes on OS X
Attached patch Fix v1.0Splinter Review
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.
Attachment #815528 - Flags: review?(jdaggett)
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.
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.
(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 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.
Attachment #815528 - Flags: review?(jdaggett) → review+
Attached patch Fix v1.1Splinter Review
Fixed the nit from jdaggett and removed Carbon includes which are no longer necessary.
https://hg.mozilla.org/mozilla-central/rev/c62ad7dd57cd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.