George Wright (:gw280) (needinfo me!) Reporter
	Description • 13 years ago

Currently our "FreeType" gfxFont implementations aren't really FreeType but in fact are Cairo/FreeType. With the overall direction we're taking in graphics to move towards Azure and removing our direct ties to Cairo, it doesn't make much sense to use Cairo's font handling APIs in our core classes.

It may be worth looking at the overall font strategy instead of just the Linux/Android cases that use Cairo. As far as I can tell, at the moment we pass around special font objects that encapsulate the underlying API very early on; it might be worth delaying our conversion to that API until fairly late on and just passing around generic "Font" objects that represent the font and/or font data stream in an abstract format until we actually need it to be in the underlying API we are using.

	George Wright (:gw280) (needinfo me!) Reporter
	Comment 1 • 13 years ago

Actually upon further thinking about this, the abstract idea doesn't make any sense. My thoughts were too pigeon holed and don't account for pretty much any of the cases we'd need to deal with with non-trivial fonts...

	George Wright (:gw280) (needinfo me!) Reporter
	Comment 2 • 12 years ago

Would like some feedback on this patch so far. As far as I know it doesn't actually break anything.

The major changes are:

- Make gfxFT2FontBase FreeType-only. It uses FT_Face internally and sets a scale matrix on it to represent a scaled font.
- Rename what used to be gfxFT2FontBase to gfxCairoFontBase
- Liberally remove a huge heap of dead code that doesn't even compile on non-Android platforms from gfxFT2Fonts.cpp (all the code that's in the #ifndef ANDROID block)
- Rename gfxFT2LockedFace to gfxCairoLockedFace
- Move all calculation functions out of gfxCairoLockedFace and into standalone functions in the gfxFT2Utils namespace, which can now be shared between Cairo and FT2 gfxFont implementations
- gfxCairoLockedFace is now just a very simple class to obtain an FT_Face from a cairo_scaled_font_t and release it when its object goes out of scope
- Made gfxFT2FontBase compile on Linux to pave the way for its use in Azure builds (we will be using the FT_Face-only class for Azure/Skia to mate with ScaledFontFreetype)
- gfxFT2FontBase now should *only* create a cairo_scaled_font_t for when we actually need to set one on the cairo_t used by gfxContext. We should hopefully never create one now for the Azure-content case.

There are a couple of concerns I have:

- gfxFT2Utils::CreateScaledFont() is basically a carbon copy of FT2FontEntry::CreateScaledFont(), but takes an FT_Face to create the scaled font instead. I'm worried primarily about the code duplication between these functions and also the way it determines whether to draw in italic; is there a difference between gfxFont::IsItalic() and gfxFont::GetStyle() & (NS_FONT_STYLE_ITALIC)?
- We don't have any face caching in place and I'm not sure if I'm going to be opening faces multiple times needlessly.

gfxFT2FontBase has been confirmed working on Android (doesn't currently work on Linux but it doesn't currently anyway) and gfxPangoFonts works fine with these changes on Linux.

	George Wright (:gw280) (needinfo me!) Reporter
	Comment 4 • 12 years ago

Comment on attachment 618368 [details] [diff] [review]
Slight cleanup; fix the IsItalic() confusion in CreateScaledFont.

Vlad's agreed to take a look at this which is great because he's based out of Toronto atm :)

	Mozilla RelEng Bot
	Comment 5 • 12 years ago

Autoland Patchset:
	Patches: 618295
	Branch: mozilla-central => try
Patch 618295 could not be applied to mozilla-central.
patching file gfx/thebes/Makefile.in
Hunk #3 FAILED at 111
1 out of 10 hunks FAILED -- saving rejects to file gfx/thebes/Makefile.in.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

	Karl Tomlinson (:karlt)
	Comment 6 • 12 years ago

If gfxCairoFontBase is used only by gfxFcFont, then it can be merged into
gfxFcFont.  If gfxFT2FontBase is used only by gfxFT2Fonts, then it can be
merged into gfxFT2Fonts.  gfxFT2LockedFace can move into gfxPangoFonts.cpp and
keep its name.

Doing the merge in a separate patch may make sense to clarify what is
changing.  The first patch can then hg copy gfxFT2FontBase.x
gfxCairoFontBase.x.  I would call gfxCairoFontBase something like
gfxCairoFTFont.

> Move all calculation functions out of [gfxFT2LockedFace] and into standalone
> functions in the gfxFT2Utils namespace,

That sounds sensible/necessary.  Perhaps, gfxFT2LockedFace could keep handy
methods that just call the FT2Utils functions.

Consider making gfxFT2Utils::GenerateMetrics only operate on a non-NULL
FT_Face so that the gfxFont* parameter is not necessary.

gfxFT2Utils sounds an unusual place to have CreateScaledFont(), but I assume
this will soon disappear once gfxFT2Fonts has non-cairo glyph extent code.
(I imagine that needs to happen to resolve cairo FT_Face scale/transform
issues.)

	Karl Tomlinson (:karlt)
	Comment 7 • 12 years ago

(In reply to Karl Tomlinson (:karlt) from comment #6)
> If gfxFT2FontBase is used only by gfxFT2Fonts, then it can be
> merged into gfxFT2Fonts.

I guess gfxFcFont is likely to move away from cairo one day, so maybe it makes sense to keep a separate gfxFT2FontBase.

	Nick Cameron [:nrc]
	Comment 8 • 12 years ago

When this is baked, please check if the patch from Bug 752380 can be removed, I hope this will obsolete it.

	George Wright (:gw280) (needinfo me!) Reporter
	Comment 9 • 12 years ago

(In reply to Karl Tomlinson (:karlt) from comment #6)
> If gfxCairoFontBase is used only by gfxFcFont, then it can be merged into
> gfxFcFont.  If gfxFT2FontBase is used only by gfxFT2Fonts, then it can be
> merged into gfxFT2Fonts.  gfxFT2LockedFace can move into gfxPangoFonts.cpp
> and
> keep its name.
> 
> Doing the merge in a separate patch may make sense to clarify what is
> changing.  The first patch can then hg copy gfxFT2FontBase.x
> gfxCairoFontBase.x.  I would call gfxCairoFontBase something like
> gfxCairoFTFont.
> 
> > Move all calculation functions out of [gfxFT2LockedFace] and into standalone
> > functions in the gfxFT2Utils namespace,
> 
> That sounds sensible/necessary.  Perhaps, gfxFT2LockedFace could keep handy
> methods that just call the FT2Utils functions.

These concerns are all being addressed in the other bug. I should probably mark this one as a duplicate of bug 762737

> Consider making gfxFT2Utils::GenerateMetrics only operate on a non-NULL
> FT_Face so that the gfxFont* parameter is not necessary.

That sounds sensible. I'll look into that.

> gfxFT2Utils sounds an unusual place to have CreateScaledFont(), but I assume
> this will soon disappear once gfxFT2Fonts has non-cairo glyph extent code.
> (I imagine that needs to happen to resolve cairo FT_Face scale/transform
> issues.)

I agree. I've moved this function into gfxFT2FontBase as "CairoScaledFont" and the cairo_scaled_font_t is lazily allocated and held as a member variable so as not to cause is to unnecessarily create it if we're using a backend like Skia.