Refactor SkFontHost_linux into SkFontMgr_custom.

Refactor SkFontHost_linux into SkFontMgr_custom.

Committed: https://skia.googlesource.com/skia/+/5cf19498f9455e4aa8685a9d7775fefe42deb22b

[bungeman-skia, 2015-06-12 22:49:08]

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

[bungeman-skia, 2015-06-12 22:49:09]

Start of clean up of files around fonts so things are cleaner and make more
sense. This particular change shouldn't affect anyone outside Skia (which is why
I started with it).

[bungeman-skia, 2015-06-12 22:49:15]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-12 22:49:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184923002/40001

[commit-bot: I haz the power, 2015-06-12 22:54:57]

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

[commit-bot: I haz the power, 2015-06-12 22:54:58]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-06-15 15:48:07]

https://codereview.chromium.org/1184923002/diff/40001/include/ports/SkFontMgr...
File include/ports/SkFontMgr_custom.h (right):

https://codereview.chromium.org/1184923002/diff/40001/include/ports/SkFontMgr...
include/ports/SkFontMgr_custom.h:18: struct SkEmbeddedResource { const uint8_t*
data; size_t size; };
This struct stuff seems funny. If it is supposed to be something like what comes
out of a directory iterator (the first API), perhaps we want

class SkFileNameIteratorBaseClass;

SkFontMgr_New_From_Iter(...)

or 

SkFontMgr_New_From_File_List(const char* names[], int count);

?

[bungeman-skia, 2015-06-15 16:15:43]

https://codereview.chromium.org/1184923002/diff/40001/include/ports/SkFontMgr...
File include/ports/SkFontMgr_custom.h (right):

https://codereview.chromium.org/1184923002/diff/40001/include/ports/SkFontMgr...
include/ports/SkFontMgr_custom.h:18: struct SkEmbeddedResource { const uint8_t*
data; size_t size; };
On 2015/06/15 15:48:07, reed1 wrote:
> This struct stuff seems funny. If it is supposed to be something like what
comes
> out of a directory iterator (the first API), perhaps we want
> 
> class SkFileNameIteratorBaseClass;
> 
> SkFontMgr_New_From_Iter(...)
> 
> or 
> 
> SkFontMgr_New_From_File_List(const char* names[], int count);
> 
> ?

No, the entire point is to avoid having a file system or names. This is designed
around providing fonts in the executable as static const data and having the
linker tell us where it is. While it would be interesting to add an abstraction
layer on top of this or have the user embed an entire file system as data and
have them mount that and pass a handle to it, this is mostly designed around
providing a simple means of embedding test fonts.

[reed1, 2015-06-15 17:25:03]

I am still trying to understand how these fit into the layering of your
code-sharing for fontmgrs.

Do both of these quickly devolve into

SkFontMgr_New_With_Typeface_List( SkTypeface* [], int count);

?

It seems like your two helper factories will make a set of typefaces that are
homogenous in storage type (e.g. all backed by files or all backed by raw-ptrs).

Can the rest of the "shared" code (around searching by names, etc.) be
efficiently constructed from an array of typefaces?

[bungeman-skia, 2015-06-15 18:52:30]

Patch Set 4 sidesteps the issue of what this should look like for now by not
exporting the embedded font manager static factory method. This allows building
and testing with the embedded font manager as the default font manager, but the
user for the moment has no API to create an instance themselves. The API can be
decided after the rest of the font manager code has been re-organized.

[reed1, 2015-06-15 20:05:01]

https://codereview.chromium.org/1184923002/diff/60001/src/ports/SkFontMgr_cus...
File src/ports/SkFontMgr_custom_embedded_factory.cpp (right):

https://codereview.chromium.org/1184923002/diff/60001/src/ports/SkFontMgr_cus...
src/ports/SkFontMgr_custom_embedded_factory.cpp:11: struct
SkEmbeddedResourceHeader { const SkEmbeddedResource* entries; int count; };
How does this work, with struct definitions in a .cpp file?

[reed1, 2015-06-15 20:23:49]

So reading the old SkFontHost_linux, we seem to do at least 2 things per-entry:

1. build a typeface wrapping a stream (with explicit style and "realname")
2. group typefaces into families based on realname

Is that correct?

Perhaps that is a lot to have to export to clients with custom fonts sources,
not sure. Seems like SkTypeface_Stream is pretty stable...

[bungeman-skia, 2015-06-15 20:39:39]

https://codereview.chromium.org/1184923002/diff/60001/src/ports/SkFontMgr_cus...
File src/ports/SkFontMgr_custom_embedded_factory.cpp (right):

https://codereview.chromium.org/1184923002/diff/60001/src/ports/SkFontMgr_cus...
src/ports/SkFontMgr_custom_embedded_factory.cpp:11: struct
SkEmbeddedResourceHeader { const SkEmbeddedResource* entries; int count; };
On 2015/06/15 20:05:00, reed1 wrote:
> How does this work, with struct definitions in a .cpp file?

It works the same way any function forward definition works. These types are
just a bit long to write out in full as part of the parameter list. The file
with the implementation also has these declarations in order to simplify the
declaration there as well, which is repetition I suppose. I could change the
declaration to something like

SkFontMgr* SkFontMgr_New_Custom_Embedded(const uint8_t** data, const size_t**
size, int count);

and update the user, avoiding any names or structure, but I rather like the
structure.

I could stick these and the declaration below in their own tiny header in
src/ports, but the aim here was just to split out all of the SkFontMgr::Factory
calls into their own files so that it's clear how to pick them at build time. We
may want SkFontMgr_Custom to be more flexible in the future, but there is no
immediate need.

[reed1, 2015-06-15 20:46:19]

On 2015/06/15 20:39:39, bungeman1 wrote:
>
https://codereview.chromium.org/1184923002/diff/60001/src/ports/SkFontMgr_cus...
> File src/ports/SkFontMgr_custom_embedded_factory.cpp (right):
> 
>
https://codereview.chromium.org/1184923002/diff/60001/src/ports/SkFontMgr_cus...
> src/ports/SkFontMgr_custom_embedded_factory.cpp:11: struct
> SkEmbeddedResourceHeader { const SkEmbeddedResource* entries; int count; };
> On 2015/06/15 20:05:00, reed1 wrote:
> > How does this work, with struct definitions in a .cpp file?
> 
> It works the same way any function forward definition works. These types are
> just a bit long to write out in full as part of the parameter list. The file
> with the implementation also has these declarations in order to simplify the
> declaration there as well, which is repetition I suppose. I could change the
> declaration to something like
> 
> SkFontMgr* SkFontMgr_New_Custom_Embedded(const uint8_t** data, const size_t**
> size, int count);

I think that isn't so bad actually.

> 
> and update the user, avoiding any names or structure, but I rather like the
> structure.
> 
> I could stick these and the declaration below in their own tiny header in
> src/ports, but the aim here was just to split out all of the
SkFontMgr::Factory
> calls into their own files so that it's clear how to pick them at build time.
We
> may want SkFontMgr_Custom to be more flexible in the future, but there is no
> immediate need.

[reed1, 2015-06-15 20:47:49]

Don't want to slow this down anymore, but sometime I would like to understand
some of my previous questions, about what the custom loaders really decompose
into.

lgtm

[bungeman-skia, 2015-06-15 20:50:48]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-15 20:51:06]

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

[commit-bot: I haz the power, 2015-06-15 21:00:11]

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

[commit-bot: I haz the power, 2015-06-15 21:00:12]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2015-06-15 21:43:59]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2015-06-15 21:44:07]

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

[commit-bot: I haz the power, 2015-06-15 21:44:47]

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

[commit-bot: I haz the power, 2015-06-15 21:44:48]

Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[bungeman-skia, 2015-06-15 22:16:30]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2015-06-15 22:16:42]

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

[commit-bot: I haz the power, 2015-06-15 22:17:25]

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