Serialize the font index.

Serialize the font index.

BUG=skia:1186
Committed: https://skia.googlesource.com/skia/+/d71b75757335393d9643a5b7a0f2769b6ba52fb6

[bungeman-skia, 2014-09-12 19:30:31]

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

[bungeman-skia, 2014-09-12 19:30:31]

This works, but I want to put it out for review to get feedback on the general
approach.

[bungeman-skia, 2014-09-12 22:22:29]

On 2014/09/12 19:30:31, bungeman1 wrote:
> This works, but I want to put it out for review to get feedback on the general
> approach.

Note that (at PS2 with test) on FontConfig, Android, and DirectWrite this is
actually working. Currently GDI and CT/CG backends don't really support TTCs
when creating from streams, and also don't really support it in serialization at
all. These two APIs go out of their way to make things difficult, but we'll
eventually need to fix these and support indexed fonts fully.

[bungeman-skia, 2014-09-15 15:52:07]

https://codereview.chromium.org/567013002/diff/40001/src/core/SkFontDescripto...
File src/core/SkFontDescriptor.cpp (right):

https://codereview.chromium.org/567013002/diff/40001/src/core/SkFontDescripto...
src/core/SkFontDescriptor.cpp:24:
SkFontDescriptor::SkFontDescriptor(SkTypeface::Style style) : fFontFileIndex(0),
fStyle(style) { }
So I took a look into an 'unknown' or 'isn't a collection' value. As a survey,
DirectWrite has IDWriteFontFace::GetIndex() which will return 0 if the font file
contains a single face in all cases. FontConfig will store 0 for the FC_INDEX
for any such font. This is mostly because FreeType always treats the first face
in a file as index 0. As a result, our Android back-end behaves this way. GDI
and CT/CG are basically collection antagonistic at the API layer, and won't tell
you what the index is.

In a certain sense, all font files are collections with at least a '0' entry,
and all of our back-ends treat the situation this way. In general we would have
to write extra code to distinguish between these cases, and we would also have
to write yet more extra code to handle this extra case. For all this extra code
it does not appear that we receive any benefit. For consistency with existing
practice, and to reduce superfluous code, I recommend sticking with '0' instead
of something like '-1' for non-collection font file formats.

Regarding setting the fFontFileIndex to '-1' only if fFontFileName is left empty
and fFontFileIndex is not set. This would imply some odd behavior like setting
the fFontFileIndex to '-1' in the constructor and then in setFontFileName
setting fFontFileIndex to '0' if it is currently '-1'. On the other side when
reading this information what do we do with it? If fFontFileName is
inaccessible, we won't be using fFontFileIndex anyway; I can't imagine anyone
would be checking fFontFileIndex as some sort of signal as to the validity of
fFontFileName. It is easier to see the converse, that a non-empty and valid
fFontFileName implies a valid fFontFileIndex.

Much of this seems to stem from the fact that the font stream (when needed) is
separate from this descriptor. This implies we need the index in two places, one
for the file name and one for the stream. Perhaps the correct answer to this is
to get rid of the 'isLocal' from onGetFontDescriptor and add a setStream to
SkFontDescriptor.

[bungeman-skia, 2014-09-18 14:50:10]

Moved things around to make things make sense (from a certain point of view).

https://codereview.chromium.org/567013002/diff/60001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/567013002/diff/60001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:237: operator T*() const { return fObj; }
mtklein and I have stated a couple of times that this should actually be const,
since this doesn't mutate 'the pointer'. It hasn't been done before because we
haven't needed it and it seems so trivial to do on its own.

https://codereview.chromium.org/567013002/diff/60001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (left):

https://codereview.chromium.org/567013002/diff/60001/src/core/SkPaint.cpp#old...
src/core/SkPaint.cpp:2315: return SkFontDescriptor(istream);
This required a copy constructor, and that seems awkward. Since this is the only
place this was needed, I just re-inlined it. Making this code an actual
SkFontDescriptor constructor would fix this. (Would you like me to add that
here?)

[reed1, 2014-09-18 17:24:46]

lgtm

[bungeman-skia, 2014-09-18 17:27:26]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2014-09-18 17:28:56]

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

[bungeman-skia, 2014-09-18 17:53:32]

The CQ bit was unchecked by bungeman@google.com

[bungeman-skia, 2014-09-18 17:54:20]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2014-09-18 17:55:16]

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

[commit-bot: I haz the power, 2014-09-18 17:55:36]

Committed patchset #4 (id:60001) as d71b75757335393d9643a5b7a0f2769b6ba52fb6