add runtime config option to retain stream for custom typefaces

add runtime config option to retain stream for custom typefaces

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/ad5e9a53270b96dcbfc9aa678c91157407058449

[humper, 2014-11-17 21:31:10]

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

[humper, 2014-11-17 21:31:15]

ptal

[humper, 2014-11-18 20:45:53]

On 2014/11/17 21:31:15, humper wrote:
> ptal

ping

[reed1, 2014-11-19 13:58:11]

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

[reed1, 2014-11-19 13:58:17]

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

https://codereview.chromium.org/729973004/diff/1/src/ports/SkFontHost_linux.c...
src/ports/SkFontHost_linux.cpp:122: fStream = SkStream::NewFromFile();
do we need to pass (path) as a parameter?

https://codereview.chromium.org/729973004/diff/1/src/ports/SkFontHost_linux.c...
src/ports/SkFontHost_linux.cpp:137: if (c_CustomTypefaceRetain) {
Can this predicate be changed to just

if (already have fStream)
    dupulicate
else
   ...

[reed1, 2014-11-19 14:00:29]

meta: could we also support the feature by just passing some flag to the factory
for the FontMgr (e.g. read-all-fonts-into-ram)? I just find runtime signals like
this hard to see when we examine the code-flow in general (basically its
globals, and those are hard to remember).

[humper, 2014-11-19 14:02:45]

On 2014/11/19 14:00:29, reed1 wrote:
> meta: could we also support the feature by just passing some flag to the
factory
> for the FontMgr (e.g. read-all-fonts-into-ram)? I just find runtime signals
like
> this hard to see when we examine the code-flow in general (basically its
> globals, and those are hard to remember).

It's possible -- currently this is all primed by a single call to
FontMgr::RefDefault(); I don't think that all font managers have a notion of
fonts-on-disk, so we'd have to add a boolean parameter to that function and make
it default to false everywhere.

I don't think the custom font manager is used anywhere unless you build with
skia_no_fontconfig; does anything except fiddle do that?

[humper, 2014-11-19 14:02:53]

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

https://codereview.chromium.org/729973004/diff/1/src/ports/SkFontHost_linux.c...
src/ports/SkFontHost_linux.cpp:122: fStream = SkStream::NewFromFile();
On 2014/11/19 13:58:16, reed1 wrote:
> do we need to pass (path) as a parameter?

:) oops, fixing.

https://codereview.chromium.org/729973004/diff/1/src/ports/SkFontHost_linux.c...
src/ports/SkFontHost_linux.cpp:137: if (c_CustomTypefaceRetain) {
On 2014/11/19 13:58:16, reed1 wrote:
> Can this predicate be changed to just
> 
> if (already have fStream)
>     dupulicate
> else
>    ...

sure, changing.

[bungeman-skia, 2014-11-19 15:02:50]

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

[bungeman-skia, 2014-11-19 15:14:14]

https://codereview.chromium.org/729973004/diff/20001/src/ports/SkFontHost_lin...
File src/ports/SkFontHost_linux.cpp (right):

https://codereview.chromium.org/729973004/diff/20001/src/ports/SkFontHost_lin...
src/ports/SkFontHost_linux.cpp:119: , fStream(NULL)
I prefer the form

, fStream(c_CustomTypefaceRetain ? SkStream::NewFromFile(fPath.c_str()) : NULL)

as this means the declaration below can be

const SkAutoTUnref<SkStreamAsset> fStream;

making it clear that we won't try to switch streams at any point.

https://codereview.chromium.org/729973004/diff/20001/src/ports/SkFontHost_lin...
src/ports/SkFontHost_linux.cpp:137: if (NULL != fStream) {
I think we're just using

if (fStream.get()) {

these days?

https://codereview.chromium.org/729973004/diff/20001/src/ports/SkFontHost_lin...
src/ports/SkFontHost_linux.cpp:146: SkAutoTUnref<SkStream> fStream;
Above (in SkTypeface_Stream) this is

const SkAutoTUnref<SkStream> fStream;

but that's just because reasons. Let's make this

const SkAutoTUnref<SkStreamAsset> fStream;

so we don't perpetuate the silliness. Sooner or later we'll be able to clean up
the one in SkTypeface_Stream.

[humper, 2014-11-19 15:45:58]

On 2014/11/19 15:14:14, bungeman1 wrote:
>
https://codereview.chromium.org/729973004/diff/20001/src/ports/SkFontHost_lin...
> File src/ports/SkFontHost_linux.cpp (right):
> 
>
https://codereview.chromium.org/729973004/diff/20001/src/ports/SkFontHost_lin...
> src/ports/SkFontHost_linux.cpp:119: , fStream(NULL)
> I prefer the form
> 
> , fStream(c_CustomTypefaceRetain ? SkStream::NewFromFile(fPath.c_str()) :
NULL)
> 
> as this means the declaration below can be
> 
> const SkAutoTUnref<SkStreamAsset> fStream;
> 
> making it clear that we won't try to switch streams at any point.
> 
>
https://codereview.chromium.org/729973004/diff/20001/src/ports/SkFontHost_lin...
> src/ports/SkFontHost_linux.cpp:137: if (NULL != fStream) {
> I think we're just using
> 
> if (fStream.get()) {
> 
> these days?
> 
>
https://codereview.chromium.org/729973004/diff/20001/src/ports/SkFontHost_lin...
> src/ports/SkFontHost_linux.cpp:146: SkAutoTUnref<SkStream> fStream;
> Above (in SkTypeface_Stream) this is
> 
> const SkAutoTUnref<SkStream> fStream;
> 
> but that's just because reasons. Let's make this
> 
> const SkAutoTUnref<SkStreamAsset> fStream;
> 
> so we don't perpetuate the silliness. Sooner or later we'll be able to clean
up
> the one in SkTypeface_Stream.

All done

[bungeman-skia, 2014-11-19 16:22:26]

lgtm

[humper, 2014-11-19 16:22:51]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-11-19 16:23:16]

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

[commit-bot: I haz the power, 2014-11-19 16:32:29]

Committed patchset #3 (id:20002) as
https://skia.googlesource.com/skia/+/ad5e9a53270b96dcbfc9aa678c91157407058449