Remove Mac native font type members from FontPlatformData

Remove Mac native font type members from FontPlatformData

FontPlatformData keeps several Mac specific font members 
around, NSFont, ctFont, cgFont, while all other platforms
rely on just using an SkTypeface. This leads to a lot of 
ifdefs in the code, confusion about which member is currently
instantiated and a history of null-dereference crashes on Mac
due to hard to track failures of font instantiation.

After we got access to a CTFont object from an
SkTypeface for compatibility with HarfBuzz AAT shaping and
clipboard text export [1], I suggest to remove the native 
handles and focus on using SkTypefaces. This allows us as 
well to remove a lot of mostly redundant platform-specific
code on Mac.

[1] https://codereview.chromium.org/872963003/

BUG=414234
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192506

[Dominik Röttsches, 2015-01-26 18:21:24]

dominik.rottsches@intel.com changed reviewers:
+ eae@chromium.org

[Dominik Röttsches, 2015-01-26 18:22:16]

Very early stages, and need to check a lot of things still, but it's displaying
something and it still loads web fonts, so not too far off :-) 

Requires: https://codereview.chromium.org/872963003/

[eae, 2015-02-10 15:40:45]

On 2015/01/26 18:22:16, Dominik Röttsches wrote:
> Very early stages, and need to check a lot of things still, but it's
displaying
> something and it still loads web fonts, so not too far off :-) 
> 
> Requires: https://codereview.chromium.org/872963003/

I like it!

[Dominik Röttsches, 2015-02-12 16:44:02]

dominik.rottsches@intel.com changed reviewers:
+ rsesek@chromium.org

[Dominik Röttsches, 2015-02-20 15:50:47]

The mac failures result from different fallback behavior when falling back from
web fonts. Previously, the input to NSFont's findFontLike for web fonts was 0
(null), now it is actually an NSFont object corresponding to the web fonts. This
results in different fallback behavior. While previously in most cases Helvetica
was selected, now depending on the font file "Manaco" or others are chosen. I
don't really know if any of these two variants is better than the other, given
that the findFontLike API is SPI and deprecated.

Probably we should change this first to using documented, public CoreText API
first, and use a non-null input for web fonts.

[Robert Sesek, 2015-02-20 17:39:23]

Can you update the CL description? What is this CL trying to accomplish? Are
there any side effects (besides the one you describe below about the Mac
fallback) here?

[eae, 2015-02-20 19:26:48]

That sounds like the right long term approach. For now the current behavior
seems fine.

[Dominik Röttsches, 2015-02-23 12:42:12]

> Can you update the CL description? 

Updated, hope this helps.

> What is this CL trying to accomplish? Are
> there any side effects (besides the one you describe below about the Mac
> fallback) here?

The code is trying to remove a lot of platform specific code, for the sake of
generally simplifying FontPlatformData and SimpleFontData, reducing the number
of font classes that we have lying around, with quite vague rationales and
unifying the font code across various platform. The goal is to make this change
without any functional changes. Unfortunately, this one change about fallback
behavior can be observed, otherwise I hope it's fine, although our test coverage
when interacting with system fonts could be better.

[Robert Sesek, 2015-02-24 16:13:08]

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
File Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontCacheMac.mm:120: NSFont* nsFont =
const_cast<NSFont*>(reinterpret_cast<const NSFont*>(platformData.ctFont()));
Can you create something lke toCTFontRef() or leave a comment saying that CTFont
is toll-free bridged to NSFont?

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
File Source/platform/fonts/mac/FontPlatformDataMac.mm (right):

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:42: NSString* fontName =
(NSString*)CGFontCopyPostScriptName(cgFont);
Use reinterpret_cast<> again like in the other CF-to-NS conversion.

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:53: const void* descriptors[] =
{ CTFontDescriptorCreateWithNameAndSize(CFSTR("LastResort"), 0) };
Functions that follow the Create naming scheme return objects that are strongly
owned. Using kCFTypeArrayCallBacks on the line below will cause all the values
to also be CFRetain'ed, which means that this object is leaked.

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:55: RetainPtr<CFDictionaryRef>
attributes(AdoptCF, CFDictionaryCreate(kCFAllocatorDefault, keys, values,
WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
Similarly, the array in |values| will be leaked because of
kCFTypeDictionaryValueCallBacks.

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:81: CTFontRef ctFont =
CTFontCreateWithGraphicsFont(cgFont, textSize, 0,
cascadeToLastResortFontDescriptor());
Create is strong ref. Unless SkCreateTypefaceFromCTFont adopts the reference,
then this is leaked.

[Dominik Röttsches, 2015-03-19 15:20:00]

Thank you for the thorough and instructive review, I learned to look closer.
There are probably more such cases to be found in the Mac font code when
auditing for those.

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
File Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontCacheMac.mm:120: NSFont* nsFont =
const_cast<NSFont*>(reinterpret_cast<const NSFont*>(platformData.ctFont()));
On 2015/02/24 16:13:07, Robert Sesek wrote:
> Can you create something lke toCTFontRef() or leave a comment saying that
CTFont
> is toll-free bridged to NSFont?

Added toNSFont to FontPlatformData.h

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
File Source/platform/fonts/mac/FontPlatformDataMac.mm (right):

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:42: NSString* fontName =
(NSString*)CGFontCopyPostScriptName(cgFont);
On 2015/02/24 16:13:07, Robert Sesek wrote:
> Use reinterpret_cast<> again like in the other CF-to-NS conversion.

Done.

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:53: const void* descriptors[] =
{ CTFontDescriptorCreateWithNameAndSize(CFSTR("LastResort"), 0) };
On 2015/02/24 16:13:07, Robert Sesek wrote:
> Functions that follow the Create naming scheme return objects that are
strongly
> owned. Using kCFTypeArrayCallBacks on the line below will cause all the values
> to also be CFRetain'ed, which means that this object is leaked.

Tried to fixed this case by using RetainPtr wrappers.

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:55: RetainPtr<CFDictionaryRef>
attributes(AdoptCF, CFDictionaryCreate(kCFAllocatorDefault, keys, values,
WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
On 2015/02/24 16:13:07, Robert Sesek wrote:
> Similarly, the array in |values| will be leaked because of
> kCFTypeDictionaryValueCallBacks.

Hopefully fixed by using a RetainPtr here as well.

https://codereview.chromium.org/879533003/diff/120001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:81: CTFontRef ctFont =
CTFontCreateWithGraphicsFont(cgFont, textSize, 0,
cascadeToLastResortFontDescriptor());
On 2015/02/24 16:13:07, Robert Sesek wrote:
> Create is strong ref. Unless SkCreateTypefaceFromCTFont adopts the reference,
> then this is leaked.

Yes, Skia does not adopt the reference. [1] Wrapped in a RetainPtr.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

[Robert Sesek, 2015-03-19 21:53:45]

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
File Source/platform/fonts/mac/FontPlatformDataMac.mm (right):

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:31: #import
"platform/LayoutTestSupport.h"
Only #import things that are Objective-C. C++ headers should be #include.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:41: CGFontRef cgFont =
CTFontCopyGraphicsFont(toCTFontRef(nsFont), 0);
Copy methods return a strong reference.

https://developer.apple.com/library/mac/documentation/CoreFoundation/Conceptu...

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:43: NSString* fontName =
const_cast<NSString*>(reinterpret_cast<const
NSString*>(CGFontCopyPostScriptName(cgFont)));
Similarly, Copy returns a strong reference.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:43: NSString* fontName =
const_cast<NSString*>(reinterpret_cast<const
NSString*>(CGFontCopyPostScriptName(cgFont)));
Drop the const. No ObjC objects use const.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:53: const void* keys[] = {
kCTFontCascadeListAttribute };
Move this down to above where you declare |values|, since it's not used in the
creation of the descriptor array.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:54:
RetainPtr<CTFontDescriptorRef> descriptorRef(AdoptCF,
CTFontDescriptorCreateWithNameAndSize(CFSTR("LastResort"), 0) );
naming: lastResort ?

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:58: RetainPtr<CFDictionaryRef>
attributes(AdoptCF, CFDictionaryCreate(kCFAllocatorDefault, keys, values,
WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
nit: Can you wrap this line?

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:78: if
(!sandboxSupport->loadFont(nsFont, &cgFont, &fontID)) {
According to the documentation of loadFont(), the caller owns the reference to
the cgFont, so that also needs to be released.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/879533003/diff/140001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/879533003/diff/140001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:82: const NSFont* font =
reinterpret_cast<const NSFont*>(fontPlatformData.ctFont());
NSObjects are never const.

[Dominik Röttsches, 2015-03-20 11:01:07]

Thanks for your review!

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
File Source/platform/fonts/mac/FontPlatformDataMac.mm (right):

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:31: #import
"platform/LayoutTestSupport.h"
On 2015/03/19 21:53:45, Robert Sesek wrote:
> Only #import things that are Objective-C. C++ headers should be #include.

Changed to #include

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:41: CGFontRef cgFont =
CTFontCopyGraphicsFont(toCTFontRef(nsFont), 0);
On 2015/03/19 21:53:45, Robert Sesek wrote:
> Copy methods return a strong reference.

Wrapped in RetainPtr.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:43: NSString* fontName =
const_cast<NSString*>(reinterpret_cast<const
NSString*>(CGFontCopyPostScriptName(cgFont)));
On 2015/03/19 21:53:45, Robert Sesek wrote:
> Drop the const. No ObjC objects use const.

Suggestions welcome, but I do not know any better way to shorten the cast
expression. If I drop the const in the reinterpret_cast template argument, I get
a clang error complaining about reinterpret_cast being unable to cast away const
qualifiers. CFToNSCast in base/mac/foundation_util.mm chains reinterpret_cast
and const_cast as well.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:53: const void* keys[] = {
kCTFontCascadeListAttribute };
On 2015/03/19 21:53:45, Robert Sesek wrote:
> Move this down to above where you declare |values|, since it's not used in the
> creation of the descriptor array.

Moved.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:54:
RetainPtr<CTFontDescriptorRef> descriptorRef(AdoptCF,
CTFontDescriptorCreateWithNameAndSize(CFSTR("LastResort"), 0) );
On 2015/03/19 21:53:45, Robert Sesek wrote:
> naming: lastResort ?

Renamed to lastResort.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:58: RetainPtr<CFDictionaryRef>
attributes(AdoptCF, CFDictionaryCreate(kCFAllocatorDefault, keys, values,
WTF_ARRAY_LENGTH(keys), &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
On 2015/03/19 21:53:45, Robert Sesek wrote:
> nit: Can you wrap this line?

Wrapped this one and similar lines above.

https://codereview.chromium.org/879533003/diff/140001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontPlatformDataMac.mm:78: if
(!sandboxSupport->loadFont(nsFont, &cgFont, &fontID)) {
On 2015/03/19 21:53:45, Robert Sesek wrote:
> According to the documentation of loadFont(), the caller owns the reference to
> the cgFont, so that also needs to be released.

Changed this to pass &loadedCGFont as argument, then, on success, adopting the
result into a RetainPtr<CGFontRef>. There might be a better way to do that,
avoiding the extra CGFontRef declaration - but I did not find such.

https://codereview.chromium.org/879533003/diff/140001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/879533003/diff/140001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:82: const NSFont* font =
reinterpret_cast<const NSFont*>(fontPlatformData.ctFont());
On 2015/03/19 21:53:45, Robert Sesek wrote:
> NSObjects are never const.

Changed to NSFont* - however, as above, the nested cast remains.

[Dominik Röttsches, 2015-03-23 08:51:03]

dominik.rottsches@intel.com changed reviewers:
+ japhet@chromium.org, kbr@chromium.org, pfeldman@chromium.org,
tkent@chromium.org

[Dominik Röttsches, 2015-03-23 08:51:03]

kbr@, japhet@, pfeldman@, tkent@: Could you review the changes in WebStringUtil?
Thanks.

[Ken Russell (switch to Gerrit), 2015-03-23 18:59:00]

WebSubstringUtil.mm LGTM

https://codereview.chromium.org/879533003/diff/180001/Source/web/mac/WebSubst...
File Source/web/mac/WebSubstringUtil.mm (right):

https://codereview.chromium.org/879533003/diff/180001/Source/web/mac/WebSubst...
Source/web/mac/WebSubstringUtil.mm:83: NSFont* font =
const_cast<NSFont*>(reinterpret_cast<const
NSFont*>((fontPlatformData.ctFont())));
Can this use the toNSFont helper from platform/fonts/FontPlatformData.h ?

[Robert Sesek, 2015-03-23 19:18:53]

What are the changes to the TestExpectations? I wouldn't expect to see any from
this change, unless it's small antialiasing changes from using CTFont instead of
CGFont in certain places (which take ever-so-slightly different rendering
paths). Have you manually checked the results?

[Dominik Röttsches, 2015-03-24 13:11:00]

On 2015/03/23 19:18:53, Robert Sesek wrote:
> What are the changes to the TestExpectations? I wouldn't expect to see any
from
> this change, unless it's small antialiasing changes from using CTFont instead
of
> CGFont in certain places (which take ever-so-slightly different rendering
> paths). Have you manually checked the results?

Yes, manually inspected, reasons explained in comment #5: The difference is due
to different fallback behavior: We do fall back based on the loaded web font
now, not based on an empty typeface. Emil kind of LGTM'ed this part in comment
#6. The Atsui* test rebaselines are actually progressions in small caps cases.
Improvement to Canvas 2d text metrics text landed in
https://codereview.chromium.org/950773003/

[Robert Sesek, 2015-03-24 13:50:19]

LGTM

[Dominik Röttsches, 2015-03-24 15:43:23]

The CQ bit was checked by dominik.rottsches@intel.com

[Dominik Röttsches, 2015-03-24 15:43:24]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/879533003/#ps200001
(title: "Canvas text metrics test does not need rebaselining after improvement
landed, toNSFont in WebSubstringUtil.mm")

[commit-bot: I haz the power, 2015-03-24 15:43:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/879533003/200001

[commit-bot: I haz the power, 2015-03-24 16:05:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-24 16:05:30]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[Dominik Röttsches, 2015-03-24 19:43:35]

Filed https://codereview.chromium.org/1034523003/

[Dominik Röttsches, 2015-03-25 07:45:43]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2015-03-25 07:46:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/879533003/200001

[commit-bot: I haz the power, 2015-03-25 07:48:59]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192506

[alph, 2015-03-25 10:39:46]

alph@chromium.org changed reviewers:
+ alph@chromium.org

[alph, 2015-03-25 10:39:47]

FYI: There are some new failures on Mac. Might be related.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds/...

[Julien - ping for review, 2015-03-25 15:32:31]

On 2015/03/25 at 10:39:47, alph wrote:
> FYI: There are some new failures on Mac. Might be related.
> 
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds/...

And this broke the GN Mac builder [1], preventing the rolls from landing. I will
revert so it can be investigated offline.

[1]
https://build.chromium.org/p/chromium.mac/builders/Mac%20GN%20%28dbg%29/build...

[Julien - ping for review, 2015-03-25 15:33:42]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1021483004/ by jchaffraix@chromium.org.

The reason for reverting is: The patch broke the Mac GN builder, preventing the
roll:

ERROR at //third_party/WebKit/Source/platform/BUILD.gn:221:7: Item not found
      "fonts/skia/FontCustomPlatformDataSkia.cpp",

It may have also caused some failures on Mac..

[Julien - ping for review, 2015-03-25 15:53:42]

On 2015/03/25 at 15:33:42, Julien Chaffraix - PST wrote:
> A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1021483004/ by jchaffraix@chromium.org.
> 
> The reason for reverting is: The patch broke the Mac GN builder, preventing
the roll:
> 
> ERROR at //third_party/WebKit/Source/platform/BUILD.gn:221:7: Item not found
>       "fonts/skia/FontCustomPlatformDataSkia.cpp",
> 
> It may have also caused some failures on Mac..

For clarity, Loislo took care of the build break and Dominik followed up on the
Mac failures so the revert was reverted. Sorry for the noise.