Closed Bug 462908 Opened 11 years ago Closed 11 years ago

Implement Freetype font backend for windows ce

Categories

(Core :: Graphics, defect)

ARM
Windows CE
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [fixed1.9.1b4])

Attachments

(1 file, 17 obsolete files)

77.27 KB, patch
jtd
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch first cut (obsolete) — Splinter Review
Attachment #346837 - Flags: superreview?(vladimir)
Attachment #346837 - Flags: review?(pavlov)
Depends on: 463532
Attachment #346837 - Flags: superreview?(vladimir)
Attachment #346837 - Flags: superreview?(pavlov)
Attachment #346837 - Flags: review?(pavlov)
Attachment #346837 - Flags: review?(mozbugz)
Comment on attachment 346837 [details] [diff] [review]
first cut

Why do we need cairo-win32-ft-font.c ?  Can't the regular ft font backend be used directly?

Also, the #ifdefs in cairo should be specific to what they're used for -- remember that we're not the upstream for that code.  Removing the fontconfig dependency is good, so I'd suggest something like #ifndef CAIRO_DISABLE_FONTCONFIG or somesuch.
Cairo-win32-ft-font.c is needed because Cairo-win32-surface.c uses functions that are in Cairo-win32-font.c but not in Cairo-FT-font.c. 

Setting a macro to use FT without fc is a good idea.
Comment on attachment 346837 [details] [diff] [review]
first cut

I looked over this once (and haven't yet looked at everything).  The main
thing I don't yet understand is the relationship between
cairo_win32_scaled_font_t and cairo_ft_font_face_t.

Why does _cairo_win32_scaled_font_is_type1 expect a cairo_ft_font_face_t while
the others _cairo_win32_scaled_font_ functions expect a
cairo_win32_scaled_font_t?

Some other mostly small things that I noticed:

+#ifndef WIN_FREETYPE
     FcPattern *pattern, *resolved;
+
     cairo_ft_unscaled_font_t *unscaled;

No need for a whitespace change.


@@ -1717,22 +1737,32 @@ _cairo_ft_scaled_font_create_toy (cairo_
 					   font_options, ft_options,
 					   font);
 
     _cairo_unscaled_font_destroy (&unscaled->base);
 
  FREE_RESOLVED:
     FcPatternDestroy (resolved);
 
  FREE_PATTERN:
     FcPatternDestroy (pattern);
-
     return status;

Leave the whitespace.

+#else
+    cairo_ft_unscaled_font_t *unscaled;
+    cairo_ft_options_t ft_options;
+    
+    _cairo_ft_unscaled_font_create_internal (TRUE, NULL, 0, toy_face->family);
+    return  _cairo_ft_scaled_font_create (unscaled,
+					  &toy_face->base,
+					  font_matrix, ctm,
+					  font_options, ft_options,
+					  font);
+#endif

Does this compile?

family is const char * whereas _cairo_ft_unscaled_font_create_internal takes
struct FT_FaceRec_*.

Maybe this function can be removed from _cairo_ft_scaled_font_backend?
Or maybe it should just return an unsupported status.


-    /* If making this compile without fontconfig, use:
-     * index = FT_Get_Char_Index (face, ucs4); */
+    /* If making this compile without fontconfig, use:*/
+#ifdef WIN_FREETYPE
+    index = FT_Get_Char_Index (face, ucs4); 
+#else
     index = FcFreeTypeCharIndex (face, ucs4);
-
+#endif

I guess the comment won't be needed any longer.

 }
 
+
 /**
  * cairo_ft_font_options_substitute:

whitespace.

+#ifndef WIN_FREETYPE  //might need and upgrade
     if (FT_Get_PS_Font_Info(face, &font_info) != 0) {
 	status = CAIRO_INT_STATUS_UNSUPPORTED;
         goto fail1;
     }
+#endif

I don't follow.  Is the FreeType version old or something?
But this function is used in cairo-win32-ft-font.c.

+cairo_bool_t
+_cairo_win32_scaled_font_is_bitmap (cairo_scaled_font_t *scaled_font)
+{
+    cairo_win32_scaled_font_t *win32_scaled_font;
+
+    win32_scaled_font = (cairo_win32_scaled_font_t *) scaled_font;
+
+    return win32_scaled_font->is_bitmap;
+}

Shouldn't _cairo_scaled_font_is_win32 (scaled_font) be checked here?

+protected:
+    PRBool FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], const gfxFontStyle& aFontStyle);

Explicitly including the virtual keyword would make things clearer here, I
think.

+    nsTArray<nsRefPtr<FontEntry> > mVariations;

What is the relationship between this and mFaces?  Are they both needed?

+#ifdef DEBUG
+        if (!mBlocks.DebugGetHeader()) {
+            printf("mHdr is null, this is bad\n");
+            return PR_FALSE;
+        }
+#endif        

I'll assume this is temporary code.

-    already_AddRefed<gfxWindowsFont> FindFontForChar(PRUint32 aCh, gfxWindowsFont *aFont);
+#ifdef WIN_FREETYPE
+already_AddRefed<gfxFont>
+#else
+already_AddRefed<gfxWindowsFont>
+#endif
+    FindFontForChar(PRUint32 aCh, gfxFont *aFont);

gfxWindowsFontGroup::WhichSystemFontSupportsChar(PRUint32) would also be fine
or better with this returning an already_AddRefed<gfxFont>.

 FontEntry *
 FontFamily::FindFontEntry(const gfxFontStyle& aFontStyle)
 {
+#ifndef WIN_FREETYPE

...

+#else
+    PRBool bold = PR_FALSE;
+    return static_cast<FontEntry*>(FindFontForStyle(aFontStyle, bold ));
+#endif

Should non-WIN_FREETYPE use FindFontForStyle too?


-    nsRefPtr<gfxFont> font = gfxFontCache::GetCache()->Lookup(aName, aStyle);
+    gfxFontStyle style(*aStyle);
+    nsRefPtr<gfxFont> font = gfxFontCache::GetCache()->Lookup(aName, &style);
     if (!font) {
-        FontEntry *fe = gfxToolkitPlatform::GetPlatform()->FindFontEntry(aName, *aStyle);
+        FontEntry *fe = gfxToolkitPlatform::GetPlatform()->FindFontEntry(aName, style);

Why the copy?

-    }
+    } 
     gfxFont *f = nsnull;
     font.swap(f);
-    return static_cast<gfxFT2Font *>(f);
+    return static_cast<gfxFT2Font*>(f);
 }
-

Don't change whitespace unnecessarily.


     // for each font family, load in various font info
     for (i = mStartIndex; i < endIndex; i++) {
         // load the cmaps for all variations
+#ifndef WIN_FREETYPE
         mFontFamilies[i]->FindStyleVariations();
+#endif
     }

This would be clearer if the preprocessor conditional enclosed the loop.
(In reply to comment #4)
> (From update of attachment 346837 [details] [diff] [review])
> I looked over this once (and haven't yet looked at everything).  The main
> thing I don't yet understand is the relationship between
> cairo_win32_scaled_font_t and cairo_ft_font_face_t.
> 
> Why does _cairo_win32_scaled_font_is_type1 expect a cairo_ft_font_face_t while
> the others _cairo_win32_scaled_font_ functions expect a
> cairo_win32_scaled_font_t?
> 

Perhaps I've read your comment wrong, but I'm a little confused. _cairo_win32_scaled_font_is_type1 expects a cairo_scaled_font_t* which is the same as the 3 other _cairo_win32_scaled_font_* functions in cairo-win32-ft-font.c.  cairo-win32-fonts.c isn't compiled when WIN_FREETYPE is set.

+cairo_bool_t
+_cairo_win32_scaled_font_is_type1 (cairo_scaled_font_t *scaled_font)
+{

> +#else
> +    cairo_ft_unscaled_font_t *unscaled;
> +    cairo_ft_options_t ft_options;
> +    
> +    _cairo_ft_unscaled_font_create_internal (TRUE, NULL, 0, toy_face->family);
> +    return  _cairo_ft_scaled_font_create (unscaled,
> +                      &toy_face->base,
> +                      font_matrix, ctm,
> +                      font_options, ft_options,
> +                      font);
> +#endif
> 
> Does this compile?
> 
> family is const char * whereas _cairo_ft_unscaled_font_create_internal takes
> struct FT_FaceRec_*.
> 
> Maybe this function can be removed from _cairo_ft_scaled_font_backend?
> Or maybe it should just return an unsupported status.


It does compile, but that doesn't mean its right would passing toy_face be the right thing to do here?

> +#ifndef WIN_FREETYPE  //might need and upgrade
>      if (FT_Get_PS_Font_Info(face, &font_info) != 0) {
>      status = CAIRO_INT_STATUS_UNSUPPORTED;
>          goto fail1;
>      }
> +#endif
> 
> I don't follow.  Is the FreeType version old or something?
> But this function is used in cairo-win32-ft-font.c.
> 


At first I was building against version 2.1.7, I've since upgraded to 2.3.7

> +cairo_bool_t
> +_cairo_win32_scaled_font_is_bitmap (cairo_scaled_font_t *scaled_font)
> +{
> +    cairo_win32_scaled_font_t *win32_scaled_font;
> +
> +    win32_scaled_font = (cairo_win32_scaled_font_t *) scaled_font;
> +
> +    return win32_scaled_font->is_bitmap;
> +}
> 
> Shouldn't _cairo_scaled_font_is_win32 (scaled_font) be checked here?

yea, that would be good

> 
> +protected:
> +    PRBool FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], const
> gfxFontStyle& aFontStyle);
> 
> Explicitly including the virtual keyword would make things clearer here, I
> think.
> 
> +    nsTArray<nsRefPtr<FontEntry> > mVariations;
> 
> What is the relationship between this and mFaces?  Are they both needed?

I don't think so, removed

> 
> +#ifdef DEBUG
> +        if (!mBlocks.DebugGetHeader()) {
> +            printf("mHdr is null, this is bad\n");
> +            return PR_FALSE;
> +        }
> +#endif        
> 
> I'll assume this is temporary code.

Its for a bug I was hitting.  It doesn't matter much to me, but it might be useful in the future to get that warning if the same situation crops up.


> 
>  FontEntry *
>  FontFamily::FindFontEntry(const gfxFontStyle& aFontStyle)
>  {
> +#ifndef WIN_FREETYPE
> 
> ...
> 
> +#else
> +    PRBool bold = PR_FALSE;
> +    return static_cast<FontEntry*>(FindFontForStyle(aFontStyle, bold ));
> +#endif
> 
> Should non-WIN_FREETYPE use FindFontForStyle too?

+#ifdef WIN_FREETYPE
+        if (mVariations.Length() == 0)
+            FindStyleVariations();
   
That bit of code in FindFontForStyle does roughly the same job, which is why the WIN_FREETYPE implementation of FindFontEntry can be so short.

> 
> -    nsRefPtr<gfxFont> font = gfxFontCache::GetCache()->Lookup(aName, aStyle);
> +    gfxFontStyle style(*aStyle);
> +    nsRefPtr<gfxFont> font = gfxFontCache::GetCache()->Lookup(aName, &style);
>      if (!font) {
> -        FontEntry *fe =
> gfxToolkitPlatform::GetPlatform()->FindFontEntry(aName, *aStyle);
> +        FontEntry *fe =
> gfxToolkitPlatform::GetPlatform()->FindFontEntry(aName, style);
> 
> Why the copy?

aStyle was getting mangled in FindFontEntry.  I suspect it may have been caused by the same (fixed) bug that I have the previous debug output to check for, so I'll try removing it.
Attachment #346837 - Attachment is obsolete: true
Attachment #347650 - Flags: superreview?(pavlov)
Attachment #347650 - Flags: review?(mozbugz)
Attachment #346837 - Flags: superreview?(pavlov)
Attachment #346837 - Flags: review?(mozbugz)
Comment on attachment 347650 [details] [diff] [review]
updated based on vlad and karlt's comments

+++ b/gfx/cairo/cairo/src/cairo-win32-ft-font.c
@@ -0,0 +1,254 @@
+/* -*- Mode: c; tab-width: 8; c-basic-offset: 4; indent-tabs-mode: t; -*- */
+/* Cairo - a vector graphics library with display and print output
+ *
+ * Copyright © 2005 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it either under the terms of the GNU Lesser General Public
+ * License version 2.1 as published by the Free Software Foundation
+ * (the "LGPL") or, at your option, under the terms of the Mozilla
+ * Public License Version 1.1 (the "MPL"). If you do not alter this
+ * notice, a recipient may use your version of this file under either
+ * the MPL or the LGPL.
+ *
+ * You should have received a copy of the LGPL along with this library
+ * in the file COPYING-LGPL-2.1; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ * You should have received a copy of the MPL along with this library
+ * in the file COPYING-MPL-1.1
+ *
+ * The contents of this file are subject to the Mozilla Public License
+ * Version 1.1 (the "License"); you may not use this file except in
+ * compliance with the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * This software is distributed on an "AS IS" basis, WITHOUT WARRANTY
+ * OF ANY KIND, either express or implied. See the LGPL or the MPL for
+ * the specific language governing rights and limitations.
+ *
+ * The Original Code is the cairo graphics library.
+ *
+ * The Initial Developer of the Original Code is Red Hat, Inc.
+ *
+ * Contributor(s):
+ *	Brad Lassey <blassey@mozilla.com>
+ */

Is this a new file, or is this from Cairo itself? This license header is strange if it's something written by Mozilla, as it's not the normal tri-license.
--- a/gfx/cairo/cairo/src/cairo-ft.h
+++ b/gfx/cairo/cairo/src/cairo-ft.h

 /* Fontconfig/Freetype platform-specific font interface */
-
+#ifndef WIN_FREETYPE
 #include <fontconfig/fontconfig.h>
+#endif

CAIRO_DISABLE_FONTCONFIG

(In reply to comment #5)
> At first I was building against version 2.1.7, I've since upgraded to 2.3.7

+#ifndef WIN_FREETYPE  // might need an upgrade
     if (FT_Get_PS_Font_Info(face, &font_info) == 0)
         is_type1 = TRUE;
+#endif

This preprocessor conditional can be removed too then, I assume.

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 346837 [details] [diff] [review] [details])
> > I looked over this once (and haven't yet looked at everything).  The main
> > thing I don't yet understand is the relationship between
> > cairo_win32_scaled_font_t and cairo_ft_font_face_t.
> > 
> > Why does _cairo_win32_scaled_font_is_type1 expect a cairo_ft_font_face_t
> > while the others _cairo_win32_scaled_font_ functions expect a
> > cairo_win32_scaled_font_t?
> > 
> 
> Perhaps I've read your comment wrong, but I'm a little confused.
> _cairo_win32_scaled_font_is_type1 expects a cairo_scaled_font_t* which is the
> same as the 3 other _cairo_win32_scaled_font_* functions in
> cairo-win32-ft-font.c.  cairo-win32-fonts.c isn't compiled when WIN_FREETYPE
> is set.

I didn't quite say what I meant.

cairo-win32-ft-font.c defines cairo_win32_scaled_font_t and
_cairo_win32_scaled_font_backend, but is a cairo_win32_scaled_font_t ever
instantiated?

_cairo_win32_scaled_font_is_type1 passes the cairo_scaled_font_t* to
_cairo_ft_scaled_font_get_unscaled_font (after checks), which casts to
cairo_ft_scaled_font_t.

But the other _cairo_win32_scaled_font_* functions in cairo-win32-ft-font.c
cast to cairo_win32_scaled_font_t.

--- a/gfx/cairo/cairo/src/Makefile.in
+++ b/gfx/cairo/cairo/src/Makefile.in
@@ -148,8 +148,14 @@
 PS_EXPORTS = cairo-ps.h
 
 ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
-CSRCS	+= 	cairo-win32-font.c \
-			cairo-win32-surface.c
+CSRCS	+=	cairo-win32-surface.c
+
+ifdef WIN_FREETYPE
+DEFINES +=	-DCAIRO_DISABLE_FONTCONFIG
+CSRCS	+= 	cairo-win32-ft-font.c 
+else
+CSRCS	+= 	cairo-win32-font.c 
+endif
 
Is it necessary to disable cairo-win32-font in order to enable freetype fonts?

> > +#else
> > +    cairo_ft_unscaled_font_t *unscaled;
> > +    cairo_ft_options_t ft_options;
> > +    
> > +    _cairo_ft_unscaled_font_create_internal (TRUE, NULL, 0, toy_face->family);
> > +    return  _cairo_ft_scaled_font_create (unscaled,
> > +                      &toy_face->base,
> > +                      font_matrix, ctm,
> > +                      font_options, ft_options,
> > +                      font);
> > +#endif
> > 
> > Does this compile?
> > 
> > family is const char * whereas _cairo_ft_unscaled_font_create_internal takes
> > struct FT_FaceRec_*.
> > 
> > Maybe this function can be removed from _cairo_ft_scaled_font_backend?
> > Or maybe it should just return an unsupported status.
> 
> 
> would passing toy_face be the right thing to do here?

No, _cairo_ft_unscaled_font_create_internal needs either a filename and index
or a FT_Face (a handle to a font opened from freetype).  So a mapping from
toy_face->family/slant/weight and size to a specific font face is required, if
you wish to implement this function.

> > +#ifdef DEBUG
> > +        if (!mBlocks.DebugGetHeader()) {
> > +            printf("mHdr is null, this is bad\n");
> > +            return PR_FALSE;
> > +        }
> > +#endif        
> > 
> > I'll assume this is temporary code.
> 
> Its for a bug I was hitting.  It doesn't matter much to me, but it might be
> useful in the future to get that warning if the same situation crops up.

If you think it will be useful to someone in the future, then an NS_ASSERTION
would be good here.  I'm not comfortable the return statement causing
debug builds to take different code paths.

> >  FontEntry *
> >  FontFamily::FindFontEntry(const gfxFontStyle& aFontStyle)
> >  {
> > +#ifndef WIN_FREETYPE
> > 
> > ...
> > 
> > +#else
> > +    PRBool bold = PR_FALSE;
> > +    return static_cast<FontEntry*>(FindFontForStyle(aFontStyle, bold ));
> > +#endif
> > 
> > Should non-WIN_FREETYPE use FindFontForStyle too?
> 
> +#ifdef WIN_FREETYPE
> +        if (mVariations.Length() == 0)
> +            FindStyleVariations();
> 
> That bit of code in FindFontForStyle does roughly the same job, which is why
> the WIN_FREETYPE implementation of FindFontEntry can be so short.

What I'm asking is: why can't FindFontForStyle and
FontFamily::FindWeightsForStyle be used for non-WIN_FREETYPE too (instead
of having two very similar implementations)?
> --- a/gfx/cairo/cairo/src/Makefile.in
> +++ b/gfx/cairo/cairo/src/Makefile.in
> @@ -148,8 +148,14 @@
>  PS_EXPORTS = cairo-ps.h
> 
>  ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
> -CSRCS    +=     cairo-win32-font.c \
> -            cairo-win32-surface.c
> +CSRCS    +=    cairo-win32-surface.c
> +
> +ifdef WIN_FREETYPE
> +DEFINES +=    -DCAIRO_DISABLE_FONTCONFIG
> +CSRCS    +=     cairo-win32-ft-font.c 
> +else
> +CSRCS    +=     cairo-win32-font.c 
> +endif
> 
> Is it necessary to disable cairo-win32-font in order to enable freetype fonts?

The original motivation for using freetype on windows was to avoid compiling cairo-win32-font.c and gfxWindowsFonts.cpp on windows ce, because both use APIs that don't exist on windows ce.  

I could do something like:

ifdef WINCE
CSRCS    +=   cairo-wince-ft-font.c
else
CSRCS    +=   cairo-win32-font.c
endif

would that be better?
(In reply to comment #9)
> > --- a/gfx/cairo/cairo/src/Makefile.in
> > +++ b/gfx/cairo/cairo/src/Makefile.in
> > @@ -148,8 +148,14 @@
> >  PS_EXPORTS = cairo-ps.h
> > 
> >  ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
> > -CSRCS    +=     cairo-win32-font.c \
> > -            cairo-win32-surface.c
> > +CSRCS    +=    cairo-win32-surface.c
> > +
> > +ifdef WIN_FREETYPE
> > +DEFINES +=    -DCAIRO_DISABLE_FONTCONFIG
> > +CSRCS    +=     cairo-win32-ft-font.c 
> > +else
> > +CSRCS    +=     cairo-win32-font.c 
> > +endif
> > 
> > Is it necessary to disable cairo-win32-font in order to enable freetype fonts?
> 
> The original motivation for using freetype on windows was to avoid compiling
> cairo-win32-font.c and gfxWindowsFonts.cpp on windows ce, because both use
> APIs that don't exist on windows ce.

OK.

> I could do something like:
> 
> ifdef WINCE
> CSRCS    +=   cairo-wince-ft-font.c
> else
> CSRCS    +=   cairo-win32-font.c
> endif
> 
> would that be better?

Maybe WINCE is clearer, but I don't really mind which way you go here.  I see that cairo/src/Makefile.in is already a Mozilla-specific file.

My question was part of me trying to understand the role of cairo-win(32|ce)-ft-font.c and its cairo_win32_scaled_font_t.  I can imagine that someone may wish to install cairo with both win32 and freetype fonts.  I expect that works now, and would work fine simply by building without cairo-win(32|ce)-ft-font.c.

From what I have seen, cairo-win(32|ce)-ft-font.c is merely to satisfy the (function) symbols used by _cairo_win32_surface_show_glyphs, but those symbols are within a CAIRO_HAS_WIN32_FONT preprocessor conditional, so I'm not seeing the need for cairo-win(32|ce)-ft-font.c.
Great point. CAIRO_HAS_WIN32_FONT is currently set, but unsetting it removes the need for cairo-win(32|ce)-ft-font.c.  I'll fight with configure.in to unset it correctly and get a new patch up tonight.
Attached patch cut out cairo-wince-ft-fonts.c (obsolete) — Splinter Review
_cairo_ft_scaled_font_create_toy should be more sane, but we never hit that function during a run. So I've never seen it work (or fail).
Attachment #347650 - Attachment is obsolete: true
Attachment #347930 - Flags: superreview?(pavlov)
Attachment #347930 - Flags: review?(mozbugz)
Attachment #347650 - Flags: superreview?(pavlov)
Attachment #347650 - Flags: review?(mozbugz)
Comment on attachment 347930 [details] [diff] [review]
cut out cairo-wince-ft-fonts.c

+#ifdef CAIRO_DISABLE_FONTCONFIG
+# define FC_RGBA_UNKNOWN 0
+# define FC_RGBA_RGB         1
+# define FC_RGBA_BGR         2
+# define FC_RGBA_VRGB        3
+# define FC_RGBA_VBGR        4
+# define FC_RGBA_NONE        5

What are these needed for?
Aren't the relevant sections already #ifdef CAIRO_DISABLE_FONTCONFIG?

-DIRS += libpixman/src cairo/src

+
+DIRS += libpixman/src cairo/src 

whitespace after cairo/src

+#ifndef CAIRO_DISABLE_FONTCONFIG
 #include <fontconfig/fontconfig.h>
+#endif
 #include <ft2build.h>
 #include FT_FREETYPE_H
 
 CAIRO_BEGIN_DECLS
 
+#ifndef WIN_FREETYPE

CAIRO_DISABLE_FONTCONFIG

-    aLeading = ROUND_TO_TWIPS(GetMetrics().externalLeading);
+    gfxFont::Metrics met = GetMetrics();
+    aLeading = ROUND_TO_TWIPS(met.externalLeading);

This shouldn't be necessary.

+    static FontEntry* 
+    FontEntry::CreateFontEntry(const nsAString& aName, PRBool aItalic, PRUint16 aWeight, gfxUserFontData* aUserFontData, HDC hdc = 0, LOGFONTW *aLogFont = nsnull);

"FontEntry::" shouldn't be necessary.

+#ifdef DEBUG
+        NS_ASSERTION(!mBlocks.DebugGetHeader(), "mHdr is null, this is bad");
+#endif        

NS_ASSERTION tests are only compiled when DEBUG, so "#ifdef DEBUG" is
redundant here.
http://hg.mozilla.org/mozilla-central/annotate/a1364547a182/xpcom/glue/nsDebug.h#l170

+++ b/gfx/thebes/src/Makefile.in
+DEFINES	+=	-DCAIRO_DISABLE_FONTCONFIG

This shouldn't be necessary.  The cairo headers should take care of this.

+ifdef WIN_FREETYPE
+CXXFLAGS += $(CAIRO_FT_CFLAGS)
+EXTRA_DSO_LDOPTS += $(call EXPAND_LIBNAME,shell32)

I'm not familiar with shell32, but the headers in gfxFT2Fonts.cpp make me
suspect that this is only needed for WINCE.

 static cairo_status_t
 _cairo_ft_scaled_font_create_toy (cairo_toy_font_face_t	      *toy_face,
 				  const cairo_matrix_t	      *font_matrix,
 				  const cairo_matrix_t	      *ctm,
 				  const cairo_font_options_t  *font_options,
 				  cairo_scaled_font_t	     **font)
 {
+#ifndef CAIRO_DISABLE_FONTCONFIG


+#else
+    cairo_status_t status;
+    cairo_ft_scaled_font_t *f;
+    cairo_matrix_t scale;
+
+
+    f = malloc (sizeof(cairo_ft_scaled_font_t));
+    if (f == NULL)
+	return _cairo_error (CAIRO_STATUS_NO_MEMORY);
+
+    cairo_matrix_multiply (&scale, font_matrix, ctm);
+
+    status = _cairo_scaled_font_init (&f->base, &toy_face->base,
+				      font_matrix, ctm, font_options,
+				      &_cairo_ft_scaled_font_backend);
+    if (status)
+	goto FREE_SCALED_FONT;
+
+    *font = &f->base;
+    return CAIRO_STATUS_SUCCESS;
+
+ FREE_SCALED_FONT:
+    free (f);
+    return status;
+
+#endif  /* CAIRO_DISABLE_FONTCONFIG */

I don't really know what this will do, but it doesn't look like its creating a
cairo_ft_scaled_font_t (apart from the size of the allocation).  The best it
can hope for is that _cairo_scaled_font_init might create some fallback font
type.

But it would be better just to clearly indicate that toy fonts are not
supported.  I'm guessing the best way to do this is to remove
_cairo_ft_scaled_font_create_toy from _cairo_ft_scaled_font_backend, but if a
function must be there then returning an unsupported status.

(In reply to comment #12)
> _cairo_ft_scaled_font_create_toy should be more sane, but we never hit that
> function during a run. So I've never seen it work (or fail).

This won't make it upstream unless it's tested, so indicating lack of support
is better than hoping something works.

(Following up to comment #8)
> > >  FontEntry *
> > >  FontFamily::FindFontEntry(const gfxFontStyle& aFontStyle)
> > >  {
> > > +#ifndef WIN_FREETYPE
> > > 
> > > ...
> > > 
> > > +#else
> > > +    PRBool bold = PR_FALSE;
> > > +    return static_cast<FontEntry*>(FindFontForStyle(aFontStyle, bold ));
> > > +#endif
> > > 
> > > Should non-WIN_FREETYPE use FindFontForStyle too?
> > 
> > +#ifdef WIN_FREETYPE
> > +        if (mVariations.Length() == 0)
> > +            FindStyleVariations();
> > 
> > That bit of code in FindFontForStyle does roughly the same job, which is why
> > the WIN_FREETYPE implementation of FindFontEntry can be so short.
> 
> What I'm asking is: why can't FindFontForStyle and
> FontFamily::FindWeightsForStyle be used for non-WIN_FREETYPE too (instead
> of having two very similar implementations)?

Can you explain, please, why two completely separate implementations are
required?
this patch includes the patch for bug 465560 since they're so intertwined.
Attachment #347930 - Attachment is obsolete: true
Attachment #349354 - Flags: superreview?(pavlov)
Attachment #349354 - Flags: review?(mozbugz)
Attachment #347930 - Flags: superreview?(pavlov)
Attachment #347930 - Flags: review?(mozbugz)
Comment on attachment 347930 [details] [diff] [review]
cut out cairo-wince-ft-fonts.c

 const gfxFont::Metrics& nsThebesFontMetrics::GetMetrics() const
 {
-    return mFontGroup->GetFontAt(0)->GetMetrics();
+    static const gfxFont::Metrics nullMetrics;
+    gfxFont* f = mFontGroup->GetFontAt(0);
+    if (f)
+        return f->GetMetrics();
+    else
+        return nullMetrics;
 }


nsThebesFontMetrics.cpp:109: error: uninitialized const 'nullMetrics'

C++ doesn't provide an elegant way to default initialize (that I know of) but
this should work:

    static const gfxFont::Metrics nullMetrics = gfxFont::Metrics();

This can also be moved to within the "else" block.
Comment on attachment 349354 [details] [diff] [review]
one implementation of FindFontEntry

I've got as far as FontEntry::CreateFontEntry, but I don't yet understand
that.

+    FontEntry *fe = nsnull;
+    for (PRUint32 i = 0; i < ff->mFaces.Length(); ++i) {
+        fe = ff->mFaces[i];
+    }
+    
+    for (PRUint32 i = 0; i < ff->mFaces.Length(); ++i) {
+        fe = ff->mFaces[i];
+        // check if we already know about this face
+        if (fe->mWeight == logFont.lfWeight &&
+            fe->mItalic == (logFont.lfItalic == 0xFF)) {
+            return 1; 
+        }
+    }

Looks like the first loop is not needed.

+    LOGFONTW logFont;
+    PRBool needRelease = PR_FALSE;

+    if (!aLogFont) {
+        aLogFont = &logFont;
+    }
+
+    if (!hdc) {
+        hdc = GetDC(nsnull);
+#ifndef WINCE
+        SetGraphicsMode(hdc, GM_ADVANCED);
+#endif
+        needRelease = PR_TRUE;
+    }
+
+    HFONT font = CreateFontIndirectW(aLogFont);
+
+    if (font) {
+        HFONT oldFont = (HFONT)SelectObject(hdc, font);
+
+        SelectObject(hdc, oldFont);
+        DeleteObject(font);
+    }
+
+    if (needRelease)
+        ReleaseDC(nsnull, hdc);

Are there some side-effects in these hdc and logfont operations that are
important?  Or can this all be removed?

 /* Fontconfig/Freetype platform-specific font interface */
-
+#ifndef CAIRO_DISABLE_FONTCONFIG

Leave the newline here.

 #include <fontconfig/fontconfig.h>
+#endif
 #include <ft2build.h>
 #include FT_FREETYPE_H
 
 CAIRO_BEGIN_DECLS
 
+#ifndef CAIRO_HAS_FT_FONT
 cairo_public cairo_font_face_t *
 cairo_ft_font_face_create_for_pattern (FcPattern *pattern);

CAIRO_DISABLE_FONTCONFIG

     FontFamily(const nsAString& aName) :
         gfxFontFamily(aName) { }
 
     FontEntry *FindFontEntry(const gfxFontStyle& aFontStyle);
+protected:
+    virtual PRBool FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], const gfxFontStyle& aFontStyle);
+#ifdef WIN_FREETYPE
+private:
+    friend class gfxWindowsPlatform;
+
+    void FindStyleVariations();
+
+    static int CALLBACK FamilyAddStylesProc(const ENUMLOGFONTEXW *lpelfe,
+                                            const NEWTEXTMETRICEXW *nmetrics,
+                                            DWORD fontType, LPARAM data);
+#endif

I think a blank line before "protected:" and before "#ifdef WIN_FREETYPE"
would be more consistent with the blank lines between method declarations, and
make this more readable, particularly so because this file expects the editor
to wrap long lines.

                                       // equal to .aveCharWidth
-    };
+    };    
     virtual const gfxFont::Metrics& GetMetrics() = 0;

Don't add the trailing whitespace.

     virtual gfxFont *GetFontAt(PRInt32 i) {
-        return static_cast<gfxFont*>(mFonts[i]);
+        if (mFonts.Length() > i) 
+            return static_cast<gfxFont*>(mFonts[i]);
+        else
+            return NULL;
     }

Doesn't the comparison of PRUint32 with PRInt32 produce a compiler warning?

I don't know why GetFontAt() takes a signed argument.  There is an assumption
here that i is non-negative, so an explicit "PRUint32(i)" seems the best way
to suppress the warning.

Similarly in gfxFT2Font.h

-        if (mUnderlineOffset == UNDERLINE_OFFSET_NOT_SET)
+        if (mUnderlineOffset == UNDERLINE_OFFSET_NOT_SET && FontListLength() > 0)
             mUnderlineOffset = GetFontAt(0)->GetMetrics().underlineOffset;

I think what you really want to test here is that GetFontAt(0) returns
non-NULL.

+    gfxFontGroup *CreateFontGroup(const nsAString &aFamilies,
+                                  const gfxFontStyle *aStyle);
+ 

This doesn't appear to used.  Remove.

+    return static_cast<FontEntry*>(FindFontForStyle(aFontStyle, bold ));

Remove space after "bold".

And I think keeping the name "needsBold" would be clearer.

+        FcFontSet* fs = FcFontSetCreate ();

+        if (match)
+            FcFontSetAdd (fs, match);

+        FcPatternGetString (fs->fonts[0], FC_FAMILY, 0, &family);

+        FcFontSetDestroy (fs);

if (match) {
    FcPatternGetString (match, FC_FAMILY, 0, &family);
}

+PRBool
+FontFamily::FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], const gfxFontStyle& aFontStyle)
+{
+    PRBool italic = (aFontStyle.style & (FONT_STYLE_ITALIC | FONT_STYLE_OBLIQUE)) != 0;
+    PRBool matchesSomething;
+
+    for (PRUint32 j = 0; j < 2; j++) {
+        matchesSomething = PR_FALSE;

Initialization at construction is preferred (and means one write instead of
two in this situation).

PRBool matchesSomething = PR_FALSE;

And can this method definition (and perhaps the other FontFamily methods)
instead be positioned in the file together with the existing FontFamily method
definition?  Putting it before FontFamily::FindFontEntry may even reduce the
number lines of blame changes.

(Following up to comment #13)

> +++ b/gfx/thebes/src/Makefile.in
> +DEFINES    +=    -DCAIRO_DISABLE_FONTCONFIG
> 
> This shouldn't be necessary.  The cairo headers should take care of this.

The issue here is that it is the cairo library that knows whether the
fontconfig-dependent functions in cairo-ft.h are available.  It shouldn't be
the user of the library that determines which functions are declared in
cairo's header file.

I think the best way to have the cairo headers indicate which functions are
available is to use cairo-features.h.  See CAIRO_HAS_PNG_FUNCTIONS for an
example that is similar to CAIRO_DISABLE_FONTCONFIG (which makes me wonder
whether CAIRO_HAS_FONTCONFIG_FUNCTIONS or similar may be a better name).

> But it would be better just to clearly indicate that toy fonts are not
> supported.  I'm guessing the best way to do this is to remove
> _cairo_ft_scaled_font_create_toy from _cairo_ft_scaled_font_backend, but if a
> function must be there then returning an unsupported status.

 const cairo_scaled_font_backend_t _cairo_ft_scaled_font_backend = {
     CAIRO_FONT_TYPE_FT,
     NULL,
+#ifdef CAIRO_DISABLE_FONTCONFIG
+    NULL,
+#else
     _cairo_ft_scaled_font_create_toy,
+#endif

Looks like cairo_scaled_font_backend_t::create_toy can't be NULL

http://hg.mozilla.org/mozilla-central/annotate/951178f392ef/gfx/cairo/cairo/src/cairo-font-face.c#l620

so _cairo_ft_scaled_font_create_toy will need to exist and return
CAIRO_INT_STATUS_UNSUPPORTED.
thanks for the constructive reviews.  I think this fixes everything you pointed out.
Attachment #349354 - Attachment is obsolete: true
Attachment #350239 - Flags: superreview?(pavlov)
Attachment #350239 - Flags: review?(mozbugz)
Attachment #349354 - Flags: superreview?(pavlov)
Attachment #349354 - Flags: review?(mozbugz)
Attachment #350239 - Flags: review?(mozbugz)
Comment on attachment 350239 [details] [diff] [review]
updated based on karlt's comments

There was some discussion on "WIN_FREETYPE" in bug 463532 comment 6 and subsequent, and we
also discussed this on irc.

One issue I have with WIN_FREETYPE is that it sounds like a windows-specific
version of FreeType (which it is not).

Also this currently implies 3 different things:
1) we're building FreeType in the tree
2) gfxFT2Fonts is used
3) fonts are found using windows APIs instead of fontconfig.

I suggest using MOZ_TREE_FREETYPE for 1, MOZ_FT2_FONTS (or MOZ_FONTGROUP_TYPE)
for 2, and a platform test, probably MOZ_WIDGET_WIN32, for 3.

+++ b/gfx/cairo/Makefile.in

+ifdef WIN_FREETYPE
+DIRS += $(DEPTH)/modules/freetype2
+endif

Is this the way modules are normally pulled in?
I don't know the right way to do this.
Did you see something similar elsewhere?

+#ifdef CAIRO_DISABLE_FONTCONFIG
+    NULL,
+#else
     _cairo_ft_scaled_font_create_toy,
+#endif

Leave this code as it is/was.

+#ifndef CAIRO_HAS_FT_FONT
 cairo_public cairo_font_face_t *
 cairo_ft_font_face_create_for_pattern (FcPattern *pattern);
 
See comment 16.

         if (matchesSomething)
             break;
         italic = !italic;
+        if (j <1)
+            matchesSomething = PR_FALSE;

No need to set to false as this code won't be executed if it is true.

+        FcResult result;
+        FcPattern* pat = FcPatternCreate ();
+        FcFontSet* fs = FcFontSetCreate ();
+        FcPattern   *match;
+        match = FcFontMatch (0, pat, &result);
+        if (match)
+            FcFontSetAdd (fs, match);
+        FcChar8 *family;
+        FcPatternGetString (fs->fonts[0], FC_FAMILY, 0, &family);
+        familyArray.AppendString(NS_ConvertUTF8toUTF16((char*)family));
+        FcFontSetDestroy (fs);

See comment 16.

 gfxFT2FontGroup::AddRange(gfxTextRun *aTextRun, gfxFT2Font *font, const PRUnichar *str, PRUint32 len)
 {
+    if (!font)
+        return;

Callers of gfxTextRun::FindFirstGlyphRunContaining seem to expect the text run
to contain at least one glyph run.

+#ifndef WIN_FREETYPE
                         lsbDeltaNext = face->glyph->lsb_delta;
+#endif

+#ifndef WIN_FREETYPE
                 if (face->glyph->rsb_delta - lsbDeltaNext >= 32) {
                     advance -= 64;
                 } else if (face->glyph->rsb_delta - lsbDeltaNext < -32) {
                     advance += 64;
                 }
+#endif

Why not use this code?

+FontEntry::CreateFontEntry(const nsAString& aName, PRBool aItalic, 
+                           PRUint16 aWeight, gfxUserFontData* aUserFontData, 
+                           HDC /*hdc*/, LOGFONTW* /*aLogFont*/)

Why not remove the unused arguments?

 PRBool
 TextRunWordCache::CacheHashEntry::KeyEquals(const KeyTypePointer aKey) const
 {
     if (!mTextRun)
         return PR_FALSE;
+    if (!aKey)
+        return PR_FALSE;
 
     PRUint32 length = aKey->mLength;
     gfxFontGroup *fontGroup = mTextRun->GetFontGroup();
+    if (!fontGroup)
+        return PR_FALSE;

Are these changes addressing an issue that can be considered separately?
I'm surprised that a text run without a font group is a possibility.

+#include "freetype/freetype.h" 

The convention here is:

#include FT_FREETYPE_H

Is FT_FREETYPE_H defined for the in-tree FreeType?

+#ifndef WIN_FREETYPE
     if (aFontEntry->SupportsLangGroup(data->mLangGroup) &&
         aFontEntry->MatchesGenericFamily(data->mGenericFamily))
         data->mStringArray.AppendString(aFontFamily->mName);
-
+#endif
     return PL_DHASH_NEXT;

Wouldn't it be better to return every family name rather than no names?

 void
 gfxWindowsPlatform::InitBadUnderlineList()
 {
+#ifndef WIN_FREETYPE
     nsAutoTArray<nsString, 10> blacklist;
     gfxFontUtils::GetPrefsFontList("font.blacklist.underline_offset", blacklist);
     PRUint32 numFonts = blacklist.Length();
     for (PRUint32 i = 0; i < numFonts; i++) {
         PRBool aborted;
         nsAutoString resolved;
         ResolveFontName(blacklist[i], SimpleResolverCallback, &resolved, aborted);
         if (resolved.IsEmpty())
             continue;
         FontFamily *ff = FindFontFamily(resolved);
         if (!ff)
             continue;
         ff->mIsBadUnderlineFontFamily = 1;
     }
+#endif
 }

Add a comment to explain why this code is not suitable for gfxFT2Fonts.

 }
-
-already_AddRefed<gfxWindowsFont>
-gfxWindowsPlatform::FindFontForChar(PRUint32 aCh, gfxWindowsFont *aFont)
+already_AddRefed<gfxFont> gfxWindowsPlatform::FindFontForChar(PRUint32 aCh, gfxFont *aFont)

The blank line between function definitions and the type of the return value
on its own line is the file style.  Please keep to that.

 
+#ifndef WIN_FREETYPE
     if (data.bestMatch) {
         nsRefPtr<gfxWindowsFont> font =
             gfxWindowsFont::GetOrMakeFont(data.bestMatch, aFont->GetStyle());
         if (font->IsValid())
             return font.forget();
         return nsnull;
     }
-
+#endif
     // no match? add to set of non-matching codepoints
     mCodepointsWithNoFonts.set(aCh);
     return nsnull;

Why not check bestMatch?
This will mark any character as having no font.

If the code is removed because the function is not used then #ifndef the whole
function and FindFontForCharProc (and the declarations).



 GetNormalLineHeight(nsIFontMetrics* aFontMetrics)
 {
   NS_PRECONDITION(nsnull != aFontMetrics, "no font metrics");
+  if (nsnull == aFontMetrics)
+    return 0;

Is this an issue that can be considered separately?

+
+    #ifdef WIN_FREETYPE
+    FT_Init_FreeType(&gPlatformFTLibrary);
+    #endif

FT_Done_FreeType is not called, which I think is a reasonable way to work
around the issue mentioned in bug 458169 comment 15.  Can you add a comment,
please, explaining that FT_Done_FreeType is not called on shutdown because
cairo is still holding on to some of the FT_Faces from the FT_Library?
(In reply to comment #16)

>  #include <fontconfig/fontconfig.h>
> +#endif
>  #include <ft2build.h>
>  #include FT_FREETYPE_H
> 
>  CAIRO_BEGIN_DECLS
> 
> +#ifndef CAIRO_HAS_FT_FONT
>  cairo_public cairo_font_face_t *
>  cairo_ft_font_face_create_for_pattern (FcPattern *pattern);
> 
> CAIRO_DISABLE_FONTCONFIG
> 
>      FontFamily(const nsAString& aName) :
>          gfxFontFamily(aName) { }
> 
>      FontEntry *FindFontEntry(const gfxFontStyle& aFontStyle);
> +protected:
> +    virtual PRBool FindWeightsForStyle(gfxFontEntry* aFontsForWeights[], const
> gfxFontStyle& aFontStyle);
> +#ifdef WIN_FREETYPE
> +private:
> +    friend class gfxWindowsPlatform;
> +
> +    void FindStyleVariations();
> +
> +    static int CALLBACK FamilyAddStylesProc(const ENUMLOGFONTEXW *lpelfe,
> +                                            const NEWTEXTMETRICEXW *nmetrics,
> +                                            DWORD fontType, LPARAM data);
> +#endif
> 
> I think a blank line before "protected:" and before "#ifdef WIN_FREETYPE"
> would be more consistent with the blank lines between method declarations, and
> make this more readable, particularly so because this file expects the editor
> to wrap long lines.
(In reply to comment #18)

> +#ifndef CAIRO_HAS_FT_FONT
>  cairo_public cairo_font_face_t *
>  cairo_ft_font_face_create_for_pattern (FcPattern *pattern);
> 
> See comment 16.
> 

I guess I don't understand comment 16, could you clarify?
(In reply to comment #19)

The "#ifndef CAIRO_HAS_FT_FONT" should be "#ifndef CAIRO_DISABLE_FONTCONFIG" (or this code will never be compiled).
(In reply to comment #18)
> (From update of attachment 350239 [details] [diff] [review])
> There was some discussion on "WIN_FREETYPE" in bug 463532 comment 6 and
> subsequent, and we
> also discussed this on irc.
> 
> One issue I have with WIN_FREETYPE is that it sounds like a windows-specific
> version of FreeType (which it is not).
> 
> Also this currently implies 3 different things:
> 1) we're building FreeType in the tree
> 2) gfxFT2Fonts is used
> 3) fonts are found using windows APIs instead of fontconfig.
> 
> I suggest using MOZ_TREE_FREETYPE for 1, MOZ_FT2_FONTS (or MOZ_FONTGROUP_TYPE)
> for 2, and a platform test, probably MOZ_WIDGET_WIN32, for 3.

MOZ_WIDGET_WIN32 didn't exist, I added it to configure.in.  I don't think we need MOZ_FT2_FONTS, we can just use CAIRO_HAS_FT_FONTS (or a negative of CAIRO_HAS_WIN32_FONTS as appropriate).
Attachment #350239 - Attachment is obsolete: true
Attachment #351132 - Flags: superreview?(pavlov)
Attachment #351132 - Flags: review?(mozbugz)
Attachment #350239 - Flags: superreview?(pavlov)
Comment on attachment 351132 [details] [diff] [review]
updated based on comments up to #19

--- a/configure.in
+++ b/configure.in
@@ -1950,7 +1950,17 @@ case "$target" in
     LIBIDL_CFLAGS="-I$MOZ_TOOLS_DIR/include ${GLIB_CFLAGS}"
     LIBIDL_LIBS="$MOZ_TOOLS_DIR/lib/libidl-0.6_s.lib $MOZ_TOOLS_DIR/lib/glib-1.2_s.lib"
     STATIC_LIBIDL=1
-
+    MOZ_TREE_FREETYPE=1
+    MOZ_ENABLE_CAIRO_FT=1
+    FT_FONT_FEATURE="#define CAIRO_HAS_FT_FONT 1"
+    FC_FONT_FEATURE="#define CAIRO_DISABLE_FONTCONFIG 1"
+    WIN32_FONT_FEATURE=
+    FT2_CFLAGS="-I${topsrcdir}/modules/freetype2/include"
+    CAIRO_FT_CFLAGS="-I${topsrcdir}/modules/freetype2/include"
+    FT2_LIBS="${LIBXUL_DIST}/lib/freetype2.lib"
+    CAIRO_FT_LIBS = "${LIBXUL_DIST}/lib/freetype2.lib"
+    AC_SUBST(CAIRO_FT_CFLAGS)
+    AC_DEFINE(MOZ_TREE_FREETYPE)
     AC_DEFINE(HAVE_SNPRINTF)
     AC_DEFINE(_WINDOWS)
     AC_DEFINE(_WIN32)
@@ -5723,6 +5733,28 @@ fi
 fi
 
 dnl ========================================================
+dnl Build Freetype in the tree
+dnl ========================================================
+MOZ_ARG_ENABLE_BOOL(tree-freetype,
+[  --enable-tree-freetype         Enable Tree FreeType],
+    MOZ_TREE_FREETYPE=1,
+    MOZ_TREE_FREETYPE= )
+if test -n "$MOZ_TREE_FREETYPE"; then
+   AC_DEFINE(MOZ_TREE_FREETYPE)
+   AC_SUBST(MOZ_TREE_FREETYPE)
+   MOZ_ENABLE_CAIRO_FT=1       
+   FT_FONT_FEATURE="#define CAIRO_HAS_FT_FONT 1"
+   FC_FONT_FEATURE="#define CAIRO_DISABLE_FONTCONFIG 1"
+   FT2_CFLAGS="-I${topsrcdir}/modules/freetype2/include"
+   CAIRO_FT_CFLAGS="-I${topsrcdir}/modules/freetype2/include"
+   FT2_LIBS="${LIBXUL_DIST}/lib/freetype2.lib"
+   CAIRO_FT_LIBS = "${LIBXUL_DIST}/lib/freetype2.lib"
+   AC_SUBST(CAIRO_FT_CFLAGS)
+   AC_SUBST(CAIRO_FT_LIBS)
+fi

The goal here needs to be to reduce duplication.  The "*-wince*)" section
should just change the default for the --enable-tree-freetype arg, so I expect
MOZ_TREE_FREETYPE=1 should be enough.  It looks like the "WIN32_FONT_FEATURE="
should not be necessary.

Note, however, that configure sets various defines that are used by
cairo-ft-fonts.c, but currently configure only sets these when X11 is used.
HAVE_FT_BITMAP_SIZE_Y_PPEM HAVE_FT_GLYPHSLOT_EMBOLDEN HAVE_FT_LOAD_SFNT_TABLE
should all be set appropriately whenever FreeType is used (even if not using
X11).  I guess these just have to be explicitly defined when using tree
freetype (because the library is not built at the time configure runs.)

+++ b/gfx/Makefile.in
@@ -44,6 +44,8 @@ include $(DEPTH)/config/autoconf.mk
 
 MODULE		= gfx
 
+REQUIRES	+= freetype2
+

Should this only be added when MOZ_TREE_FREETYPE?

@@ -56,4 +58,9 @@ endif
 endif
 endif
 
+ifdef WINCE
+MOZ_TREE_FREETYPE	= 1
+DEFINES		+= -DMOZ_TREE_FREETYPE
+endif

configure should take care of this.

+++ b/gfx/cairo/cairo/src/Makefile.in
@@ -69,6 +69,7 @@ REQUIRES        = $(PNG_REQUIRES) \
 REQUIRES        = $(PNG_REQUIRES) \
                   $(ZLIB_REQUIRES) \
                   libpixman \
+		  freetype2 \

Follow the surrounding style for tabs,
but I think this should only be added when MOZ_TREE_FREETYPE
(and perhaps only when MOZ_FT2_FONTS).

+ifdef MOZ_TREE_FREETYPE
+EXPORTS +=	gfxFT2Fonts.h 
+else
+EXPORTS +=	gfxWindowsFonts.h 
+endif

MOZ_TREE_FREETYPE doesn't seem the right thing to test here.

+#include "cairo-features.h" // To get CAIRO_HAS_WIN32_FONT/CAIRO_HAS_FT_FONT
+
 #include "gfxFontUtils.h"
 #include "gfxWindowsSurface.h"
+#if defined(CAIRO_HAS_WIN32_FONT)
 #include "gfxWindowsFonts.h"
+#elif defined(CAIRO_HAS_FT_FONT)
+#include "gfxFT2Fonts.h"
+#else
+#error "Windows gfx platform needs a font backend"
+#endif
 #include "gfxPlatform.h"
 
 #include "nsVoidArray.h"
 #include "nsDataHashtable.h"
+
+#ifdef CAIRO_HAS_FT_FONT
+typedef struct FT_LibraryRec_ *FT_Library;
+#endif

The use of cairo preprocessor symbols here is going to cause problems when
using a system cairo with both win32 and ft fonts.

MOZ_FT2_FONTS (or MOZ_FONTGROUP_TYPE) seems to me the best solution to this,
but feel free to suggest something else if you have a better idea.
(In reply to comment #23)
> The goal here needs to be to reduce duplication.  The "*-wince*)" section
> should just change the default for the --enable-tree-freetype arg, so I expect
> MOZ_TREE_FREETYPE=1 should be enough.  It looks like the "WIN32_FONT_FEATURE="
> should not be necessary.
> 
I think we'd like to clear WIN32_FONT_FEATURE for wince since we don't want to build the windows font code under windows ce.  Please correct me if I'm wrong.

> Note, however, that configure sets various defines that are used by
> cairo-ft-fonts.c, but currently configure only sets these when X11 is used.
> HAVE_FT_BITMAP_SIZE_Y_PPEM HAVE_FT_GLYPHSLOT_EMBOLDEN HAVE_FT_LOAD_SFNT_TABLE
> should all be set appropriately whenever FreeType is used (even if not using
> X11).  I guess these just have to be explicitly defined when using tree
> freetype (because the library is not built at the time configure runs.)
> 
I believe this shoud all be set to true, correct?

> +++ b/gfx/Makefile.in
> @@ -44,6 +44,8 @@ include $(DEPTH)/config/autoconf.mk
> 
>  MODULE        = gfx
> 
> +REQUIRES    += freetype2
> +
> 
> Should this only be added when MOZ_TREE_FREETYPE?

yup

> The use of cairo preprocessor symbols here is going to cause problems when
> using a system cairo with both win32 and ft fonts.
> 
> MOZ_FT2_FONTS (or MOZ_FONTGROUP_TYPE) seems to me the best solution to this,
> but feel free to suggest something else if you have a better idea.

MOZ_FT2_FONTS works, I was just trying to avoid creating what looked like redundant macros.
(In reply to comment #23)
> +ifdef MOZ_TREE_FREETYPE
> +EXPORTS +=    gfxFT2Fonts.h 
> +else
> +EXPORTS +=    gfxWindowsFonts.h 
> +endif
> 
> MOZ_TREE_FREETYPE doesn't seem the right thing to test here.

does this sound better?

ifdef MOZ_TREE_FREETYPE
CPPSRCS	+= 	gfxFT2Fonts.cpp 
endif

ifndef WINCE
CPPSRCS	+= 	gfxWindowsFonts.cpp 
endif


if we are building freetype in the tree, we want to use it so we should build gfxFT2Fonts.cpp.  Separately, gfxWindowsFonts.cpp doesn't compile for windows ce so don't build it.
(In reply to comment #24)
> (In reply to comment #23)
> > The goal here needs to be to reduce duplication.  The "*-wince*)" section
> > should just change the default for the --enable-tree-freetype arg, so I expect
> > MOZ_TREE_FREETYPE=1 should be enough.  It looks like the "WIN32_FONT_FEATURE="
> > should not be necessary.
> > 
> I think we'd like to clear WIN32_FONT_FEATURE for wince since we don't want to
> build the windows font code under windows ce.  Please correct me if I'm wrong.

I got the impression that this defaulted to off.  It's probably easiest just to test by trying only:

-        WIN32_FONT_FEATURE="#define CAIRO_HAS_WIN32_FONT 1"
+        if test -z "$WINCE"; then
+           WIN32_FONT_FEATURE="#define CAIRO_HAS_WIN32_FONT 1"
+        fi  

> > Note, however, that configure sets various defines that are used by
> > cairo-ft-fonts.c, but currently configure only sets these when X11 is used.
> > HAVE_FT_BITMAP_SIZE_Y_PPEM HAVE_FT_GLYPHSLOT_EMBOLDEN HAVE_FT_LOAD_SFNT_TABLE
> > should all be set appropriately whenever FreeType is used (even if not using
> > X11).  I guess these just have to be explicitly defined when using tree
> > freetype (because the library is not built at the time configure runs.)
> > 
> I believe this shoud all be set to true, correct?

That sounds right.
(In reply to comment #25)
> does this sound better?
> 
> ifdef MOZ_TREE_FREETYPE
> CPPSRCS    +=     gfxFT2Fonts.cpp 
> endif
> 
> ifndef WINCE
> CPPSRCS    +=     gfxWindowsFonts.cpp 
> endif
> 
> if we are building freetype in the tree, we want to use it so we should build
> gfxFT2Fonts.cpp.  Separately, gfxWindowsFonts.cpp doesn't compile for windows
> ce so don't build it.

Though we don't want to build gfxWindowsFonts.cpp if building with --enable-freetype-fonts (or similar) on non-WINCE platforms.

The key issue is that the choice of system/tree FreeType is really independent of the choice of gfxFontGroup implementation, especially on platforms that often have FreeType installed such as Qt/Linux.
this patch compiles/exports gfxWindowsFonts or gfxFT2Fonts based on WINCE being defined
Attachment #351132 - Attachment is obsolete: true
Attachment #351854 - Flags: superreview?(pavlov)
Attachment #351854 - Flags: review?(mozbugz)
Attachment #351132 - Flags: superreview?(pavlov)
Attachment #351132 - Flags: review?(mozbugz)
Attachment #351854 - Flags: review?(mozbugz)
Comment on attachment 351854 [details] [diff] [review]
updated, mostly build system changes

+if test -n "$MOZ_TREE_FREETYPE"; then

+   HAVE_FT_BITMAP_SIZE_Y_PPEM = 1
+   HAVE_FT_GLYPHSLOT_EMBOLDEN = 1
+   HAVE_FT_LOAD_SFNT_TABLE = 1

These need AC_DEFINE to generate the preprocessor symbols afaict.

+   AC_SUBST(CAIRO_FT_LIBS)

I don't think this will do anything as CAIRO_FT_LIBS is not in
autoconf.mk.in (and it probably doesn't need to do anything).

But, I'm confused about how this works with _WIN32_MSVC as CAIRO_FT_LIBS is
only added to MOZ_CAIRO_LIBS when not _WIN32_MSVC?

On the other hand, it's weird how autoconf does the AC_SUBST for FT2_CFLAGS
and FT2_LIBS because of the AC_SUBST in the MOZ_ENABLE_GTK2 and !MOZ_PANGO
conditional.  I think this is more obvious if AC_SUBST statements are all
outside the conditionals, but there are already a few examples in configure.in
where this is not the case...

 	$(NULL)
 
 
+
+
 ifeq ($(MOZ_WIDGET_TOOLKIT),windows)

No more blank lines, please.

+ifdef CAIRO_HAS_FT_FONT
+CXXFLAGS += $(CAIRO_FT_CFLAGS)
+ifdef WINCE
+EXTRA_DSO_LDOPTS += $(call EXPAND_LIBNAME,shell32)
+endif
+endif

AFAICT CAIRO_HAS_FT_FONT is only a C/C++ preprocessor variable, which is
correct I think, but that means that it's not defined here.
So I'm wondering whether any of this is needed?

But I think what you want is to add FT2_CFLAGS to CXXFLAGS when using
gfxFT2Fonts.cpp.

+        FcPattern   *match = FcFontMatch (0, pat, &result);

Remove extra spaces.
Use nsnull instead of 0 for pointers to make the type of the parameter
clearer.

+        if (match)
+            FcPatternGetString (match, FC_FAMILY, 0, &family);
+        familyArray.AppendString(NS_ConvertUTF8toUTF16((char*)family));

check for null family, before familyArray.AppendString.

-    if (fe->mWindowsFamily == data->fontToMatch->GetFontEntry()->mWindowsFamily)
+    if (fe->mWindowsFamily == static_cast<FontEntry*>(data->fontToMatch->GetFontEntry())->mWindowsFamily)
         rank += 3;
-    if (fe->mWindowsPitch == data->fontToMatch->GetFontEntry()->mWindowsFamily)
+    if (fe->mWindowsPitch == static_cast<FontEntry*>(data->fontToMatch->GetFontEntry())->mWindowsFamily)
         rank += 3;

It would be nice to use a temporary pointer and a single
static_cast<FontEntry*>(data->fontToMatch->GetFontEntry()).

Comparing mWindowsFamily to mWindowsPitch must be wrong (but I see that bug
already existed).

+#ifndef WINCE
 already_AddRefed<gfxWindowsFont>
 gfxWindowsPlatform::FindFontForChar(PRUint32 aCh, gfxWindowsFont *aFont)
+#else
+already_AddRefed<gfxFont>
+gfxWindowsPlatform::FindFontForChar(PRUint32 aCh, gfxFont *aFont)
+#endif

Is there a reason why non-WINCE must have a gfxWindowsFont* arg or return a
gfxWindowsFont reference?  Could both WINCE and non-WINCE just use gfxFont?
Comment on attachment 351854 [details] [diff] [review]
updated, mostly build system changes

I'd like Stuart to review GetFileNameForFont, FamilyAddStylesProc, and
FindStyleVariations as he knows much more about what is going on here, but I
have some comments:

+    #ifdef MOZ_FT2_FONTS
+    FT_Init_FreeType(&gPlatformFTLibrary);
+    #endif

File style seems to be no indentation of preprocessor directives.

+static nsCString GetFileNameForFont(FontFamily *ff) {   

Using an nsACString out parameter instead of the nsCString return value would
save a string allocation and copy.

+    wchar_t pathBuf[256];

Can MAX_PATH be used here?

+        if (!Substring(keyName, 0,fontName.Length()).Equals(fontName))

StringBeginsWith would do the same thing here, but is there a terminator to
distinguish keys when one keyName begins with another?

+    searchPaths.AppendElement(nsDependentString(pathBuf));

nsString has a constructor from PRUnichar*, so I expect AppendElement(pathBuf)
would work fine here.

+                NS_ConvertUTF16toUTF8 compareString(Substring(leafName, 0, 
+                                                              fontName.Length()));
+                ToLowerCase(compareString);
+                if (compareString.Equals(fontName)) {

Comparing with ff->Name() instead of fontName would save some conversions.

+    NS_ASSERTION(0, "couldn't find file for font");

NS_NOTREACHED may be more appropriate here, but these macros are really for
things that shouldn't happen.  I expect removal of font files is something
that could happen, so NS_WARNING is probably better.
(In reply to comment #30)
> 
> +    NS_ASSERTION(0, "couldn't find file for font");
> 
> NS_NOTREACHED may be more appropriate here, but these macros are really for
> things that shouldn't happen.  I expect removal of font files is something
> that could happen, so NS_WARNING is probably better.

This really shouldn't happen.  We assume there is at least one font in a font group in a lot of places by doing things like fontgroup.FontAt(0)->GetMetrics(), so returning a fontgroup with zero fonts will lead to a crash.

NS_NOTREACHED may be more apropriate, is the difference between NS_NOTREACHED and NS_ASSERTION documented anywhere?
(In reply to comment #30)
> +        if (!Substring(keyName, 0,fontName.Length()).Equals(fontName))
> 
> StringBeginsWith would do the same thing here, but is there a terminator to
> distinguish keys when one keyName begins with another?
 
StringBeginsWith should work, I'll try it.

There's no terminator, so this code will hit false positives.  I don't see any way around that though give the wince api's.  

Alternatively I could open every font file found and read in its font name, that seems like it would be really slow though.
(In reply to comment #31)
> (In reply to comment #30)
> > 
> > +    NS_ASSERTION(0, "couldn't find file for font");
> > 
> > NS_NOTREACHED may be more appropriate here, but these macros are really for
> > things that shouldn't happen.  I expect removal of font files is something
> > that could happen, so NS_WARNING is probably better.
> 
> This really shouldn't happen.  We assume there is at least one font in a font
> group in a lot of places by doing things like
> fontgroup.FontAt(0)->GetMetrics(), so returning a fontgroup with zero fonts
> will lead to a crash.

ehh...I was looking at the wrong part of the code, so please ignore comment #31, NS_WARNING is more appropriate.
(In reply to comment #32)
> There's no terminator, so this code will hit false positives.  I don't see any
> way around that though give the wince api's.  

OK.

> Alternatively I could open every font file found and read in its font name,
> that seems like it would be really slow though.

Yes, that would be inconvenient.  On WINCE, if there are only a few font files, then perhaps it wouldn't be too slow, so it may be option to consider if/when false positives become a problem.
Attached patch addressess points in comment #30 (obsolete) — Splinter Review
rewrote the font name comparison to use nsCaseInsensitiveStringComparator
Attachment #351854 - Attachment is obsolete: true
Attachment #353571 - Flags: superreview?
Attachment #353571 - Flags: review?(mozbugz)
Attachment #351854 - Flags: superreview?(pavlov)
Attachment #353571 - Flags: superreview? → superreview?(pavlov)
Comment on attachment 353571 [details] [diff] [review]
addressess points in comment #30

+   HAVE_FT_BITMAP_SIZE_Y_PPEM = 1
+   HAVE_FT_GLYPHSLOT_EMBOLDEN = 1
+   HAVE_FT_LOAD_SFNT_TABLE = 1

These shell variables are not required, only the preprocessor symbols through
AC_DEFINE.

+++ b/gfx/thebes/public/Makefile.in

 REQUIRES	= cairo \
 			unicharutil \
+			freetype2 \

Only add freetype2 when MOZ_TREE_CAIRO and WINCE

+        if (mFonts.Length() > i)

PRUint32(i)

+++ b/gfx/thebes/src/Makefile.in

 REQUIRES = \
 	cairo \
 	string \
 	pref \
 	xpcom \
 	unicharutil \
+	freetype2 \

only when MOZ_TREE_CAIRO and WINCE

+ifdef MOZ_TREE_FREETYPE
+LOCAL_INCLUDES	= -I$(topsrcdir)/modules/freetype2/include
+endif

REQUIRES should provide this.

+    FontEntry* mfe = static_cast<FontEntry*>(data->fontToMatch->GetFontEntry());

This is only used ifndef WINCE, so it should be inside the preprocessor
conditional, preferably close to where it is used.

+#ifndef WINCE
         nsRefPtr<gfxWindowsFont> font =
             gfxWindowsFont::GetOrMakeFont(data.bestMatch, aFont->GetStyle());
-        if (font->IsValid())
-            return font.forget();
+        if (font->IsValid()) {
+            gfxFont* ret = font.forget().get();
+            return already_AddRefed<gfxFont>(ret);
+        }
+#elif defined(MOZ_FT2_FONTS)
+        nsRefPtr<gfxFT2Font> font =
+            gfxFT2Font::GetOrMakeFont(data.bestMatch->mName, aFont->GetStyle()); 
+        if (font.get())
+            return static_cast<already_AddRefed<gfxFont>>(font);

The MOZ_FT2_FONTS block doesn't make sense to me.  I think this should be
similar to the WINCE block.  There's no need to test "font" for non-NULL as
the nsRefPtr stuff will handle NULL.
Attachment #353571 - Flags: review?(mozbugz)
Getting the filename is the major issue here.

One problem is that the same filename is used for all faces in a family, and so a bold/italic font may get used instead of regular.

+    ff->mFaces.AppendElement(fe);
+	
+    fe->mFilename = GetFileNameForFont(ff);

The problem could be resolved on NT by looking up lfFaceName instead of the
family name in the registry, and either making the assumption that shorter face
names appear first or by checking for "=" or " (" after the face name.

http://support.microsoft.com/kb/102960

But the problem is worse on WINCE.  The big assumption here is that font file
names begin with the family name.  While this may be true for some fonts, it
is very often not true.

Is there a standard set of fonts with WINCE, or do manufacturers each provided
different sets of fonts?
And may users install other fonts?

One option is opening each font file and creating a database (as suggested in
comment 32) instead of using EnumFontFamiliesExW.  This would perhaps work OK
if there are not too many fonts.

The other option is to avoid filenames altogether and create FT_Faces that use
GetFontData with dwTable = 0 to get the information when necessary:

http://msdn.microsoft.com/en-us/library/aa911428.aspx

Although the documentation says "If an application attempts to use this
function to retrieve information for a non-TrueType font, an error occurs",
the same is said in the NT documentation

http://msdn.microsoft.com/en-us/library/ms534211(VS.85).aspx

and cairo uses this successfully, even with Type 1 fonts

http://hg.mozilla.org/mozilla-central/annotate/0e61b5765cd3/gfx/cairo/cairo/src/cairo-win32-font.c#l915

The FT_Faces would be created using FT_Open_Face where the FT_Open_Args would
point to a FT_Stream implementation provided by Mozilla to use GetFontData.

http://freetype.sourceforge.net/freetype2/docs/reference/ft2-base_interface.html#FT_Open_Face

http://freetype.sourceforge.net/freetype2/docs/reference/ft2-base_interface.html#FT_Open_Args

http://freetype.sourceforge.net/freetype2/docs/reference/ft2-system_interface.html#FT_StreamRec
(In reply to comment #36)

> +ifdef MOZ_TREE_FREETYPE
> +LOCAL_INCLUDES    = -I$(topsrcdir)/modules/freetype2/include
> +endif
> 
> REQUIRES should provide this.

REQUIRES provides $(objdir)/dist/include/freetype2.  Currently, we don't copy the headers from freetype over to the dist dir, so we need to point to them in the srcdir.  

Also, I'll be removing freetype from REQUIRES entirely since its not needed.
(In reply to comment #38)
> REQUIRES provides $(objdir)/dist/include/freetype2.  Currently, we don't copy
> the headers from freetype over to the dist dir, so we need to point to them in
> the srcdir.  

OK.  Sorry, I should have read bug 463532 comment 7 to 11.

> Also, I'll be removing freetype from REQUIRES entirely since its not needed.

Is there another mechanism to ensure that the freetype library is built before the thebes shared library (with --disable-libxul), or is REQUIRES needed for this?
(In reply to comment #39)
> 
> > Also, I'll be removing freetype from REQUIRES entirely since its not needed.
> 
> Is there another mechanism to ensure that the freetype library is built before
> the thebes shared library (with --disable-libxul), or is REQUIRES needed for
> this?

I struggled with this for a while actually.  The secret sauce is to put freetype into tier_external_dirs, which essentially are modules that mozilla depends on but don't depend on anything else.  They are the first set of things to get built after nspr and js.

see:
http://mxr.mozilla.org/mozilla-central/source/toolkit/toolkit-tiers.mk#277

I don't think gfx is ever built without toolkit.  If it is though, it'll need some extra love.
This patch reads in all of the truetype font files found in the search path and maps their faces into FontFamilies and FontEntries.  Note that there is a fair amount of dead code in gfxFT2Font.cpp that still needs to be cleaned up.  Also, the font rendering looks "heavy" now.  I'm not sure if we're mistakenly bolding the fonts or if that's an artifact of the high dpi screen that's just working correctly now.
Attached patch cleaned up patch (obsolete) — Splinter Review
Attachment #354545 - Attachment is obsolete: true
Attachment #354597 - Flags: review?(mozbugz)
Comment on attachment 354597 [details] [diff] [review]
cleaned up patch

+   HAVE_FT_BITMAP_SIZE_Y_PPEM = 1
+   HAVE_FT_GLYPHSLOT_EMBOLDEN = 1
+   HAVE_FT_LOAD_SFNT_TABLE = 1

Don't forget to remove these.

+++ b/gfx/cairo/cairo/src/Makefile.in

 ifdef MOZ_TREE_CAIRO
+CXXFLAGS += $(CAIRO_FT_CFLAGS)
 DEFINES += -DMOZ_TREE_CAIRO

I can't think why this change would be needed.

+++ b/gfx/thebes/public/Makefile.in

 REQUIRES	= cairo \
 			unicharutil \
+			freetype2 \

I assume this should be removed also.

+DEFINES += 	MOZ_FT2_FONTS

Is this actually necessary for exporting header files?

+    static FontEntry* CreateFontEntryFromFile(char *fileName, long faceNum);

Only add this for WINCE.

+#ifdef XP_WIN
+    static FontEntry* 
+    CreateFontEntry(const nsAString& aName, PRBool aItalic, PRUint16 aWeight, gfxUserFontData* aUserFontData);
+#endif

I don't think this method is serving any useful purpose.  The resulting
FontEntry would not have a filename and there is no way to find the filename.

+        if (mFonts.Length() > i)

PRUint32(i)

+    void AppendFacesFromFontFile(const PRUnichar *aFileName);
+    void FindFonts();

These should be private.

+++ b/gfx/thebes/src/Makefile.in

 REQUIRES = \
 	cairo \
 	string \
 	pref \
 	xpcom \
 	unicharutil \
+	freetype2 \

Unnecessary?

+ifdef MOZ_TREE_FREETYPE
+LOCAL_INCLUDES	= -I$(topsrcdir)/modules/freetype2/include
+endif

FT2_CFLAGS will include this when MOZ_TREE_FREETYPE or other appropriate flags
when not MOZ_TREE_FREETYPE, so I think it would be better to add FT2_CFLAGS to
CXXFLAGS when using gfxFT2Fonts.cpp

+    fe->mFilename = strdup(fileName); 

FontEntry::mFilename is a nsCString, which will copy fileName, so the strdup
is a leak.

+    FT_Face face;
+    FT_New_Face(gfxToolkitPlatform::GetPlatform()->GetFTLibrary(),
+                fileName, faceNum, &face);

+    fe->mFontFace = cairo_ft_font_face_create_for_ft_face(face, 0);

The return value from FT_New_Face should be checked or face may be
undefined.  FontEntry::CairoFontFace() already has the same bug.

+    if ( face->style_flags & FT_STYLE_FLAG_BOLD)
+        fe->mWeight = 400;
+    else
+        fe->mWeight = 200;

These numbers are CSS weights and so should be 700 (bold) and 400 (normal)
(which explains why normal is selecting the bold face).

+    FontEntry *fe;
+
+    fe = new FontEntry(aName);
+        fe->mUserFontData = aUserFontData;

The first two lines of code can be merged into one.
Looks like there might be a tab on the third.

+        FcPattern* pat = FcPatternCreate ();
+        FcPattern *match = FcFontMatch (nsnull, pat, &result);
+        if (match)
+            FcPatternGetString (match, FC_FAMILY, 0, &family);

File style doesn't have spaces between function names and arguments.

+    FT_Face dummy;
+    FT_New_Face(gPlatformFTLibrary, 
+                fileName, -1, &dummy);
+    for (long i = 0; i < dummy->num_faces; i++) {

Check the return value from FT_New_Face.

Might as well use "FT_Long i;" to make it clearer that the choice of type
matches dummy->num_faces.

+    nsTArray<nsString> searchPaths(3);

+    SHGetSpecialFolderPathW(0, pathBuf, CSIDL_WINDOWS, 0);
+    searchPaths.AppendElement(pathBuf);
+    SHGetSpecialFolderPathW(0, pathBuf, CSIDL_FONTS, 0);
+    searchPaths.AppendElement(pathBuf);

searchPaths(2) ?

+    for (int i = 0;  i < searchPaths.Length(); i++) {
+        nsString path(searchPaths[i]);

Avoid the copy by using const nsString&.

+            nsString pattern(path);

nsAutoString instead of nsString may avoid an allocation.

+            HANDLE rv = FindFirstFileW(pattern.get(), &results);

Is rv a conventional name for a HANDLE?
If not better name it differently, as rv is normally used for returning
nsresults from functions.

+    FontEntry* mfe =
static_cast<FontEntry*>(data->fontToMatch->GetFontEntry());

This is only used ifndef WINCE, so it should be inside the preprocessor
conditional, preferably close to where it is used.

+#ifndef WINCE
         nsRefPtr<gfxWindowsFont> font =
             gfxWindowsFont::GetOrMakeFont(data.bestMatch, aFont->GetStyle());
-        if (font->IsValid())
-            return font.forget();
+        if (font->IsValid()) {
+            gfxFont* ret = font.forget().get();
+            return already_AddRefed<gfxFont>(ret);
+        }
+#elif defined(MOZ_FT2_FONTS)
+        nsRefPtr<gfxFT2Font> font =
+            gfxFT2Font::GetOrMakeFont(data.bestMatch->mName,
aFont->GetStyle()); 
+        if (font.get())
+            return static_cast<already_AddRefed<gfxFont>>(font);

The MOZ_FT2_FONTS block doesn't make sense to me.  I think this should be
similar to the ndef WINCE block.  There's no need to test "font" for non-NULL
as the nsRefPtr stuff will handle NULL.

@@ -557,23 +666,28 @@ FindFullNameForFace(const ENUMLOGFONTEXW
 
+#ifndef WINCE
     gfxWindowsFontType feType = FontEntry::DetermineFontType(metrics, fontType);
-
     data->mFontEntry = FontEntry::CreateFontEntry(data->mFamilyName, feType, (logFont.lfItalic == 0xFF), (PRUint16) (logFont.lfWeight), nsnull, data->mDC, &logFont);
+#elif defined(MOZ_FT2_FONTS)
+    data->mFontEntry = FontEntry::CreateFontEntry(data->mFamilyName, (logFont.lfItalic == 0xFF), (PRUint16) (logFont.lfWeight), nsnull);

MOZ_FT2_FONTS doesn't have a way to find the filename so this isn't going to
be useful.  Better to #ifdef out the functions that won't work.

@@ -857,27 +976,30 @@ gfxWindowsPlatform::MakePlatformFont(con
 
     FontEntry *fe = FontEntry::CreateFontEntry(uniqueName, 
+#ifndef WINCE
         gfxWindowsFontType(isCFF ? GFX_FONT_TYPE_PS_OPENTYPE : GFX_FONT_TYPE_TRUETYPE) /*type*/, 
+#endif
         PRUint32(aProxyEntry->mItalic ? FONT_STYLE_ITALIC : FONT_STYLE_NORMAL), 
         w, winUserFontData);

Similarly here, this is not going to be useful because winUserFontData is not
useful to MOZ_FT2_FONTS.
Attachment #354597 - Flags: review?(mozbugz)
Note that EnumFontFamiliesExW is used to standardize family names but the font
database uses FreeType to get family names from fonts.

Perhaps that is OK because the family names from FreeType are tried before
EnumFontFamiliesExW, but it would be worth doing some testing on a non-English
system with fonts that have a family name translation for the system language.
Attached patch updated on last commens (obsolete) — Splinter Review
For the sections of code where I've cut out CreateFontEntry, I'm making the assumption that we've already read in all of the fonts.  For LookupLocalFont, instead of enumerating the faces of a font family using windows APIs, I'm emumerating the mFaces in a gfxFontFamily.  In MakePlatformFont, I'm calling LookupLocalFont.
Attachment #354597 - Attachment is obsolete: true
Attachment #354670 - Flags: review?(mozbugz)
> MOZ_FT2_FONTS doesn't have a way to find the filename so this isn't going to
> be useful.  Better to #ifdef out the functions that won't work.


Also, we do have a mapping of font name to font files in the various font entries.
Attachment #354670 - Flags: review?(mozbugz)
Comment on attachment 354670 [details] [diff] [review]
updated on last commens

     return status;
+#endif  /* CAIRO_DISABLE_FONTCONFIG */
+    return CAIRO_INT_STATUS_UNSUPPORTED;

Better to use an #else here, or there may be compiler warnings about
unreachable code.

+#ifdef XP_WIN
+private:
+    friend class gfxWindowsPlatform;
+#endif
+

Looks like this isn't needed.

+#elif defined(XP_WIN)
+#ifdef WINCE
+#include <wingdi.h>
+#include <shellapi.h>
+#include <shlwapi.h>
+#define SHGetSpecialFolderPathW SHGetSpecialFolderPath
+#include "nsUnicharUtils.h"
+#endif
+#include "gfxWindowsPlatform.h"
+#define gfxToolkitPlatform gfxWindowsPlatform
+#include "nsIWindowsRegKey.h"
+#include "shlobj.h"
+#include "nsLocalFile.h"

Which of these are still needed?

+#endif
+
 
 private:

Unnecessary blank line.

+ifdef MOZ_TREE_FREETYPE
+CXXFLAGS += $(FT2_CFLAGS)
+endif

This is needed even when not MOZ_TREE_FREETYPE, so move this to the same block
that adds gfxFT2Fonts.cpp.

+    if (FT_New_Face(gfxToolkitPlatform::GetPlatform()->GetFTLibrary(),
+                    fileName, faceNum, &face))
+        return nsnull;

This is a bit confusing, as it looks like a test for success.

I'd prefer that it was clearer that FT_New_Face returns an error code.  This
can be done by explicitly comparing the result of FT_New_Face with FT_Err_Ok.

+    if (!FT_New_Face(gPlatformFTLibrary, 
+                     fileName, -1, &dummy))
+    for (FT_Long i = 0; i < dummy->num_faces; i++) {

+    }
+    FT_Done_Face(dummy);

Here also, and the indentation is quite unconventional, and FT_Done_Face should
not be called when FT_New_Face fails.  An early return seems simplest.

+}
+
+
+PRBool

Unnecessary blank line.

+    nsCaseInsensitiveStringComparator comparer;

unused.

+        nsRefPtr<gfxFT2Font> font =
+            gfxFT2Font::GetOrMakeFont(data.bestMatch->mName, 
+                                      aFont->GetStyle()); 
+        return static_cast<already_AddRefed<gfxFont>>(font);

I still don't understand what the static_cast from nsRefPtr<gfxFT2Font> to
already_AddRefed<gfxFont> is doing.  Is there an intermediate conversion
happening?  The nsRefPtr<gfxFT2Font> must "forget" that is owns the font so
that it doesn't unref.

     gfxWindowsFontType feType = FontEntry::DetermineFontType(metrics, fontType);
-
     data->mFontEntry = FontEntry::CreateFontEntry(data->mFamilyName, feType, (logFont.lfItalic == 0xFF), (PRUint16) (logFont.lfWeight), nsnull, data->mDC, &logFont);
-    
     return 0;  // stop iteration

Leave this as is.  It is usually best not be make changes if they don't need
to be made.

+                 aFontFamily->mFaces[index]->GetName().Equals(data->mFullName); index++);

The peculiarity here is that gfxFontEntry->GetName() is returning the family
name not the full name.  And don't you want not equals?

Perhaps the font entry should be created with a full name instead of a family
name.  A reasonable approximation for full name may be to join family_name and
style_name from the FT_Face with a space separator, except when style_name is
"Regular" in which case the full name is just the family name.  But check that
you are getting non-NULL style_names from FreeType.

+    FontEntry *fe = new FontEntry(NS_ConvertUTF8toUTF16(face->family_name));

Check that family_name is non-NULL too.

Another option is to just not support @font-face at this stage, by removing
gfxWindowsPlatform::LookupLocalFont on WINCE.

         ret = TTLoadEmbeddedFontPtr(&fontRef, TTLOAD_PRIVATE, &privStatus, 
                                    LICENSE_PREVIEWPRINT, &pulStatus, 
                                    EOTFontStreamReader::ReadEOTStream, 
                                    &eotReader, (PRUnichar*)(fontName.get()), 0, 0);
         if (ret != E_NONE)
             return nsnull;
     }
 
-    
+#ifdef WINCE
+    gfxFontEntry *fe = LookupLocalFont(uniqueName);
+#else    

The font created with TTLoadEmbeddedFontPtr is not going to have a file
(I hope) and so will not be found.  Better to just remove
gfxWindowsPlatform::MakePlatformFont on WINCE for now.
Attached patch updated based on comments (obsolete) — Splinter Review
Attachment #353571 - Attachment is obsolete: true
Attachment #354670 - Attachment is obsolete: true
Attachment #355338 - Flags: superreview?(pavlov)
Attachment #355338 - Flags: review?(mozbugz)
Attachment #353571 - Flags: superreview?(pavlov)
(In reply to comment #47)


> +#elif defined(XP_WIN)
> +#ifdef WINCE
> +#include <wingdi.h>
> +#include <shellapi.h>
> +#include <shlwapi.h>
> +#define SHGetSpecialFolderPathW SHGetSpecialFolderPath
> +#include "nsUnicharUtils.h"
> +#endif
> +#include "gfxWindowsPlatform.h"
> +#define gfxToolkitPlatform gfxWindowsPlatform
> +#include "nsIWindowsRegKey.h"
> +#include "shlobj.h"
> +#include "nsLocalFile.h"
> 
> Which of these are still needed?

it can be reduced to this:
#elif defined(XP_WIN)
#ifdef WINCE
#define SHGetSpecialFolderPathW SHGetSpecialFolderPath
#endif
#include "gfxWindowsPlatform.h"
#define gfxToolkitPlatform gfxWindowsPlatform
#endif


> +ifdef MOZ_TREE_FREETYPE
> +CXXFLAGS += $(FT2_CFLAGS)
> +endif
> 
> This is needed even when not MOZ_TREE_FREETYPE, so move this to the same block
> that adds gfxFT2Fonts.cpp.

This can't be in the block where we add gfxFT2Fonts.cpp because CXXFLAGS gets set by rules.mk.  I've changed it to have the same test that block (XP_WIN && MOZ_TREE_FREETYPE) and to add CAIRO_FT_CFLAGS instead of FT2_CFLAGS, since that is what the other platforms used.

Also, it's not clear to me why we have both FT2_CFLAGS and CAIRO_FT_CFLAGS.
 

> +        nsRefPtr<gfxFT2Font> font =
> +            gfxFT2Font::GetOrMakeFont(data.bestMatch->mName, 
> +                                      aFont->GetStyle()); 
> +        return static_cast<already_AddRefed<gfxFont>>(font);
> 
> I still don't understand what the static_cast from nsRefPtr<gfxFT2Font> to
> already_AddRefed<gfxFont> is doing.  Is there an intermediate conversion
> happening?  The nsRefPtr<gfxFT2Font> must "forget" that is owns the font so
> that it doesn't unref.

The original code a is a little confusing to me to be honest.  Does this look better?
> 
> +                
> aFontFamily->mFaces[index]->GetName().Equals(data->mFullName); index++);
> 
> The peculiarity here is that gfxFontEntry->GetName() is returning the family
> name not the full name.  And don't you want not equals?
> 
> Perhaps the font entry should be created with a full name instead of a family
> name.  A reasonable approximation for full name may be to join family_name and
> style_name from the FT_Face with a space separator, except when style_name is
> "Regular" in which case the full name is just the family name.  But check that
> you are getting non-NULL style_names from FreeType.

Done. FreeType is returning the style names as you would expect them.  Is there a reason not to append (Regular)? or is that just convention?
 
> Another option is to just not support @font-face at this stage, by removing
> gfxWindowsPlatform::LookupLocalFont on WINCE.
 
I think @font-face is particularly valuable for mobile devices because of the lack of installed fonts, so I'd like to support it if possible.

>          ret = TTLoadEmbeddedFontPtr(&fontRef, TTLOAD_PRIVATE, &privStatus, 
>                                     LICENSE_PREVIEWPRINT, &pulStatus, 
>                                     EOTFontStreamReader::ReadEOTStream, 
>                                     &eotReader, (PRUnichar*)(fontName.get()),
> 0, 0);
>          if (ret != E_NONE)
>              return nsnull;
>      }
> 
> -    
> +#ifdef WINCE
> +    gfxFontEntry *fe = LookupLocalFont(uniqueName);
> +#else    
> 
> The font created with TTLoadEmbeddedFontPtr is not going to have a file
> (I hope) and so will not be found.  Better to just remove
> gfxWindowsPlatform::MakePlatformFont on WINCE for now.

I've reimplemented this function using FT_New_Memory_Face.  I used the pango implementation as my template, but obviously we're not using FontConfig on windows ce, so its a bit different. What is a good way to test this code?
Attachment #355338 - Flags: superreview?(pavlov) → superreview+
Comment on attachment 355338 [details] [diff] [review]
updated based on comments

this looks generally fine.  There might well be better ways to do a lot of the build changes (do we want --enable-system-freetype as a default rather than --enable-tree-freetype for example?)

I would get rid of all the checks for no font from GetFontAt(0) -- it can't happen with working code and there is no point in the checks.  If you want, maybe convert some of them in to assertions so they get compiled out in optimized code.
Comment on attachment 355338 [details] [diff] [review]
updated based on comments

(In reply to comment #49)
> (In reply to comment #47)
> 
> > +ifdef MOZ_TREE_FREETYPE
> > +CXXFLAGS += $(FT2_CFLAGS)
> > +endif
> > 
> > This is needed even when not MOZ_TREE_FREETYPE, so move this to the same
> > block that adds gfxFT2Fonts.cpp.
> 
> This can't be in the block where we add gfxFT2Fonts.cpp because CXXFLAGS gets
> set by rules.mk.  I've changed it to have the same test that block (XP_WIN &&
> MOZ_TREE_FREETYPE)

+ifdef MOZ_TREE_FREETYPE
+DEFINES += -DMOZ_FT2_FONTS
+CPPSRCS	+= gfxFT2Fonts.cpp
+else
+CPPSRCS	+= gfxWindowsFonts.cpp 
+endif

I liked the test for WINCE better than MOZ_TREE_FREETYPE.
The gfxFT2Font backend should work fine with a system FreeType (and
appropriate detection of flags in configure.in).

+ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
+ifdef MOZ_TREE_FREETYPE
+CXXFLAGS += $(CAIRO_FT_CFLAGS)
+endif
+endif

Similarly here.

> and to add CAIRO_FT_CFLAGS instead of FT2_CFLAGS, since that
> is what the other platforms used.
>
> Also, it's not clear to me why we have both FT2_CFLAGS and CAIRO_FT_CFLAGS.

Yes.  I'm not clear either.

Initially I assumed that CAIRO_FT_CFLAGS was for building the cairo-ft
component and FT2_CLFAGS for Mozilla users of freetype, but perhaps
CAIRO_FT_CFLAGS is additional flags for users of the cairo-ft component.

Following the other platforms seems sensible.

> > +        nsRefPtr<gfxFT2Font> font =
> > +            gfxFT2Font::GetOrMakeFont(data.bestMatch->mName, 
> > +                                      aFont->GetStyle()); 
> > +        return static_cast<already_AddRefed<gfxFont>>(font);
> > 
> > I still don't understand what the static_cast from nsRefPtr<gfxFT2Font> to
> > already_AddRefed<gfxFont> is doing.  Is there an intermediate conversion
> > happening?  The nsRefPtr<gfxFT2Font> must "forget" that is owns the font so
> > that it doesn't unref.
> 
> The original code a is a little confusing to me to be honest.  Does this look
> better?

+            gfxFont* ret = font.forget().get();
+            return already_AddRefed<gfxFont>(ret);

Yes, this is doing the right thing.  I don't think nsRefPtr etc were really
set up for base/derived class transformations, so, although forget().get() is
not elegant, I don't know anything better.

+            //        return static_cast<already_AddRefed<gfxFont>>(font);

Remove this.

> > Perhaps the font entry should be created with a full name instead of a
> > family name.  A reasonable approximation for full name may be to join
> > family_name and style_name from the FT_Face with a space separator, except
> > when style_name is "Regular" in which case the full name is just the
> > family name.  But check that you are getting non-NULL style_names from
> > FreeType.
> 
> Done. FreeType is returning the style names as you would expect them.  Is
> there a reason not to append (Regular)? or is that just convention?

Special casing Regular is based on Name ID 4 "Full font name" for TrueType
fonts.  http://www.microsoft.com/typography/otspec/name.htm#enc4

It also is consistent with the Windows behaviour that I've seen.
The Windows code uses ENUMLOGFONTEXW::elfFullName.

WINE sets elfFullName to otmpFaceName

  http://source.winehq.org/source/dlls/gdi32/freetype.c#L3804

which has similar special casing of the regular face

  http://source.winehq.org/source/dlls/gdi32/freetype.c#L5132
  http://source.winehq.org/source/dlls/gdi32/freetype.c#L5329

+const nsCString kFT_REGULAR = NS_LITERAL_CSTRING("Regular");

I think this will show up as a string leak (even though it is not really).
Mozilla generally avoids having non-POD objects with static initialization.

+    if (!face->family_name)
+        return nsnull;

This leaks the face.

+    if (face->style_name && !(kFT_REGULAR.Equals(static_cast<char*>(face->style_name)))) {

The static_cast should be unnecessary here as FT_String is char.
Given that style_name is char* I wouldn't bother converting "Regular" to a
nsCString, but just use strcmp.

+        fontName.Append(L" (");

No brackets around the style.

I'm not sure that L"" notation should be used for PRUnichar/nsString.
I think the accepted convention is AppendLiteral(" ").

+        fontName.Append(NS_ConvertUTF8toUTF16(face->style_name));

AppendUTF8toUTF16(face->style_name, fontName);

+                 !(aFontFamily->mFaces[index]->GetName().Equals(data->mFullName)); index++);

I don't think the parentheses are needed here.

Note that FontEntry::GetName returns mFaceName, which is never set.  Better to
remove GetName and mFaceName from FontEntry and use Name() (from
gfxFontEntry).

> I think @font-face is particularly valuable for mobile devices because of the
> lack of installed fonts, so I'd like to support it if possible.

Fair enough, but it can be added in a separate bug too.

+                nsDependentString name(fe->mName);
+                BuildKeyNameFromFontName(name);       

I'm surprised nsDependentString allows modification of its dependent string
but I guess it's not a const nsDependentString.

Anyway, now that font entries have the full name for mName rather than a
family name, we need another means to get the family name.

> > The font created with TTLoadEmbeddedFontPtr is not going to have a file
> > (I hope) and so will not be found.  Better to just remove
> > gfxWindowsPlatform::MakePlatformFont on WINCE for now.
> 
> I've reimplemented this function using FT_New_Memory_Face.  I used the pango
> implementation as my template, but obviously we're not using FontConfig on
> windows ce, so its a bit different. What is a good way to test this code?

There are reftests in layout/reftests/font-face but the pages need to be
loaded via http to work.  David has them online at
http://dbaron.org/tmp/reftest/font-face/
Attachment #355338 - Flags: review?(mozbugz)
carrying Pav's sr
Attachment #355338 - Attachment is obsolete: true
Attachment #355526 - Flags: superreview+
Attachment #355526 - Flags: review?(mozbugz)
(In reply to comment #51)
> 
> Anyway, now that font entries have the full name for mName rather than a
> family name, we need another means to get the family name.

I added an mFamilyName to FontEntry.  

Another alternative is to pull the FT_Face out of mFontFace and get the family_name from it. That would seem to be a little messy though.

 
> > > The font created with TTLoadEmbeddedFontPtr is not going to have a file
> > > (I hope) and so will not be found.  Better to just remove
> > > gfxWindowsPlatform::MakePlatformFont on WINCE for now.
> > 
> > I've reimplemented this function using FT_New_Memory_Face.  I used the pango
> > implementation as my template, but obviously we're not using FontConfig on
> > windows ce, so its a bit different. What is a good way to test this code?
> 
> There are reftests in layout/reftests/font-face but the pages need to be
> loaded via http to work.  David has them online at
> http://dbaron.org/tmp/reftest/font-face/

I ran fennec through some of these ref tests and MakePlatformFont never gets called so this code doesn't get exercised.  @font-face seems to be failing someplace else.
Comment on attachment 355526 [details] [diff] [review]
updated based on comments #50 and 51

This needs to split out the windows mobile code in gfxWindowsPlatform into a separate one for windows mobile (gfxPlatformWindowsMobile or whatever), given that a large chunk is already and will end up quite different.  Much of the platform code now deals with downloadable fonts, which are tied to windows font APIs.  Also, the windows platform code needs to know the specific font type in use; it should not need to use a generic gfxFont.


Also, the changes to cairo need a patch file with those changes placed in gfx/cairo and a description of the patch in the README -- otherwise they'll be blown away the next time a cairo upgrade happens.

(Sorry for not bringing this stuff up sooner; the early version of the patch that I saw had fewer changes, and I wasn't aware of the gfxFont/gfxWindowsFont issues then.)
(In reply to comment #53)
> (In reply to comment #51)
> > 
> > Anyway, now that font entries have the full name for mName rather than a
> > family name, we need another means to get the family name.
> 
> I added an mFamilyName to FontEntry.  
> 
> Another alternative is to pull the FT_Face out of mFontFace and get the
> family_name from it. That would seem to be a little messy though.

AFAIK there's no case (after construction) where we need the family of a FontEntry, so a better solution is probably to create the FT_Face in gfxWindowsPlatform::AppendFacesFromFontFile (the same function as the first FT_New_Face call) and change FontEntry::CreateFontEntryFromFile to something that creates a font entry from an FT_Face.  (mFilename need not exist either.)
Attached patch got rid of mFamilyName (obsolete) — Splinter Review
Attachment #355526 - Attachment is obsolete: true
Attachment #356525 - Flags: review?(mozbugz)
Attachment #355526 - Flags: review?(mozbugz)
Vlad, does this look better to you?  It passes dbaron's ref tests.
Attachment #356531 - Flags: superreview?(vladimir)
Comment on attachment 356525 [details] [diff] [review]
got rid of mFamilyName

This looks mostly fine to me.  A few small things:

+    fe->mFontFace = cairo_ft_font_face_create_for_ft_face(aFace, 0);

Destruction of the FT_Face needs to be arranged using:

     cairo_font_face_set_user_data(fe->mFontFace, &key,
                                   aFace, FTFontDestroyFunc);

+        fontName.Append(L" ");

I'm not sure that L"" notation should be used for PRUnichar/nsString.
I think the accepted convention is AppendLiteral(" ").

+    static FontEntry* 
+    CreateFontEntryFromFace(FT_Face aFace, long aFaceNum);

+    fe->mFTFontIndex = aFaceNum;

+void gfxWindowsPlatform::AppendFacesFromFontFile(const PRUnichar *aFileName) {

+                fe->mFilename.Assign(fileName);

mFTFontIndex (and aFaceNum) is only useful with mFilename, so these should be
kept together, but neither of these are needed, so I think it's clearer to
store neither.

+    if (FT_Err_Ok == FT_New_Face(gPlatformFTLibrary, 
+                                 fileName, -1, &dummy)) {

+            if (FT_Err_Ok != FT_New_Face(GetFTLibrary(), fileName, 
+                                         i, &face))

Be consistent with gPlatformFTLibrary/GetFTLibrary().

+    for (unsigned int i = 0;  i < searchPaths.Length(); i++) {
+        const nsString& path(searchPaths[i]);
+        for (unsigned int j = 0; j < fontPatterns.Length(); j++) { 

PRUint32 i (and j) would be more conventional here.

@@ -852,34 +988,43 @@ gfxWindowsPlatform::MakePlatformFont(con

-    FontEntry* fe = new FontEntry(aProxyEntry->mFamily->Name());
-    fe->mFontFace = cairo_ft_font_face_create_for_ft_face(face, 0);
+    FontEntry* fe = FontEntry::CreateFontEntryFromFace(face, 0);
-    fe->mItalic = aProxyEntry->mItalic;
-    fe->mWeight = aProxyEntry->mWeight;
-    fe->mStretch = aProxyEntry->mStretch;
     return fe;

What you had before, assigning fe properties according to the proxy entry, is
actually more correct (Bug 465463).
(In reply to comment #58)

> What you had before, assigning fe properties according to the proxy entry, is
> actually more correct (Bug 465463).

Should I not use CreateFontEntryFromFace at all?  If not, its not clear to me whihch properties are best to over write from the proxy entry.
Using CreateFontEntryFromFace is fine.  These entries are (currently) all that need to be copied.  (Unicode ranges are currently not supported and are likely to be something more complicated than a copy, like an intersection, so don't worry about them now.  Most other members of gfxFontEntry seem to be for particular gfxFontGroup implementations and are not set in gfxProxyFontEntrys)

    fe->mItalic = aProxyEntry->mItalic;
    fe->mWeight = aProxyEntry->mWeight;
    fe->mStretch = aProxyEntry->mStretch;
Attachment #356525 - Attachment is obsolete: true
Attachment #356531 - Attachment is obsolete: true
Attachment #357663 - Flags: superreview?(vladimir)
Attachment #357663 - Flags: review?(mozbugz)
Attachment #356525 - Flags: review?(mozbugz)
Attachment #356531 - Flags: superreview?(vladimir)
Karl, its not clear to me if CreateFontEntryFromFace should share the static cairo_user_data_key_t passed to cairo_font_face_set_user_data with FontEntry::CairoFontFace() or not.  This patch doesn't.
Attached patch fixed to apply to tip (obsolete) — Splinter Review
The last patch had a couple conflicts due to the landing of Bug 461047. Also I picked up Pav's comments about not needing checks around GetFontAt(0).
Attachment #357663 - Attachment is obsolete: true
Attachment #357676 - Flags: superreview?(vladimir)
Attachment #357676 - Flags: review?(mozbugz)
Attachment #357663 - Flags: superreview?(vladimir)
Attachment #357663 - Flags: review?(mozbugz)
(In reply to comment #62)
> Karl, its not clear to me if CreateFontEntryFromFace should share the static
> cairo_user_data_key_t passed to cairo_font_face_set_user_data with
> FontEntry::CairoFontFace() or not.  This patch doesn't.

They don't _need_ separate keys.  Separate keys would only be needed if the FTFontDestroyFunc should be called twice, once for each user_data.
It will work fine with separate keys because only one is ever used at a time.
It's not obvious to me whether one shared or two separate keys is better.
(Neither situation would make it clearer that only one destroy function should call FT_Done_Face.)
(In reply to comment #57)
> Created an attachment (id=356531) [details]
> moves @font-face code to gfxWindowsFont
> 
> Vlad, does this look better to you?

Dealing with all our different font backend / surface backends / widget
toolkit / OS combinations is difficult to do tidily.  I hope there is a better
way than what we currently have in gfxPlatformGtk.cpp and nsObjectFrame.cpp,
but the preprocessor conditionals here don't seem any worse than those
examples.

Moving font code out of gfxPlatform implementations and into gfxFont
implementations seems a sensible improvement to the situation to me (and
perhaps in the future moving some of the font part of gfxPlatform API to a
gfxFont or related API may be worth considering.)

But Vlad would have a good overall picture of the issues involved, so I'm
interested to hear if he has any ideas on where we should be heading here.

This may become even more interesting when we have multiple font backends
compiled (and selected at run time) for some platforms, but perhaps they will
be hidden behind a single font group interface and a single font entry
interface.
Comment on attachment 357676 [details] [diff] [review]
fixed to apply to tip

r+=me with the following small changes.

+    CreateFontEntry(const gfxProxyFontEntry &aProxyEntry, nsISupports *aLoader,
+                 const PRUint8 *aFontData, PRUint32 aLength);

Indentation.

-    already_AddRefed<gfxWindowsFont> FindFontForChar(PRUint32 aCh, gfxWindowsFont *aFont);
+    already_AddRefed<gfxFont>
+    gfxWindowsPlatform::FindFontForChar(PRUint32 aCh, gfxFont *aFont);

"gfxWindowsPlatform::" is not necessary.

+/* static */
+
+FontEntry*  
+FontEntry::CreateFontEntry(const gfxProxyFontEntry &aProxyEntry, 

No blank line here.

+/* static */
+FontEntry*  FontEntry::CreateFontEntryFromFace(FT_Face aFace) {

Convention has FontEntry::CreateFontEntryFromFace on a new line.
(I don't know whether there's a convention re whether FontEntry* goes on the
same line as /* static */ or not.)

+            familyArray.AppendString(NS_ConvertUTF8toUTF16((char*)family));

AppendElement due to the recent nsTArray changes.

+FontEntry::CreateFontEntry(const gfxProxyFontEntry &aProxyEntry, nsISupports *aLoader,
+             const PRUint8 *aFontData, PRUint32 aLength) {

Indentation.
Attachment #357676 - Flags: review?(mozbugz) → review+
Attachment #357676 - Flags: superreview?(vladimir)
Attachment #357676 - Flags: superreview+
Attachment #357676 - Flags: review?(jdaggett)
Comment on attachment 357676 [details] [diff] [review]
fixed to apply to tip

Looks ok to me, though some of the bits in gfxWindowsPlatform should probably be FT2-ifdefs instead of WINCE.  Not a big deal.

I'd like John to take a quick look over it though before you check in, to make sure there isn't anything that I missed font-wise.
Product: Core → Core Graveyard
Assignee: bugmail → nobody
Component: GFX → GFX: Thebes
Product: Core Graveyard → Core
QA Contact: general → thebes
Version: unspecified → Trunk
(In reply to comment #67)

Overall, looks good.  But I think the use of WINCE vs. xxx is confusing and should be fixed before checkin.

> +#ifndef WINCE
>  #include "gfxWindowsFonts.h"
> +#elif MOZ_FT2_FONTS
> +#include "gfxFT2Fonts.h"
> +#else
> +#error "Windows gfx platform needs a font backend"
> +#endif

I don't think the use of WINCE belongs here, you're really conditionalizing the use of FT font handling vs. windows font handling.  Let's deal with ports of the FT code to other non-WINCE Windows platforms when the need arises, I think the #error's clutter the code unnecessarily.  Only within code calling WINCE-specific API's does the WINCE conditional seem appropriate.

Instead I would suggest:

#ifdef MOZ_FT2_FONTS
#include "gfxFT2Fonts.h"
#else
#include "gfxWindowsFonts.h"
#endif

This pattern is repeated in multiple places.  Also, the use of the WINCE conditional in gfxWindowsFonts.cpp can be omitted, right?

> +#ifdef MOZ_FT2_FONTS
> +    FT_Init_FreeType(&gPlatformFTLibrary);
> +#endif
> +
>      UpdateFontList();
>  
>      nsCOMPtr<nsIPref> pref = do_GetService(NS_PREF_CONTRACTID);
>      pref->RegisterCallback("font.", PrefChangedCallback, this);
>      pref->RegisterCallback("font.name-list.", PrefChangedCallback, this);
>      pref->RegisterCallback("intl.accept_languages", PrefChangedCallback, this);
>      // don't bother unregistering.  We'll get shutdown after the pref service
>  
> -    InitializeFontEmbeddingProcs();
> +#ifndef WINCE
> +    FontEntry::InitializeFontEmbeddingProcs();
> +#endif

This should just be:

#ifdef MOZ_FT2_FONTS
    FT_Init_FreeType(&gPlatformFTLibrary);
#else
   FontEntry::InitializeFontEmbeddingProcs();
#endif

> +#ifndef WINCE
>      if (aFontEntry->SupportsLangGroup(data->mLangGroup) &&
>          aFontEntry->MatchesGenericFamily(data->mGenericFamily))
> +#endif

Same, use MOZ_FT2_FONTS instead.

> +#ifdef MOZ_FT2_FONTS
> +    FT_Library GetFTLibrary();
> +private:
> +    void AppendFacesFromFontFile(const PRUnichar *aFileName);
> +    void FindFonts();
> +#endif
>  
> +void
> +gfxWindowsPlatform::FindFonts()
> +{
> +#ifdef WINCE

Trim WINCE here.

>   void
>  gfxWindowsPlatform::InitBadUnderlineList()
>  {
> +// Only windows fonts have mIsBadUnderlineFontFamily flag
> +#ifndef WINCE
>      nsAutoTArray<nsString, 10> blacklist;
>      gfxFontUtils::GetPrefsFontList("font.blacklist.underline_offset", blacklist);
>      PRUint32 numFonts = blacklist.Length();
>      for (PRUint32 i = 0; i < numFonts; i++) {
>          PRBool aborted;
>          nsAutoString resolved;
>          ResolveFontName(blacklist[i], SimpleResolverCallback, &resolved, aborted);
>          if (resolved.IsEmpty())
>              continue;
>          FontFamily *ff = FindFontFamily(resolved);
>          if (!ff)
>              continue;
>          ff->mIsBadUnderlineFontFamily = 1;
>      }
> +#endif
>  }
 
Is WinCE not affected by this problem?  If it is, please open a new bug so that we don't forget to deal with it at some point.

>       nsRefPtr<FontEntry> fe = aFontFamily->FindFontEntry(*data->fontToMatch->GetStyle());
> +    PRInt32 rank = 0;
>  
> +#ifndef WINCE
>      // skip over non-unicode and bitmap fonts and fonts that don't have
>      // the code point we're looking for
>      if (fe->IsCrappyFont() || !fe->mCharacterMap.test(ch))
>          return PL_DHASH_NEXT;
>  
> -    PRInt32 rank = 0;
>      // fonts that claim to support the range are more
>      // likely to be "better fonts" than ones that don't... (in theory)

WINCE ==> MOZ_FT2_FONTS

> +/* static */
> +FontEntry*  FontEntry::CreateFontEntryFromFace(FT_Face aFace) {
> +    static cairo_user_data_key_t key;
> +
> +    if (!aFace->family_name) {
> +        FT_Done_Face(aFace);
> +        return nsnull;
> +    }
> +    NS_ConvertUTF8toUTF16 fontName(aFace->family_name);
> +    if (aFace->style_name && strcmp("Regular", aFace->style_name)) {
> +        fontName.AppendLiteral(" ");
> +        AppendUTF8toUTF16(aFace->style_name, fontName);
> +    }
> +    FontEntry *fe = new FontEntry(fontName);
> +    fe->mItalic = aFace->style_flags & FT_STYLE_FLAG_ITALIC;
> +    fe->mFontFace = cairo_ft_font_face_create_for_ft_face(aFace, 0);
> +    cairo_font_face_set_user_data(fe->mFontFace, &key,
> +                                  aFace, FTFontDestroyFunc);
> +    if (aFace->style_flags & FT_STYLE_FLAG_BOLD)
> +        fe->mWeight = 700;
> +    else
> +        fe->mWeight = 400;
> +    return fe;
>  }

So the assumption is that no installed fonts on WinCE are never anything other than normal or bold weights?  Are the font lists fixed, with no way of adding fonts?  And what's the style name check for?  Both of these need comments I think.  Seems like determining the real weight wouldn't be such a big deal, but if we're skipping that for perf reasons we should note that in the code:

  os2 = (TT_OS2 *) FT_Get_Sfnt_Table (face, ft_sfnt_os2);
  if (os2 && os2->version != 0xffff)
    os2->usWeightClass

> +void
> +gfxWindowsPlatform::FindFonts()
> +{
> +#ifdef WINCE
> +    nsTArray<nsString> searchPaths(2);
> +    nsTArray<nsString> fontPatterns(2);
> +    fontPatterns.AppendElement(NS_LITERAL_STRING("\\*.TTF"));
> +    fontPatterns.AppendElement(NS_LITERAL_STRING("\\*.ttf"));

FT supports Postscript CFF fonts (*.otf).  Is there a reason to not support them?  Not a big deal but seems like it would be nice to support them if that support comes for free.
Argh, revise to below:

"Overall, looks good.  But I think the use of WINCE vs. MOZ_FT2_FONTS is confusing and should be fixed before checkin."
Attachment #357676 - Attachment is obsolete: true
Attachment #358336 - Flags: review?
Attachment #357676 - Flags: review?(jdaggett)
(In reply to comment #68)

> >   void
> >  gfxWindowsPlatform::InitBadUnderlineList()
> >  {
> > +// Only windows fonts have mIsBadUnderlineFontFamily flag
> > +#ifndef WINCE
> >      nsAutoTArray<nsString, 10> blacklist;
> >      gfxFontUtils::GetPrefsFontList("font.blacklist.underline_offset", blacklist);
> >      PRUint32 numFonts = blacklist.Length();
> >      for (PRUint32 i = 0; i < numFonts; i++) {
> >          PRBool aborted;
> >          nsAutoString resolved;
> >          ResolveFontName(blacklist[i], SimpleResolverCallback, &resolved, aborted);
> >          if (resolved.IsEmpty())
> >              continue;
> >          FontFamily *ff = FindFontFamily(resolved);
> >          if (!ff)
> >              continue;
> >          ff->mIsBadUnderlineFontFamily = 1;
> >      }
> > +#endif
> >  }
> 
> Is WinCE not affected by this problem?  If it is, please open a new bug so that
> we don't forget to deal with it at some point.

I don't know what problem this is meant to address, but if you point me to the original bug I'll file a follow up.

> > +/* static */
> > +FontEntry*  FontEntry::CreateFontEntryFromFace(FT_Face aFace) {
> > +    static cairo_user_data_key_t key;
> > +
> > +    if (!aFace->family_name) {
> > +        FT_Done_Face(aFace);
> > +        return nsnull;
> > +    }
> > +    NS_ConvertUTF8toUTF16 fontName(aFace->family_name);
> > +    if (aFace->style_name && strcmp("Regular", aFace->style_name)) {
> > +        fontName.AppendLiteral(" ");
> > +        AppendUTF8toUTF16(aFace->style_name, fontName);
> > +    }
> > +    FontEntry *fe = new FontEntry(fontName);
> > +    fe->mItalic = aFace->style_flags & FT_STYLE_FLAG_ITALIC;
> > +    fe->mFontFace = cairo_ft_font_face_create_for_ft_face(aFace, 0);
> > +    cairo_font_face_set_user_data(fe->mFontFace, &key,
> > +                                  aFace, FTFontDestroyFunc);
> > +    if (aFace->style_flags & FT_STYLE_FLAG_BOLD)
> > +        fe->mWeight = 700;
> > +    else
> > +        fe->mWeight = 400;
> > +    return fe;
> >  }
> 
> So the assumption is that no installed fonts on WinCE are never anything other
> than normal or bold weights?  Are the font lists fixed, with no way of adding
> fonts?  And what's the style name check for?  Both of these need comments I
> think.  Seems like determining the real weight wouldn't be such a big deal, but
> if we're skipping that for perf reasons we should note that in the code:
> 
>   os2 = (TT_OS2 *) FT_Get_Sfnt_Table (face, ft_sfnt_os2);
>   if (os2 && os2->version != 0xffff)
>     os2->usWeightClass
>

The font list isn't fixed, I just didn't know a better way to get the weight.   I've taken your the code you suggested and used what I had as a fall back.

wrt the style_name check see comment 51.  I've added a comment, hope that clears it up.
 
> > +void
> > +gfxWindowsPlatform::FindFonts()
> > +{
> > +#ifdef WINCE
> > +    nsTArray<nsString> searchPaths(2);
> > +    nsTArray<nsString> fontPatterns(2);
> > +    fontPatterns.AppendElement(NS_LITERAL_STRING("\\*.TTF"));
> > +    fontPatterns.AppendElement(NS_LITERAL_STRING("\\*.ttf"));
> 
> FT supports Postscript CFF fonts (*.otf).  Is there a reason to not support
> them?  Not a big deal but seems like it would be nice to support them if that
> support comes for free.

Agreed, no reason not to support them.
Comment on attachment 358336 [details] [diff] [review]
fixed up according to jdaggett comments

Looks good, as long as the units for osWeightClass are right.

The bad underline blacklist was put in by Masayuki for bug 417014.
Attachment #358336 - Flags: review? → review+
-#ifdef MOZ_FT2_FONTS
-    FT_Init_FreeType(&gPlatformFTLibrary);
-#endif
-
     UpdateFontList();
 
     nsCOMPtr<nsIPref> pref = do_GetService(NS_PREF_CONTRACTID);
@@ -121,7 +115,9 @@ 
     pref->RegisterCallback("intl.accept_languages", PrefChangedCallback, this);
     // don't bother unregistering.  We'll get shutdown after the pref service
 
-#ifndef WINCE
+#ifdef MOZ_FT2_FONTS
+    FT_Init_FreeType(&gPlatformFTLibrary);
+#else
     FontEntry::InitializeFontEmbeddingProcs();
 #endif

Should these be moved to before UpdateFontList()?
(In reply to comment #73)
> -#ifdef MOZ_FT2_FONTS
> -    FT_Init_FreeType(&gPlatformFTLibrary);
> -#endif
> -
>      UpdateFontList();
> 
>      nsCOMPtr<nsIPref> pref = do_GetService(NS_PREF_CONTRACTID);
> @@ -121,7 +115,9 @@ 
>      pref->RegisterCallback("intl.accept_languages", PrefChangedCallback,
> this);
>      // don't bother unregistering.  We'll get shutdown after the pref service
> 
> -#ifndef WINCE
> +#ifdef MOZ_FT2_FONTS
> +    FT_Init_FreeType(&gPlatformFTLibrary);
> +#else
>      FontEntry::InitializeFontEmbeddingProcs();
>  #endif
> 
> Should these be moved to before UpdateFontList()?

Yes, I caught that before I pushed
pushed http://hg.mozilla.org/mozilla-central/rev/aa27de3b8563

(In reply to comment #72)
> (From update of attachment 358336 [details] [diff] [review])
> Looks good, as long as the units for osWeightClass are right.

It appears to be the right units, the fonts that were loaded were all either 400 or 700
 
> The bad underline blacklist was put in by Masayuki for bug 417014.

looks likes this issue handled by cairo here:
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-ft-font.c#1932
for reference, this flag is documented here:
http://freetype.sourceforge.net/freetype2/docs/reference/ft2-base_interface.html#FT_LOAD_XXX
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #75)
> pushed http://hg.mozilla.org/mozilla-central/rev/aa27de3b8563

Don't forget to add the patch to gfx/cairo and gfx/cairo/README (comment 54).
Attachment #358336 - Flags: approval1.9.1?
Duplicate of this bug: 465560
Assignee: nobody → bugmail
Attachment #358336 - Flags: approval1.9.1? → approval1.9.1+
I backed this out of 1.9.1 to try to fix unittest orange. Sorry. :-(
Keywords: fixed1.9.1
Ted, what was the orange that led you to pull this and other patches?

I'm looking at the Tinderbox history and it actually looks rather green for a changeset that contains all the checkins you reverted, cd99dc2701b0:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox3.5&maxdate=1238153561&legend=0&norules=1

Is there something not shown here that caused you to pull this?  I looks like there a couple of the Windows boxes had some mochitest timeouts but most are green.
Yeah, at the time Linux unittest was orange, and Windows unittest had the two orange cycles in a row on the same changeset (with timeouts). It then cycled green on the same changeset, so it's quite possible it was unrelated. Apologies if so!
Keywords: fixed1.9.1
Whiteboard: [fixed1.9.1b4]
Target Milestone: --- → mozilla1.9.1b2
You need to log in before you can comment on or make changes to this bug.