Font variations.

Font variations.

Multiple Master and TrueType fonts support variation axes.
This implements back-end support for axes on platforms which
support it.

Committed: https://skia.googlesource.com/skia/+/05773ed30920c0214d1433c07cf6360a05476c97

Committed: https://skia.googlesource.com/skia/+/3489ee0f4fa34f124f9de090d12bdc2107d52aa9

Committed: https://skia.googlesource.com/skia/+/41868fe5625fc3bd70daa3f461c881b5db6a9265

[bungeman-skia, 2015-03-23 21:59:04]

bungeman@google.com changed reviewers:
+ djsollen@google.com, mtklein@google.com, reed@google.com

[bungeman-skia, 2015-03-23 21:59:05]

At PS1 this is kind of a rough design document style implementation of Multiple
Master / GX Var font support (specifically for Android). At this point, just let
me know if there is anything horribly wrong with the approach or have any
comments on the direction this should take.

At the moment the idea is to more or less keep most of this hidden, just making
things work internally so we can toy with this.

https://codereview.chromium.org/1027373002/diff/1/gm/fontscaler.cpp
File gm/fontscaler.cpp (right):

https://codereview.chromium.org/1027373002/diff/1/gm/fontscaler.cpp#newcode61
gm/fontscaler.cpp:61: /* Added the following to /system/etc/fonts.xml
This is just a hacked-up example to show that it works.

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

https://codereview.chromium.org/1027373002/diff/1/include/core/SkTypeface.h#n...
include/core/SkTypeface.h:285: SkStreamAsset* openStream2(SkFontParameters*
params) const;
Passing around pointers means that 'NULL' is ambiguous. In any event, this is a
working name.

https://codereview.chromium.org/1027373002/diff/1/include/core/SkTypeface.h#n...
include/core/SkTypeface.h:312: static SkTypeface*
CreateFromStream(SkStreamAsset* stream, const SkFontParameters& params);
Initially hiding this so no one other than serialization/deserialization can use
it.

https://codereview.chromium.org/1027373002/diff/1/src/core/SkFontDescriptor.h
File src/core/SkFontDescriptor.h (right):

https://codereview.chromium.org/1027373002/diff/1/src/core/SkFontDescriptor.h...
src/core/SkFontDescriptor.h:15: class SkFontParameters {
So this class is pragmatic, but also represents the odd rift (which FontConfig
ignores) between font request properties (properties of a font request like
name, style, characters, language), and extended font properties (information in
addition to the font data which is needed to define the font like index, axis,
extra scale, extra matrix, synthetic attributes).

The SkFontDescriptor wants to be able to represent both, so it almost seems like
it wants to be two parts. The SkTypeface base class knows everything about the
font request, but it's up to the subclasses to know about font parameters. The
font paramaters would also always need to ride with the data, so maybe this
should also have the data on it and the 'openStream' return one of these as a
whole package.

The implementation of this is also currently an ugly hack, just to get things
working. Needs to be cleaned up.

https://codereview.chromium.org/1027373002/diff/1/src/core/SkFontMgr.cpp
File src/core/SkFontMgr.cpp (right):

https://codereview.chromium.org/1027373002/diff/1/src/core/SkFontMgr.cpp#newc...
src/core/SkFontMgr.cpp:144: SkTypeface*
SkFontMgr::onCreateFromStream(SkStreamAsset* stream, const SkFontParameters&
params) const {
These are just the base 'default' shims to keep existing SkFontMgrs working.

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

https://codereview.chromium.org/1027373002/diff/1/src/core/SkTypeface.cpp#new...
src/core/SkTypeface.cpp:228: SkStreamAsset*
SkTypeface::onOpenStream2(SkFontParameters* fontParams) const {
These are just the base 'default' shims to keep existing SkTypefaces working.

https://codereview.chromium.org/1027373002/diff/1/src/ports/SkFontMgr_fontcon...
File src/ports/SkFontMgr_fontconfig.cpp (right):

https://codereview.chromium.org/1027373002/diff/1/src/ports/SkFontMgr_fontcon...
src/ports/SkFontMgr_fontconfig.cpp:399: SkStreamAsset*
onOpenStream2(SkFontParameters* fontParams) const SK_OVERRIDE {
Temporary implementation to just muck about with this on the desktop (where it's
faster to iterate). Unfortunately, FontConfig doesn't really support axes at
all.

[bungeman-skia, 2015-04-01 20:38:29]

Some more notes to myself as I self review the intermediates.

https://codereview.chromium.org/1027373002/diff/20001/include/core/SkTypeface.h
File include/core/SkTypeface.h (right):

https://codereview.chromium.org/1027373002/diff/20001/include/core/SkTypeface...
include/core/SkTypeface.h:285: SkFontData* createFontData() const;
Maybe this guy should take an SkFontData* so he isn't required to new one up. I
think we also want this to be able to fail. The stream is still going to be
new'ed up though, but that was true before as well. Of course, that means making
SkFontData non-const as well.

https://codereview.chromium.org/1027373002/diff/20001/src/core/SkFontDescript...
File src/core/SkFontDescriptor.cpp (right):

https://codereview.chromium.org/1027373002/diff/20001/src/core/SkFontDescript...
src/core/SkFontDescriptor.cpp:56: SkAutoSTMalloc<4, SkFixed> axis;
It would be nice to break up the serialization of the font data and the font
request. There are times when we want one, the other, or both.

https://codereview.chromium.org/1027373002/diff/20001/src/core/SkFontDescript...
src/core/SkFontDescriptor.cpp:107: if (fFontData->getIndex()) {
Might not have a fontData here.

https://codereview.chromium.org/1027373002/diff/20001/src/ports/SkFontConfigP...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/1027373002/diff/20001/src/ports/SkFontConfigP...
src/ports/SkFontConfigParser_android.cpp:166: } else if (MEMEQ("axis", name,
nameLen)) {
Need support for multiple axes. By index, id (four char tag), or name? Note that
this already currently assumes the user knows the font here. As a result index
is easy and nice.

Axis space. Normalized or design? Prefer design.

Need to be able to specify in something other than 'SkFixed' in base 10.

Possibilities for 400 weight, slightly greater than normal width, no slant:

axis="400 1.2 0"

axis="wght:400 wdth:1.2 slnt:0" (also 'opsz' is standard)

axis="weight:400 width:1.2 slant:0" (also 'optical-size')

axis-wght="400" axis-wdth="1.2" axis-slnt="0"

axis-weight="400" axis-width="1.2" axis-slant="0"

'weight' is easier to read, but 'wght' maps directly to the underlying data in
the font, meaning that it is easy to use non-standard variation ids (since no
translation is needed). Using positional indexes here isn't very clear at all,
but is very simple to implement and extend, 'wght' means we have to do string
matching at some point. In theory though, we could do this string matching up
front based on the font scan. However, it's not clear how to specify all four
byte codes like this. In css this would look something like

font-axis-settings: 'wght' 800, 'wdth' 1.2, 'slnt' 0;

with the grammar:

<axis-tag-value>#
axis-tag-value: <string> <number>

on the other hand, I don't want to write too many parsers here.

[behdad_google, 2015-04-03 00:59:52]

> Axis space. Normalized or design? Prefer design.

I'm leaning on design space.  Problem is, even that is under specified even for
the standard axes.


> Need to be able to specify in something other than 'SkFixed' in base 10.

Does such mapping exist?


> Possibilities for 400 weight, slightly greater than normal width, no slant:
> 
> axis="400 1.2 0"

But if Skia font is to be relied on, weight would be more like 1.0.

> axis="wght:400 wdth:1.2 slnt:0" (also 'opsz' is standard)

I like this, losely based on CSS font-feature-settings.


> axis="weight:400 width:1.2 slant:0" (also 'optical-size')
> 
> axis-wght="400" axis-wdth="1.2" axis-slnt="0"
> 
> axis-weight="400" axis-width="1.2" axis-slant="0"

You can have the last line for the standard axes.


> 'weight' is easier to read, but 'wght' maps directly to the underlying data in
> the font, meaning that it is easy to use non-standard variation ids (since no
> translation is needed).

You don't want to allow arbitrary axis tags as XML attributes.

[bungeman-skia, 2015-04-13 21:00:39]

On 2015/04/03 00:59:52, behdad_google wrote:
> > Axis space. Normalized or design? Prefer design.
> 
> I'm leaning on design space.  Problem is, even that is under specified even
for
> the standard axes.
> 

Design space is correct for this application at the very least. There is no
meaning at all to the normalized space (it's just the extremes of these
particular axis). While the design space isn't really standardized, it at least
makes sense to the font. Since in this application it is assumed you know the
font, the design space is appropriate.

> 
> > Need to be able to specify in something other than 'SkFixed' in base 10.
> 
> Does such mapping exist?
> 

The issue here is just that currently the implementation reads in a base 10
number, but interprets it as base 2 16.16. This is silly and needs to be fixed
before committing anything. This is just a TODO to myself.

> 
> > Possibilities for 400 weight, slightly greater than normal width, no slant:
> > 
> > axis="400 1.2 0"
> 
> But if Skia font is to be relied on, weight would be more like 1.0.

Sure, for the Skia font's 'wght' axis. We can blame reed for this, since he
started it. The idea was that these would have some meaning like 'wght' and
'wdth' would be a kind of multiplier, in some sense 1.0 would be the 'normal'
weight and then 0.5 would be half the weight and 2.0 would be twice the weight.
Similar with 'wdth'. However, these are totally arbitrary and the design units
can be anything. I'd actually prefer if the weights sat on the 100 - 900 scale
and the widths on the 1 - 9 scale to match everything else (and the common way
in which everyone now refers to them on the user side). You have to know
something about the font anyway to map the design units to anything anyhow.

> 
> > axis="wght:400 wdth:1.2 slnt:0" (also 'opsz' is standard)
> 
> I like this, losely based on CSS font-feature-settings.
> 

I rather like this as well, but it means I need to write a parser.

> 
> > axis="weight:400 width:1.2 slant:0" (also 'optical-size')
> > 
> > axis-wght="400" axis-wdth="1.2" axis-slnt="0"
> > 
> > axis-weight="400" axis-width="1.2" axis-slant="0"
> 
> You can have the last line for the standard axes.

Yes, but more than one format means more work for me :-) Maybe.

> 
> 
> > 'weight' is easier to read, but 'wght' maps directly to the underlying data
in
> > the font, meaning that it is easy to use non-standard variation ids (since
no
> > translation is needed).
> 
> You don't want to allow arbitrary axis tags as XML attributes.

Yeah, open ended attributes doesn't look good.

[bungeman-skia, 2015-04-23 21:16:34]

Tried other platforms, to see how that affects what serializes.

https://codereview.chromium.org/1027373002/diff/40001/src/core/SkFontDescript...
File src/core/SkFontDescriptor.h (right):

https://codereview.chromium.org/1027373002/diff/40001/src/core/SkFontDescript...
src/core/SkFontDescriptor.h:35: SkAutoSTMalloc<4, SkFixed> fAxis;
Is this the right way to serialize the axes (as a list of fixed point values)?
Internally, we know that in the font they come in some consistent order, so this
should be enough to reconstruct. However, making each axis here a pair
(SkFourByteCode, SkFixed) does make things clearer, and while the order and the
code are somewhat redundant information, it does make working with CoreText a
lot simpler (and probably safer).

In FreeType the ordering can be trusted to work out.

With CoreText, CTFontCopyVariation returns an unordered { tag|name : double }
and while currently always in the right order there is no guarantee.
CTFontCopyVariationAxes returns a [{tag:int, min:double, max:double, def:double,
name:string}] which could be used to sort against, but there is also no strict
guarantee that the order of this array will be stable. Also, as it turns out
CTFont*Variation* methods only work when the font is created with CT/NS. Fonts
created with CG and converted to CT don't seem to work correctly (much like
CTFontCopyTable / CGFontCopyTableForTag), so it turns out we may need to rely on
the CG versions of these since that's the only way to create a font from data.
This means that just using public API on OSX, we cannot even know the tags, and
must rely on array ordering.

GDI kinda supports MM, and you can get the values used, but there doesn't appear
to be any means of setting the values when creating a font. DirectWrite doesn't
appear to support the notion at all, outside of its synthetic attributes. If
DirectWrite ever does support this, I would imagine that there would be a
enumeration with the various known values and a IDWriteFontX / IDWriteFontFaceX
with some interface for querying/setting values.

enum DWRITE_FONT_AXIS_TAG {
  DWRITE_FONT_AXIS_TAG_DEFAULT,
  DWRITE_FONT_AXIS_TAG_WEIGHT = 0x74686777,
  DWRITE_FONT_AXIS_TAG_WIDTH = 0x68746477,
  DWRITE_FONT_AXIS_TAG_SLANT = 0x746E6C73,
  DWRITE_FONT_AXIS_TAG_OPTICAL_SIZE = 0x7A73706F,
};

struct DWRITE_FONT_AXIS_METRICS {
  DWRITE_FONT_AXIS_TAG tag;
  FLOAT min;
  FLOAT default;
  FLOAT max;
};

IDWriteFontX {
  virtual UINT32 GetAxisCount() = 0;
  virtual HRESULT GetAxesCoordinates([in, out] UINT32 * numberOfAxes, [out,
optional] FLOAT* axes) = 0;
  virtual HRESULT GetAxisMetrics(UINT32 index, [out] DWRITE_FONT_AXIS_METRICS *
metrics) = 0;
  virtual HRESULT GetAxisNames(UINT32 index, [out] IDWriteLocalizedStrings **
names) = 0;
  virtual HRESULT CreateFontFace(UINT32 numberOfAxes, [in] FLOAT* axes, [out]
IDWriteFontFaceX ** fontFace) = 0;

  virtual UINT32 GetNamedStylesCount() = 0;
  virtual HRESULT GetNamedStyleCoordinates(UINT32 index, UINT32 numberOfAxes,
[out] FLOAT* axes) = 0;
  virtual HRESULT GetNamedStyleNames(UINT32 index, [out] IDWriteLocalizedStrings
** names) = 0;
};

From this sort of thought experiment, it seems likely that positional axes would
be supported, though like the current CoreText API, there may be no reason to
believe that after being serialized and deserialized that the axes in the
deserialized font are necessarily in the same order. All of this to say, we
should always have access to an axis tag, so always pass that with the values.
This is a minimal amount of extra information to carry, as this information is
being passed with the rest of the font data in any event.

https://codereview.chromium.org/1027373002/diff/60001/src/ports/SkFontHost_ma...
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/1027373002/diff/60001/src/ports/SkFontHost_ma...
src/ports/SkFontHost_mac.cpp:1857: axes[0].fValue = 4 * SK_Fixed1;
This is a test to see what happens when a variation is set to a value out of
bounds. It appears that there are no complaints, though it's not clear what the
implications are. Pinning will need to be done manually.

https://codereview.chromium.org/1027373002/diff/60001/src/ports/SkFontHost_ma...
src/ports/SkFontHost_mac.cpp:2393: CTFontRef ct =
CTFontCreateWithGraphicsFont(cg, 0, NULL, descriptor);
So this starts the buggy. This will create a CTFont which does have the axes set
internally (will draw correctly), but when the variations are requested it just
returns NULL (like it didn't have any). As a result, it seems variations will
have to be handled at the CGFont level. This patch exists mostly so that if this
ever becomes not buggy, the code exists to take advantage of that (since I
already wrote it to find this out). This also means that we won't have access to
the axis tag at all.

[bungeman-skia, 2015-04-28 22:11:55]

reed: This (at Patch Set 8) still needs a lot of cleanup work, but I would
appreciate any input on the changes to SkFontDescriptor, SkFontMgr, and
SkTypeface. The rest of this is implementation and test which can possibly be
landed separately.

[reed1, 2015-04-29 15:36:46]

https://codereview.chromium.org/1027373002/diff/140001/resources/android_font...
File resources/android_fonts/v22/fonts.xml (right):

https://codereview.chromium.org/1027373002/diff/140001/resources/android_font...
resources/android_fonts/v22/fonts.xml:5: <font weight="100" style="normal"><axis
tag="wght" stylevalue="32767.9999999"/>Roboto-Thin.ttf</font>
what is this value?

https://codereview.chromium.org/1027373002/diff/140001/src/core/SkFontDescrip...
File src/core/SkFontDescriptor.h (right):

https://codereview.chromium.org/1027373002/diff/140001/src/core/SkFontDescrip...
src/core/SkFontDescriptor.h:15: class SkFontData {
Is this class copyable? Having trouble knowing its lifetime given the public
apis that return a * and take a *

https://codereview.chromium.org/1027373002/diff/140001/src/core/SkFontDescrip...
src/core/SkFontDescriptor.h:26: SkStreamAsset* transferStream() { return
fStream.detach(); }
transfer -vs- detach? The latter seems like is already a verb used by skia. Can
we use it here?

https://codereview.chromium.org/1027373002/diff/140001/src/ports/SkFontConfig...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/1027373002/diff/140001/src/ports/SkFontConfig...
src/ports/SkFontConfigParser_android.cpp:122: */
why not cal sscanf and convert the double to a fixed?

[bungeman-skia, 2015-04-29 22:12:44]

https://codereview.chromium.org/1027373002/diff/140001/resources/android_font...
File resources/android_fonts/v22/fonts.xml (right):

https://codereview.chromium.org/1027373002/diff/140001/resources/android_font...
resources/android_fonts/v22/fonts.xml:5: <font weight="100" style="normal"><axis
tag="wght" stylevalue="32767.9999999"/>Roboto-Thin.ttf</font>
On 2015/04/29 15:36:46, reed1 wrote:
> what is this value?

This is more generally an example of how to specify the axis values in the xml
file, and this value itself is (close to) the maximum value which can fit in an
SkFixed (when truncating the low bits). This is just testing, so I'll probably
just add a section below with more representative values.

https://codereview.chromium.org/1027373002/diff/140001/src/core/SkFontDescrip...
File src/core/SkFontDescriptor.h (right):

https://codereview.chromium.org/1027373002/diff/140001/src/core/SkFontDescrip...
src/core/SkFontDescriptor.h:15: class SkFontData {
On 2015/04/29 15:36:46, reed1 wrote:
> Is this class copyable? Having trouble knowing its lifetime given the public
> apis that return a * and take a *

Done. Same issue with SkFontDescriptor below.

https://codereview.chromium.org/1027373002/diff/140001/src/core/SkFontDescrip...
src/core/SkFontDescriptor.h:26: SkStreamAsset* transferStream() { return
fStream.detach(); }
On 2015/04/29 15:36:46, reed1 wrote:
> transfer -vs- detach? The latter seems like is already a verb used by skia.
Can
> we use it here?

Done. I just moved the old 'transferFontData' up here. 'detach' is probably
good, since that's what we're doing in the impl.

https://codereview.chromium.org/1027373002/diff/140001/src/ports/SkFontConfig...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/1027373002/diff/140001/src/ports/SkFontConfig...
src/ports/SkFontConfigParser_android.cpp:122: */
On 2015/04/29 15:36:46, reed1 wrote:
> why not cal sscanf and convert the double to a fixed?

Because it's evil and should never be used.

https://www.securecoding.cert.org/confluence/display/c/INT05-C.+Do+not+use+in...

even the alternatives get mired in the current locale setting, which is
impossible to get right (and cannot be used correctly in threaded environments).
Also, I explicitly don't want all the double stuff (exponentiation and the
like), mostly so this in the future this can be reliably done more automatically
with a more full featured xml parser which supports xml types.

[bungeman-skia, 2015-05-06 19:55:41]

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

[commit-bot: I haz the power, 2015-05-06 19:56:17]

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

[commit-bot: I haz the power, 2015-05-06 19:57:41]

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

[commit-bot: I haz the power, 2015-05-06 19:57:42]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[bungeman-skia, 2015-05-06 21:08:09]

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

[commit-bot: I haz the power, 2015-05-06 21:08:28]

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

[commit-bot: I haz the power, 2015-05-06 21:14:40]

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

[commit-bot: I haz the power, 2015-05-06 21:14:42]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2015-05-06 23:09:54]

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

[bungeman-skia, 2015-05-06 23:09:55]

At Patch Set 17 this has tests and is fairly clean and doesn't appear to break
anything else (see tries in Patch Set 16). Please take a look.

[bungeman-skia, 2015-05-07 15:49:38]

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

[commit-bot: I haz the power, 2015-05-07 15:49:58]

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

[commit-bot: I haz the power, 2015-05-07 15:56:12]

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

[commit-bot: I haz the power, 2015-05-07 15:56:13]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-05-08 20:46:30]

https://codereview.chromium.org/1027373002/diff/320001/include/core/SkTypeface.h
File include/core/SkTypeface.h (right):

https://codereview.chromium.org/1027373002/diff/320001/include/core/SkTypefac...
include/core/SkTypeface.h:138: is not valid font data, returns null. Ownership
of the stream is
ownership of the data?

https://codereview.chromium.org/1027373002/diff/320001/include/ports/SkFontMgr.h
File include/ports/SkFontMgr.h (right):

https://codereview.chromium.org/1027373002/diff/320001/include/ports/SkFontMg...
include/ports/SkFontMgr.h:97: SkTypeface* createFromFontData(SkFontData*) const;
ownership of data?

[bungeman-skia, 2015-05-11 22:09:35]

https://codereview.chromium.org/1027373002/diff/320001/include/core/SkTypeface.h
File include/core/SkTypeface.h (right):

https://codereview.chromium.org/1027373002/diff/320001/include/core/SkTypefac...
include/core/SkTypeface.h:138: is not valid font data, returns null. Ownership
of the stream is
On 2015/05/08 20:46:30, reed1 wrote:
> ownership of the data?

Right now the caller retains ownership of the SkFontData*, but after this call
the stream (if any) on the SkFontData* will have been detached and the callee
now owns the stream (either my storing it or deleting it). This is a bit
confusing. The reason this works at the moment is because the stream is the only
thing big and a pointer to a long lived thing, so essentially the stream is
moved, but the rest is copied. I think the idea is that the user could just put
a SkFontData on the stack and have passed its address, but they're not allowed
to have done that with the stream. I'm already requiring an asset stream here,
so we could duplicate it in this case, but that seems like extra work that isn't
usually needed.

I can, of course, explain this better in the comment, but this sort of thing is
what I'm most concerned about anyway. I'll have to see if there is a cleaner way
of doing this (which also allows fewer copies of information to begin with, like
the axis data which is copied like three times in the Android case).

This is just a helper function to call the associated method on SkFontMgr, so
the same goes for that method.

[bungeman-skia, 2015-05-12 15:28:58]

https://codereview.chromium.org/1027373002/diff/340001/src/ports/SkFontHost_F...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/1027373002/diff/340001/src/ports/SkFontHost_F...
src/ports/SkFontHost_FreeType.cpp:325: SkFontData* data =
typeface->createFontData();
This is a leak. The stream is detached, but the data is never deleted.

[bungeman-skia, 2015-05-12 19:17:33]

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

[commit-bot: I haz the power, 2015-05-12 19:18:03]

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

[commit-bot: I haz the power, 2015-05-12 19:18:21]

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

[commit-bot: I haz the power, 2015-05-12 19:18:22]

Dry run: Failed to apply patch for include/core/SkAdvancedTypefaceMetrics.h:
While running git apply --index -3 -p1;
  error: include/core/SkAdvancedTypefaceMetrics.h: does not exist in index

Patch:       include/core/SkAdvancedTypefaceMetrics.h
Index: include/core/SkAdvancedTypefaceMetrics.h
diff --git a/include/core/SkAdvancedTypefaceMetrics.h
b/include/core/SkAdvancedTypefaceMetrics.h
index
06757c0c17288bb459d6f65a443ca8e44c64ba07..000e8fbfea70350e585d66eadd41f74e3348cfbe
100644
--- a/include/core/SkAdvancedTypefaceMetrics.h
+++ b/include/core/SkAdvancedTypefaceMetrics.h
@@ -43,7 +43,7 @@ public:
 
     enum FontFlags {
         kEmpty_FontFlag          = 0x0,  //!<No flags set
-        kMultiMaster_FontFlag    = 0x1,  //!<May be true for Type1 or CFF
fonts.
+        kMultiMaster_FontFlag    = 0x1,  //!<May be true for Type1, CFF, or
TrueType fonts.
         kNotEmbeddable_FontFlag  = 0x2,  //!<May not be embedded.
         kNotSubsettable_FontFlag = 0x4,  //!<May not be subset.
     };

[bungeman-skia, 2015-05-12 19:26:59]

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

[commit-bot: I haz the power, 2015-05-12 19:27:15]

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

[commit-bot: I haz the power, 2015-05-12 19:33:31]

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

[commit-bot: I haz the power, 2015-05-12 19:33:32]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2015-05-12 19:40:07]

At PS20 this should now have easier to understand ownership of SkFontData as it
is passed around.

[reed1, 2015-05-13 17:40:25]

lgtm

[bungeman-skia, 2015-05-13 17:50:14]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2015-05-13 17:50:30]

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

[commit-bot: I haz the power, 2015-05-13 17:57:16]

Committed patchset #21 (id:400001) as
https://skia.googlesource.com/skia/+/05773ed30920c0214d1433c07cf6360a05476c97

[bungeman-skia, 2015-05-13 19:13:56]

A revert of this CL (patchset #21 id:400001) has been created in
https://codereview.chromium.org/1128913008/ by bungeman@google.com.

The reason for reverting is: Mac failing to build due to CFNumberType in
Chromium Canary..

[bungeman-skia, 2015-05-13 23:04:45]

At PS23, Mac and Windows should now have no change in behavior. Still trying to
see what to do about Android.

https://codereview.chromium.org/1027373002/diff/400001/src/ports/SkFontHost_m...
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/1027373002/diff/400001/src/ports/SkFontHost_m...
src/ports/SkFontHost_mac.cpp:1800: if (!CFNumberGetValue(valueNumber,
CFNumberType::kCFNumberDoubleType, &valueDouble) ||
This should have just been just kCFNumberDoubleType as CFNumberType is an enum.

https://codereview.chromium.org/1027373002/diff/400001/src/ports/SkFontHost_m...
src/ports/SkFontHost_mac.cpp:1841: if (!CFNumberGetValue(axisDefaultValueNumber,
CFNumberType::kCFNumberDoubleType,
Same here.

https://codereview.chromium.org/1027373002/diff/420001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1027373002/diff/420001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:164: if (!desc.hasFontData()) {
Whoops, meant to leave the isLocal. It shouldn't make a difference, but
apparently it does. This exposes a failure on DirectWrite, and possible changes
in behavior on Mac.

https://codereview.chromium.org/1027373002/diff/420001/src/ports/SkFontHost_F...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/1027373002/diff/420001/src/ports/SkFontHost_F...
src/ports/SkFontHost_FreeType.cpp:285: if (FT_Get_MM_Var(face, &variations)) {
The Android Framework build died here because, while it builds all of the code
to do this (and if you know the right hackery to get the function pointer it
works), it doesn't build the ftmm.c file which is tiny and exports FT_Get_MM_Var
and several other calls which are basically just forwards through the
appropriate vtable. This will need to be added.

[bungeman-skia, 2015-05-14 20:25:41]

The CQ bit was checked by bungeman@google.com

[bungeman-skia, 2015-05-14 20:25:42]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1027373002/#ps500001
(title: "Use defined.")

[commit-bot: I haz the power, 2015-05-14 20:26:18]

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

[commit-bot: I haz the power, 2015-05-14 21:18:11]

Committed patchset #26 (id:500001) as
https://skia.googlesource.com/skia/+/3489ee0f4fa34f124f9de090d12bdc2107d52aa9

[robertphillips, 2015-05-15 18:29:52]

A revert of this CL (patchset #26 id:500001) has been created in
https://codereview.chromium.org/1139123008/ by robertphillips@google.com.

The reason for reverting is: Appears to be breaking  Linux ARM bots:

FAILED:
/usr/local/google/home/mosaic-role/slave/repo_clients/chromium_tot/chromium/src/../../prebuilt/toolchain/armv7a/bin/armv7a-cros-linux-gnueabi-g++
... -o obj/third_party/skia/src/ports/skia_library.SkFontHost_FreeType.o
../../third_party/skia/src/ports/SkFontHost_FreeType.cpp:37:31: fatal error:
freetype/ftmm.h: No such file or directory
 #include FT_MULTIPLE_MASTERS_H
                               ^
compilation terminated.
.

[bungeman-skia, 2015-05-20 15:46:41]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2015-05-20 15:46:54]

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

[commit-bot: I haz the power, 2015-05-20 15:47:56]

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

[commit-bot: I haz the power, 2015-05-20 15:47:57]

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-05-20 15:48:17]

The CQ bit was unchecked by bungeman@google.com

[bungeman-skia, 2015-05-20 16:14:13]

The CQ bit was checked by bungeman@google.com

[bungeman-skia, 2015-05-20 16:14:13]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1027373002/#ps520001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-20 16:14:34]

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

[commit-bot: I haz the power, 2015-05-20 16:21:09]

Committed patchset #27 (id:520001) as
https://skia.googlesource.com/skia/+/41868fe5625fc3bd70daa3f461c881b5db6a9265