move most of SkFontHost to private (preceeding making it all private)

move most of SkFontHost to private (preceeding making it all private)

In this change, have to accomodate PDF wanting to call openStream and advancedMetrics
Committed: https://code.google.com/p/skia/source/detail?r=8156

[reed1, 2013-03-12 20:11:08]

grump -- don't really like this. suggestions?

[bungeman-skia, 2013-03-12 21:07:23]

Some suggestions.

https://codereview.chromium.org/12739006/diff/1/trunk/include/core/SkTypeface.h
File trunk/include/core/SkTypeface.h (right):

https://codereview.chromium.org/12739006/diff/1/trunk/include/core/SkTypeface...
trunk/include/core/SkTypeface.h:87: static SkTypeface* RefDefault();
So all of these creates on Typeface will be shorthand to the 'default' font
configurator?

https://codereview.chromium.org/12739006/diff/1/trunk/src/core/SkTypeface.cpp
File trunk/src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/12739006/diff/1/trunk/src/core/SkTypeface.cpp...
trunk/src/core/SkTypeface.cpp:53: SkTypeface* face = GetDefaultTypeface();
Any reason not to

return SkRef(GetDefaultTypeface());

https://codereview.chromium.org/12739006/diff/1/trunk/src/core/SkTypeface.cpp...
trunk/src/core/SkTypeface.cpp:133: (void)SkFontHost::GetFileName(fUniqueID,
NULL, 0, &ndx);
Still using that hack, are we :-)

https://codereview.chromium.org/12739006/diff/1/trunk/src/pdf/SkPDFFont.cpp
File trunk/src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/12739006/diff/1/trunk/src/pdf/SkPDFFont.cpp#n...
trunk/src/pdf/SkPDFFont.cpp:792: SkAutoDefaultTypeface autoFace(typeface);
Is this less verbose and achieve the same ends?

SkAutoTUnref<SkTypeface> autoTypeface;
if (NULL == typeface) {
    autoTypeface.reset(SkTypeface::RefDefault())
    typeface = autoTypeface.get();
}

https://codereview.chromium.org/12739006/diff/1/trunk/src/pdf/SkPDFFont.cpp#n...
trunk/src/pdf/SkPDFFont.cpp:886: fTypeface(typeface),
Because fTypeface is an SkAutoTUnref<SkTypeface> this change should technically
be written

fTypeface(typeface ? SkRef(typeface) : SkTypeface::RefDefault()),

because Auto things should be written so that they do the right thing should
there be a failure latter in the initializer list. Another reason is that it's a
bit shorter.

https://codereview.chromium.org/12739006/diff/1/trunk/src/pdf/SkPDFFont.cpp#n...
trunk/src/pdf/SkPDFFont.cpp:889: fFontInfo(info),
For the same reason this should have been

fFontInfo(SkSafeRef(info))

and remove the ref in the body, since this is an
SkAutoTUnref<SkAdvancedTypefaceMetrics>.

[reed1, 2013-03-13 21:18:17]

https://codereview.chromium.org/12739006/diff/1/trunk/include/core/SkTypeface.h
File trunk/include/core/SkTypeface.h (right):

https://codereview.chromium.org/12739006/diff/1/trunk/include/core/SkTypeface...
trunk/include/core/SkTypeface.h:87: static SkTypeface* RefDefault();
On 2013/03/12 21:07:23, bungeman1 wrote:
> So all of these creates on Typeface will be shorthand to the 'default' font
> configurator?

Yes, I think any runtime-configurator (fontmgr) we want to invent is sort of
orthogonal to this CL, which is trying to add enough apis to allow fonthost (as
written today) to be nearly private.

https://codereview.chromium.org/12739006/diff/1/trunk/src/core/SkTypeface.cpp
File trunk/src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/12739006/diff/1/trunk/src/core/SkTypeface.cpp...
trunk/src/core/SkTypeface.cpp:53: SkTypeface* face = GetDefaultTypeface();
On 2013/03/12 21:07:23, bungeman1 wrote:
> Any reason not to
> 
> return SkRef(GetDefaultTypeface());

Done.

https://codereview.chromium.org/12739006/diff/1/trunk/src/core/SkTypeface.cpp...
trunk/src/core/SkTypeface.cpp:133: (void)SkFontHost::GetFileName(fUniqueID,
NULL, 0, &ndx);
On 2013/03/12 21:07:23, bungeman1 wrote:
> Still using that hack, are we :-)

Only temporarily. Once I can land this, I can start to kill off large parts of
skfonthost, including OpenStream/GetFileName, and just have freetype call
face->openStream().

https://codereview.chromium.org/12739006/diff/1/trunk/src/pdf/SkPDFFont.cpp
File trunk/src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/12739006/diff/1/trunk/src/pdf/SkPDFFont.cpp#n...
trunk/src/pdf/SkPDFFont.cpp:886: fTypeface(typeface),
On 2013/03/12 21:07:23, bungeman1 wrote:
> Because fTypeface is an SkAutoTUnref<SkTypeface> this change should
technically
> be written
> 
> fTypeface(typeface ? SkRef(typeface) : SkTypeface::RefDefault()),
> 
> because Auto things should be written so that they do the right thing should
> there be a failure latter in the initializer list. Another reason is that it's
a
> bit shorter.

Done.

[bungeman-skia, 2013-03-13 21:33:25]

Aside from the one comment lgtm.

https://codereview.chromium.org/12739006/diff/14001/trunk/src/pdf/SkPDFFont.cpp
File trunk/src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/12739006/diff/14001/trunk/src/pdf/SkPDFFont.c...
trunk/src/pdf/SkPDFFont.cpp:52: SkTypeface* operator->() { return fFace; }
Doe we want operator-> and operator SkTypeface* ? These aren't currently used
and it would be nice to avoid yet another smart pointer like thing.

[reed1, 2013-03-13 22:09:09]

- inherit from SkAutoTUnref
- rearrange SkFontHost.h to put all public apis on top

[reed1, 2013-03-13 22:11:27]

No so secret goal:

- reduce SkFontHost to just LCD setters/getters (and then move into chrome)
- push all non-factory functions onto SkTypeface instances, so that subclasses
are responsible
- eventually support multiple FontMgrs, which will take over the factory
functions (ala SkFontConfigInterface)

-- support multiple font scalers at runtime (e.g. directwrite and gdi)

[bungeman-skia, 2013-03-14 03:47:34]

https://codereview.chromium.org/12739006/diff/19009/trunk/tests/FontHostStrea...
File trunk/tests/FontHostStreamTest.cpp (right):

https://codereview.chromium.org/12739006/diff/19009/trunk/tests/FontHostStrea...
trunk/tests/FontHostStreamTest.cpp:96: origTypeface =
aur.reset(SkTypeface::RefDefault());
I see that you checked in https://code.google.com/p/skia/source/detail?r=8141 to
make what I suggested in
https://codereview.chromium.org/12739006/diff/1/trunk/src/pdf/SkPDFFont.cpp#n...
shorter (sneaky). However, you're doing this here, why not do it in SkPDFFont
where I suggested it and drop SkTypefacePriv.h?

[reed1, 2013-03-14 14:22:42]

https://codereview.chromium.org/12739006/diff/19009/trunk/tests/FontHostStrea...
File trunk/tests/FontHostStreamTest.cpp (right):

https://codereview.chromium.org/12739006/diff/19009/trunk/tests/FontHostStrea...
trunk/tests/FontHostStreamTest.cpp:96: origTypeface =
aur.reset(SkTypeface::RefDefault());
On 2013/03/14 03:47:34, bungeman1 wrote:
> I see that you checked in https://code.google.com/p/skia/source/detail?r=8141
to
> make what I suggested in
>
https://codereview.chromium.org/12739006/diff/1/trunk/src/pdf/SkPDFFont.cpp#n...
> shorter (sneaky). However, you're doing this here, why not do it in SkPDFFont
> where I suggested it and drop SkTypefacePriv.h?

I'm not sure which pattern I like better yet, so I'm trying each. I anticipate
we will need other places to handle null-typefaces as I push more and more
fonthost changes (e.g. adding typeface* to more apis instead of just fontIDs) so
I thought I'd create the priv.h preemptively (which is private after all). When
the dust settles may be the better time to consolidate on a common pattern...

[reed1, 2013-03-14 15:05:02]

Committed patchset #5 manually as r8156 (presubmit successful).