Add a flag indicating locally-created fonts to SkCreateTypefaceFromCTFont

Add a flag indicating locally-created fonts to SkCreateTypefaceFromCTFont

We need to properly indicate whether the typeface was created from
local stream when creating SkTypeface_Mac, otherwise the typeface
won't be serialized. SkCreateTypefaceFromCTFont used to assume
all fonts to be non-local, which caused web fonts to be incorrectly serialized on Mac.

BUG=chromium:426088

[caseq, 2014-10-29 14:52:53]

caseq@chromium.org changed reviewers:
+ bungeman@gmail.com

[caseq, 2014-10-29 14:52:53]

Please take a look -- we've already fixed a similar bug before
(https://codereview.chromium.org/353993003), but it looks like chromium is now
using a different code path (SkCreateTypefaceFromCTFont), where we used to
always consider typeface as a non-local.

[reed1, 2014-10-29 15:36:33]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-10-29 15:36:34]

https://codereview.chromium.org/692553002/diff/1/src/ports/SkFontHost_mac.cpp
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/692553002/diff/1/src/ports/SkFontHost_mac.cpp...
src/ports/SkFontHost_mac.cpp:580: AutoCFRelease<CFTypeRef>
priority(CTFontDescriptorCopyAttribute(fontDescriptor,
kCTFontPriorityAttribute));
nit: 100cols

[bungeman-skia, 2014-10-29 15:38:34]

bungeman@google.com changed reviewers:
+ bungeman@google.com
- bungeman@gmail.com

[caseq, 2014-10-29 15:46:40]

On 2014/10/29 15:36:34, reed1 wrote:
> https://codereview.chromium.org/692553002/diff/1/src/ports/SkFontHost_mac.cpp
> File src/ports/SkFontHost_mac.cpp (right):
> 
>
https://codereview.chromium.org/692553002/diff/1/src/ports/SkFontHost_mac.cpp...
> src/ports/SkFontHost_mac.cpp:580: AutoCFRelease<CFTypeRef>
> priority(CTFontDescriptorCopyAttribute(fontDescriptor,
> kCTFontPriorityAttribute));
> nit: 100cols

Thanks, fixed!

[reed1, 2014-10-29 15:58:49]

Would it be cleaner to just add a parameter to this factory? either a flag, or
perhaps even the storage itself?

[caseq, 2014-10-29 16:12:01]

On 2014/10/29 15:58:49, reed1 wrote:
> Would it be cleaner to just add a parameter to this factory? either a flag, or
> perhaps even the storage itself?

This is also an option, it's just that this is called form chrome, so I was
trying to avoid a two-side patch. Besides, my idea was that if the font is
registered per-process, we're not likely to be able to match it by the
descriptor at the deserialization time, so it may be actually a more robust
thing to check the priority. But if you think this is a hack, I think we can as
well pass this flag explicitly from chrome.

[bungeman-skia, 2014-10-29 17:40:55]

Can you verify that this change doesn't make *all* fonts local on Mac? I don't
remember exactly how everything works, but in the sandbox all fonts on Mac might
be marked as process local.

I'd much prefer a flag to pass in here. The one place this is called from Blink
can easily know if this font is a local font or not.

The Mac font code path has recently gone through a lot of churn to use harfbuzz
and a shared paining path, but it has not yet been refactored to think in terms
of SkTypefaces as the other platforms have. The longer term solution is to get
Blink on Mac to use SkTypeface and remove this method.

https://codereview.chromium.org/692553002/diff/20001/src/ports/SkFontHost_mac...
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/692553002/diff/20001/src/ports/SkFontHost_mac...
src/ports/SkFontHost_mac.cpp:582: if (CFGetTypeID(priority) ==
CFNumberGetTypeID()) {
According to the documentation, the request for kCTFontPriorityAttribute may be
NULL which means it's not a registered font, which seems like it would also make
it 'local'.

https://codereview.chromium.org/692553002/diff/20001/src/ports/SkFontHost_mac...
src/ports/SkFontHost_mac.cpp:583: CFNumberRef priorityNumber =
reinterpret_cast<CFNumberRef>(priority.get());
I believe this can be a static_cast .

[caseq, 2014-10-30 16:57:27]

On 2014/10/29 17:40:55, bungeman1 wrote:
> Can you verify that this change doesn't make *all* fonts local on Mac? I don't
> remember exactly how everything works, but in the sandbox all fonts on Mac
might
> be marked as process local.

Yep, I did verify that and it actually produced plausible results (i.e. only web
fonts had local flag set).

> I'd much prefer a flag to pass in here. The one place this is called from
Blink
> can easily know if this font is a local font or not.

It turned out to be not that simple to figure whether the font is local in that
particular call site,
but then I realized that for custom fonts, we actually already create them with
SkTypeface::CreateFromStream(),
so it looks like we can just use the resulting value instead of trying to create
another typeface form CTFont.

Thanks for the reviews, but I guess we don't even need the flag for
SkCreateTypefaceFromCTFont() with https://codereview.chromium.org/691733004, so
I'm closing this one.

[caseq, 2014-10-31 13:08:28]

Patchset #3 (id:40001) has been deleted

[caseq, 2014-10-31 13:09:39]

Reopened, ptal. As you were suggesting earlier, I'm just adding a bool to flag
to SkCreateTypefaceFromCTFont().

[caseq, 2014-11-06 22:54:06]

Closing once again as we decided to go with
https://codereview.chromium.org/691733004/.