Add a new SkFontHost that wraps a cairo_scaled_font_t

Add a new SkFontHost that wraps a cairo_scaled_font_t

Committed: https://code.google.com/p/skia/source/detail?r=5246

[gw280, 2012-08-06 21:59:30]

Just a request for comments at the moment. I've tested the SkFontHost_FreeType
using gm and got 100% pass. I've not yet tested the SkFontHost_cairo except
within Mozilla (using an older version of skia) but hopefully it works!

[reed1, 2012-08-06 22:08:57]

In general, I like it. Could almost make this two CLs
1. moving some utilities out into freetype_common
2. adding cairo subclass

But I'm ok with them combined, as it helps motivate why some functions were
exported.

In general, my interest will be in making freetype_common as robustly named and
documented as possible, since now that will become, in its own way, an API that
we need to maintain.

For example, the function names themselves should probably be better scoped.
Could we make up a class just to house them?

e.g.

class SkFreeTypeCommon {
public:
    static void generateGlyphImage(...);
    ...
};

https://codereview.appspot.com/6442092/diff/1/include/ports/SkTypeface_cairo.h
File include/ports/SkTypeface_cairo.h (right):

https://codereview.appspot.com/6442092/diff/1/include/ports/SkTypeface_cairo....
include/ports/SkTypeface_cairo.h:5: 
Can you document here any lifecycle constraints?
- Does the typeface have a "ref" on the cairo font?
- Can the typeface be used in any thread after this?

https://codereview.appspot.com/6442092/diff/1/src/ports/SkFontHost_FreeType_c...
File src/ports/SkFontHost_FreeType_common.cpp (right):

https://codereview.appspot.com/6442092/diff/1/src/ports/SkFontHost_FreeType_c...
src/ports/SkFontHost_FreeType_common.cpp:6: */
Update copyright to 2012 ?

https://codereview.appspot.com/6442092/diff/1/src/ports/SkFontHost_FreeType_c...
File src/ports/SkFontHost_FreeType_common.h (right):

https://codereview.appspot.com/6442092/diff/1/src/ports/SkFontHost_FreeType_c...
src/ports/SkFontHost_FreeType_common.h:35: // convert from Skia's fixed (16.16)
to FreeType's fixed (26.6) representation
Are these converter functions already in SkFDot6.h ?

https://codereview.appspot.com/6442092/diff/1/src/ports/SkFontHost_FreeType_c...
src/ports/SkFontHost_FreeType_common.h:42: void
generateFreeTypeGlyphImage(FT_Face face, SkScalerContext::Rec rec, const
SkGlyph& glyph, SkMaskGamma::PreBlend* maskPreBlend);
const SkScalerContext::Rec& rec ?

[gw280, 2012-08-15 18:13:45]

So here's the updated patch that only deals with splitting out common functions
from SkFontHost_FreeType for now. I've incorporated all your suggestions, Mike.

[bungeman, 2012-08-16 19:55:03]

I like this approach in general much more than the previous approach (
http://codereview.appspot.com/5846051/ ).

https://codereview.appspot.com/6442092/diff/7001/src/ports/SkFontHost_FreeTyp...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.appspot.com/6442092/diff/7001/src/ports/SkFontHost_FreeTyp...
src/ports/SkFontHost_FreeType.cpp:9: #include "SkFontHost_FreeType_common.h"
Is there any particular reason why this include is here instead of just after

17: #include "SkFontHost.h"

which would be the normal place to put it.

https://codereview.appspot.com/6442092/diff/7001/src/ports/SkFontHost_FreeTyp...
File src/ports/SkFontHost_FreeType_common.cpp (right):

https://codereview.appspot.com/6442092/diff/7001/src/ports/SkFontHost_FreeTyp...
src/ports/SkFontHost_FreeType_common.cpp:9: #include
"SkFontHost_FreeType_common.h"
Should be after

12: #include "SkFDot6.h"

unless there is good reason.

https://codereview.appspot.com/6442092/diff/7001/src/ports/SkFontHost_FreeTyp...
File src/ports/SkFontHost_FreeType_common.h (right):

https://codereview.appspot.com/6442092/diff/7001/src/ports/SkFontHost_FreeTyp...
src/ports/SkFontHost_FreeType_common.h:29: static const FT_Pos
kBitmapEmboldenStrength = 1 << 6;
kBitmapEmboldenStrength needs to be scoped inside SkFreeTypeCommon.

https://codereview.appspot.com/6442092/diff/7001/src/ports/SkFontHost_FreeTyp...
src/ports/SkFontHost_FreeType_common.h:31: class SkFreeTypeCommon {
Since the methods here are only useful to FreeType FontHosts and would need to
be used by all of them, I would prefer this not to look like a utility class. I
would prefer something like

SkScalerContext_FreeType_Base : public SkScalerContext {
protected:
    SkScalerContext_FreeType_Base(const SkDescriptor* desc) :
SkScalerContext(desc) { }
    void generateFreeTypeGlyphImage(FT_Face face, const SkGlyph& glyph,
SkMaskGamma::PreBlend* maskPreBlend);
    void generateFreeTypeGlyphPath(FT_Face face, const SkGlyph& glyph, SkPath*
path);
    void emboldenFreeTypeOutline(FT_Face face, FT_Outline* outline);

    static const FT_Pos kBitmapEmboldenStrength = 1 << 6;
};

in order to drop the explicit "const SkScalerContext::Rec& rec" "this" and make
it explicit that no one else should ever be using these. Then
SkScalerContext_FreeType_XXX derive from this type. I would prefer to also drop
the FT_Face parameter as well by having this base class know about the FT_Face,
but I see that this would be an issue for the cairo backed font host.

I'm not exactly sure about the naming convention, reed may have a better name
than SkScalerContext_FreeType_Base. Depending on the name chosen, this may also
entail renaming this file.

[gw280, 2012-08-22 21:41:40]

Review ping

[bungeman, 2012-08-22 22:19:13]

LGTM