Expose method to retrieve CTFont

Expose method to retrieve CTFont

BUG=skia:3351
R=bungeman@chromium.org

Committed: https://skia.googlesource.com/skia/+/94886482ffe10e17c62cfd274de1c4f4a7393384

[Dominik Röttsches, 2015-01-26 18:06:05]

dominik.rottsches@intel.com changed reviewers:
+ behdad@chromium.org, bsalomon@google.com, bungeman@gmail.com,
eae@chromium.org, reed@google.com

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

reed@google.com: Please review changes in 

bsalomon@google.com: Please review changes in

[bungeman-skia, 2015-01-26 18:08:12]

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

[reed1, 2015-01-26 18:08:32]

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

[reed1, 2015-01-26 18:08:33]

Perhaps document that the return ctfont is only valid as long as the typeface
object is in scope...

defer to Ben for final say on api

[Dominik Röttsches, 2015-01-26 18:20:33]

Thanks for your review.

On 2015/01/26 18:08:33, reed1 wrote:
> Perhaps document that the return ctfont is only valid as long as the typeface
> object is in scope...

IIUC, the CTFontRef is valid as long as the SkTypeface's refcount doesn't drop
to zero, right?

> defer to Ben for final say on api

[tomhudson, 2015-01-26 18:22:09]

On 2015/01/26 18:20:33, Dominik Röttsches wrote:
> Thanks for your review.
> 
> On 2015/01/26 18:08:33, reed1 wrote:
> > Perhaps document that the return ctfont is only valid as long as the
typeface
> > object is in scope...
> 
> IIUC, the CTFontRef is valid as long as the SkTypeface's refcount doesn't drop
> to zero, right?

But you can't know that the refcount hasn't dropped to zero except by keeping it
in scope / nonzero yourself?

[reed1, 2015-01-26 18:25:19]

We can either promise that the ctfont will be valid while its "parent" typeface
is in scope, or we can change the API to actually "ref" the ctfont before return
it (and rename the call to SkTypeface_RefCTFont(face)".

[Dominik Röttsches, 2015-01-26 18:30:10]

> But you can't know that the refcount hasn't dropped to zero except by keeping
it
> in scope / nonzero yourself?

Yes, that's fine in the way that Blink would use it. I'll keep a
RefPtr<SkTypeface> around, and just occasionally need to access the CTFont
internals of it, while doing text shaping of this live-object.

> We can either promise that the ctfont will be valid while its "parent"
typeface
> is in scope, or we can change the API to actually "ref" the ctfont before
return
> it (and rename the call to SkTypeface_RefCTFont(face)".

I think the former works, no need for an extra ref on the CTFont.

[bungeman-skia, 2015-01-26 18:32:48]

On 2015/01/26 18:08:33, reed1 wrote:
> Perhaps document that the return ctfont is only valid as long as the typeface
> object is in scope...
> 
> defer to Ben for final say on api

The reason for requesting this CTFont is to pass it to HarfBuzz, due to HarfBuzz
not directly supporting AAT. If HarfBuzz did support AAT natively, there would
be no need for this change, so in some sense this is a 'temporary' request.

As far as API goes, we need to be removing these sorts of things for good, and
if still needed replacing them with methods on the sub classes of SkFontMgr and
SkTypeface. The reason why these functions exist the way they do was because in
the past it was only possible to have one SkFontHost, and you just compiled the
right one in, so one could get away with these global 'do something magic with
this typeface' since there could be only one kind of typeface. In the new world,
the casts performed internally by these magic functions may not be valid, which
is why this one is currently hidden.

The long term place to put this sort of function, should we wish to support it,
is methods on the SkFontMgr or SkTypeface subclasses. Then, Blink would use
these subclasses. The former is fairly simple to fix (especially in this limited
context), the latter seems more difficult (essentially fix the FIXME at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
). This latter fix is needed for other reasons on other platforms as well,
especially Android testing.

Also, I've been putting off this sort of change since it's directly blocking
further work on Mac text in Blink. The change to use HarfBuzz in M40 created
regressions which have taken until M42 to fix, and not all of the changed test
results have even yet been evaluated or resolved. I'll have to take a look and
see if we've really stabilized enough for another round of churn.

So maybe I'll accept something like this now, but with this function marked as
deprecated and a large warning that this is a fragile hack specifically for
Blink to use HarfBuzz on Mac with AAT. I'll have to make it a priority to clean
these functions up for good, now that I'm currently migrating APIs.

[Dominik Röttsches, 2015-01-26 18:32:50]

"while doing text shaping of this live-object."

s/of/using/

[reed1, 2015-01-26 18:33:30]

I think clients could live with either. My (implied) question is really directed
at Ben. Doing the former locks us into never being able to change the underlying
CTFont obj once the typeface is created. This restriction may be fine, but it is
not a requirement today, so we should add such an API with that
future-constraint in mind.

[Dominik Röttsches, 2015-01-26 18:37:45]

> So maybe I'll accept something like this now, but with this function marked as
> deprecated and a large warning that this is a fragile hack specifically for
> Blink to use HarfBuzz on Mac with AAT. I'll have to make it a priority to
clean
> these functions up for good, now that I'm currently migrating APIs.

That'd be great if you could do that. I am happy to discuss and work with you on
removing this in the future, or moving it to the right place. One way to do this
is to implement native AAT shaping in HarfBuzz with support for retrieving all
required tables from Skia. We could also compile this only when using Skia in
Blink, for example. Thanks.

[behdad_google, 2015-01-26 18:52:07]

On 2015/01/26 18:37:45, Dominik Röttsches wrote:
> > So maybe I'll accept something like this now, but with this function marked
as
> > deprecated and a large warning that this is a fragile hack specifically for
> > Blink to use HarfBuzz on Mac with AAT. I'll have to make it a priority to
> clean
> > these functions up for good, now that I'm currently migrating APIs.
> 
> That'd be great if you could do that. I am happy to discuss and work with you
on
> removing this in the future, or moving it to the right place. One way to do
this
> is to implement native AAT shaping in HarfBuzz with support for retrieving all
> required tables from Skia. We could also compile this only when using Skia in
> Blink, for example. Thanks.

[behdad_google, 2015-01-26 18:52:50]

On 2015/01/26 18:52:07, behdad_google wrote:
> On 2015/01/26 18:37:45, Dominik Röttsches wrote:
> > > So maybe I'll accept something like this now, but with this function
marked
> as
> > > deprecated and a large warning that this is a fragile hack specifically
for
> > > Blink to use HarfBuzz on Mac with AAT. I'll have to make it a priority to
> > clean
> > > these functions up for good, now that I'm currently migrating APIs.
> > 
> > That'd be great if you could do that. I am happy to discuss and work with
you
> on
> > removing this in the future, or moving it to the right place. One way to do
> this
> > is to implement native AAT shaping in HarfBuzz with support for retrieving
all
> > required tables from Skia. We could also compile this only when using Skia
in
> > Blink, for example. Thanks.

The plan indeed is to implement AAT natively in HarfBuzz.  I'll get the ball
rolling on that one.  Mid-year would be a good estimate of when it will be ready
I would say.

[Dominik Röttsches, 2015-02-03 14:20:38]

Ben, could we please land this? I'd like to proceed with the Blink changes
removing native font handles. (Work started in
https://codereview.chromium.org/879533003/ )

[djsollen, 2015-02-03 19:52:12]

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

[djsollen, 2015-02-03 19:52:12]

Ben, can you provide the bug number for the work to move everything off of these
global functions into the SkFontMgr subclasses?  Do you have an ETA on when
Chrome will be in position to use SkFontMgr for things like this?

Dominik, I propose that you document that this function is temporary and note
the bug (for Ben's SkFontMgr work) that will make this call site obsolete. 
Further, if the Mac SkFontMgr subclass is ready to go in Chrome (or has a very
short ETA) then we should add this functionality there.  Otherwise, I don't see
any reason not to land it with the appropriate warning comments.

[Dominik Röttsches, 2015-02-09 13:43:18]

> Dominik, I propose that you document that this function is temporary and note
> the bug (for Ben's SkFontMgr work) that will make this call site obsolete. 
> Further, if the Mac SkFontMgr subclass is ready to go in Chrome (or has a very
> short ETA) then we should add this functionality there.  Otherwise, I don't
see
> any reason not to land it with the appropriate warning comments.

I created https://code.google.com/p/skia/issues/detail?id=3408 to track this
issue, and updated the CL to include a comment mentioning deprecation and
referring to the bug.

[Dominik Röttsches, 2015-02-10 14:15:40]

Are there any other open concerns? If not, can I ask a Skia committer to land
this? Thank you!

[reed1, 2015-02-10 14:45:52]

I am presuming Ben is ok with the runtime semantics of this. Therefore please
update the dox to state the life-time of the returned ref: It is only valid as
long as the typeface is valid.

[bungeman-skia, 2015-02-10 16:10:43]

On 2015/02/10 14:45:52, reed1 wrote:
> I am presuming Ben is ok with the runtime semantics of this. Therefore please
> update the dox to state the life-time of the returned ref: It is only valid as
> long as the typeface is valid.

Is there a plan around getNSFont() at
https://codereview.chromium.org/879533003/diff/40001/Source/platform/fonts/Si...
. It's one thing to have this exposed for temporary use while HarfBuzz gets
worked on, but it seems you're also going to have to use this to implement
getNSFont as well. The NSFont escapes and it isn't clear to me exactly what the
lifetime even is. (I got down to where it ended up as an attribute on an
attributed string, but I haven't gone much beyond that yet).

[d.roettsches, 2015-02-11 09:16:34]

> I am presuming Ben is ok with the runtime semantics of this. Therefore please
> update the dox to state the life-time of the returned ref: It is only valid as
> long as the typeface is valid.

I understand it is released when the typeface is destroyed, documentation
updated accordingly. 

> Is there a plan around getNSFont() at
>
https://codereview.chromium.org/879533003/diff/40001/Source/platform/fonts/Si...
> . It's one thing to have this exposed for temporary use while HarfBuzz gets
> worked on, but it seems you're also going to have to use this to implement
> getNSFont as well. The NSFont escapes and it isn't clear to me exactly what
the
> lifetime even is. (I got down to where it ended up as an attribute on an
> attributed string, but I haven't gone much beyond that yet).

Yes, I have faced this additional problem and one more in font fallback, where
the starting point to look for a fallback font is the original NSFont* (I am
experimenting with using CoreText API instead of AppKit, but still at least a
CTFontRef, instead of an NSFont*, will be needed.) The long term alternative is
to have SkFontMgr help with fallback, I suppose. I updated the comment regarding
legacy use cases.

The functionality which you pointed at is an attributed string which is used for
copying selected text of the page as formatted text to the clipboard, a kind of
snapshot.
A crude alternative/workaround here would be to instantiate a new NSFont after
extracting the font description from the SkTypeface. 

But regarding your lifetime concern in this case: The attributed string is not
meant for internal use inside Blink. It seems quite straightforward to me that
in this case we need to interface with the clipboard using native font types.
I'll check whether an additional retain call is needed or whether the clipboard
API takes care of that.

Would this address your concerns?

[reed1, 2015-02-11 13:45:13]

https://codereview.chromium.org/872963003/diff/60001/include/ports/SkTypeface...
File include/ports/SkTypeface_mac.h (right):

https://codereview.chromium.org/872963003/diff/60001/include/ports/SkTypeface...
include/ports/SkTypeface_mac.h:31: *  released when the source SkTypeface is
destroyed.
Thanks.

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

Pinging again. Ben, could you please land this? This would help enormously with
removing redundant code from FontPlatformData and reducing platform specific
code in Blink, which in turn helps us further optimize those parts of layout
that depend on data from the font. Progress on
https://codereview.chromium.org/879533003/ is good, and I'd like to run it
through the try bots and put it up for review soon. Thank you!

[reed1, 2015-02-18 14:44:45]

The CQ bit was checked by reed@google.com

[reed1, 2015-02-18 14:44:46]

lgtm

[commit-bot: I haz the power, 2015-02-18 14:45:47]

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

[commit-bot: I haz the power, 2015-02-18 18:51:03]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/94886482ffe10e17c62cfd274de1c4f4a7393384