478871 - compile error 'struct _PangoFcFontMapClass' has no member named 'context_substitute' with pango 1.23

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Robert Kaiser]

Yesterday, I upgraded my system to the newest openSUSE Factory, which now includes pango 1.23.0, and I get that compile error on both mozilla-central and 1.9.1:

c++ -o gfxPangoFonts.o -c -I../../../dist/include/system_wrappers -include /mnt/mozilla/hg/comm-central/mozilla/config/gcc_hidden.h -DIMPL_THEBES -DMOZILLA_INTERNAL_API -DMOZ_SUITE=1 -DSUITE_USING_XPFE_DM=1 -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux  -I/mnt/mozilla/hg/comm-central/mozilla/gfx/thebes/src -I. -I../../../dist/include/cairo -I../../../dist/include/string -I../../../dist/include/pref -I../../../dist/include/xpcom -I../../../dist/include/unicharutil -I../../../dist/include/lcms -I../../../dist/include/locale -I../../../dist/include   -I../../../dist/include/thebes -I/mnt/mozilla/build/seamonkey/mozilla/dist/include/nspr     -I/mnt/mozilla/build/seamonkey/mozilla/dist/sdk/include    -fPIC   -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -march=pentium4 -mtune=nocona -msse2 -msse3 -mssse3 -mfpmath=sse -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions  -I/mnt/mozilla/build/seamonkey/mozilla/dist/include/cairo -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0   -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2     -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/gfxPangoFonts.pp /mnt/mozilla/hg/comm-central/mozilla/gfx/thebes/src/gfxPangoFonts.cpp
/mnt/mozilla/hg/comm-central/mozilla/gfx/thebes/src/gfxPangoFonts.cpp: In function 'void gfx_pango_font_map_class_init(gfxPangoFontMapClass*)':
/mnt/mozilla/hg/comm-central/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:1833: error: 'struct _PangoFcFontMapClass' has no member named 'context_substitute'
/mnt/mozilla/hg/comm-central/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:1834: error: invalid conversion from 'PangoFcFont* (*)(PangoFcFontMap*, PangoContext*, const PangoFontDescription*, FcPattern*)' to 'PangoFcFont* (*)(PangoFcFontMap*, PangoFcFontKey*)'
gmake[7]: *** [gfxPangoFonts.o] Error 1
gmake[7]: *** Waiting for unfinished jobs....
gmake[7]: Leaving directory `/mnt/mozilla/build/seamonkey/mozilla/gfx/thebes/src'
gmake[6]: *** [libs] Error 2
gmake[6]: Leaving directory `/mnt/mozilla/build/seamonkey/mozilla/gfx/thebes'
gmake[5]: *** [libs] Error 2
gmake[5]: Leaving directory `/mnt/mozilla/build/seamonkey/mozilla/gfx'
gmake[4]: *** [libs_tier_gecko] Error 2
gmake[4]: Leaving directory `/mnt/mozilla/build/seamonkey/mozilla'
gmake[3]: *** [tier_gecko] Error 2
gmake[3]: Leaving directory `/mnt/mozilla/build/seamonkey/mozilla'
gmake[2]: *** [default] Error 2
gmake[2]: Leaving directory `/mnt/mozilla/build/seamonkey/mozilla'
gmake[1]: *** [default] Error 2
gmake[1]: Leaving directory `/mnt/mozilla/build/seamonkey'
gmake: *** [build] Error 2


http://svn.gnome.org/viewvc/pango/tags/PANGO_1_23_0/pango/pangofc-fontmap.h?view=log tells me they removed context_substitute in revision 2804, ultimately (in rev. 2808) with fontset_key_substitute, the final 1.23.0 definition is in http://svn.gnome.org/viewvc/pango/tags/PANGO_1_23_0/pango/pangofc-fontmap.h?revision=2830&view=markup#l174

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch v0

1.23 series is unstable version.

Also, I don't test this patch on older version.  After that, I will send a review.

Comment 2

[Robert Kaiser]

It sounds reasonable for a development distribution to pick up an unstable version of that library, I guess. And that's why I'm using that stuff: Better to catch and fix our code for that change now than when it comes up in the stable version ;-)

Comment 3

[Makoto Kato [:m_kato]]

The latest snapshot of fedora rawhide (Fedora 11?) uses Pango-1.23.  So, some distributions may use 1.23 series.  And, the API Document says that these APIs are from 1.24 (stable) series.

So, we need fix this even if API is changed by later release.  (I don't know release data of 1.24.  1.23 was this month.)

Comment 4

[Robert Kaiser]

I can confirm that my builds (both 1.9.1-based and mozilla-central) compile and work with this patch and Pango 1.23.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: patch v1

add comment

Comment 6

[Ginn Chen]

Since "// context_substitute and get_font are not likely to be used" 
and from pango header both context_substitute and fontset_key_substitute may be NULL.
I think the right fix should be remove these functions from mozilla code. Use NULL.
Otherwise we may introduce crashes if Firefox is compiled and run with different versions of pango.

Comment 7

[Makoto Kato [:m_kato]]

Since stable version (1.24) is released, I analyse this.

I tested with pango 1.24, gfx callback function of fontset_key_subtitute and get_font are never called.

Becaues, the following reasons.  So these call functions are never called.

- get_font and foreach in gfx_pango_fontset_class_init() is implemented.
- get_resolution in gfx_pango_font_map_class_init() is implemented.

So I will rewrite fix...

Comment 8

[Makoto Kato [:m_kato]]

Attachment: patch v2

Comment 10

[Robert Kaiser]

Comment on attachment 369648 [details] [diff] [review]
patch v2

>+    if (pango_version() < PANGO_VERSION_ENCODE(1,23,0)) {
>+#if PANGO_VERSION_CHECK(1,23,0)

When do we get inside here to that code? The outer if says we have pango < 1.23, right?

Comment 12

[Josh Aas]

This bug also causes bustage on Ubuntu 9.04 Beta.

Comment 13

[Ginn Chen]

(In reply to comment #10)
> (From update of attachment 369648 [details] [diff] [review])
> >+    if (pango_version() < PANGO_VERSION_ENCODE(1,23,0)) {
> >+#if PANGO_VERSION_CHECK(1,23,0)
> 
> When do we get inside here to that code? The outer if says we have pango <
> 1.23, right?

The outer checking is runtime checking.
The inner checking is a compile-time checking.

I think it might be less confusion if we include a private structure declaration of PangoFcFontMapClass.

Also I'm wondering if context_substitute and create_font are really called in Firefox with Pango < 1.23.

Comment 14

[Makoto Kato [:m_kato]]

(In reply to comment #10)
> (From update of attachment 369648 [details] [diff] [review])
> >+    if (pango_version() < PANGO_VERSION_ENCODE(1,23,0)) {
> >+#if PANGO_VERSION_CHECK(1,23,0)
> 
> When do we get inside here to that code? The outer if says we have pango <
> 1.23, right?

see Ginn's comment. (comment #13)

(In reply to comment #10)
> Also I'm wondering if context_substitute and create_font are really called in
> Firefox with Pango < 1.23

I don't confirm it, yet.  But I have checked code of version 1.23 and 1.24.  Should we check all version of source code? (1.14.0 or greater??)

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 369648 [details] [diff] [review]
patch v2

create_font and context_key_substitute are not currently used but are provided for a
complete PangoFcFontMap implementation.  This is meant to protect from future
changes to Pango that may result in these functions being used, or, less
likely, shaper modules choosing to use these functions.

create_font can be NULL but in that case "new_font() is used", and should not
be NULL.  Building two different create_font implementations and choosing at
runtime seems unappealing, so I suggest (unless Behdad suggests otherwise)
to change create_font() to new_font().  context and desc are not used anyway
and there is some history of maintaining backward compatibility for the
deprecated new_font().

context/fontset_key_substitute is probably even less likely to be used, but similarly a
default_substitute implementation would be the same for pre/post 1.23.
Without a context, defaults would need to be used for size and usePrinterFont.
usePrinterFont false is probably the better option, and pangocairo uses 18.0
for the default size, so let's do the same.

Comment 16

[Behdad Esfahbod]

Karl's analysis is accurate.  Although in some distant future when pango 1.24 can be assume, would be nice to switch to the new API.  The change was necessary to get the PangoContext argument out of the callbacks to enable a range of optimizations.  In general I don't like breaking backend API.  Sorry that happened this time.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Karl, anything additional needed with your patch?

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> Yesterday, I upgraded my system to the newest openSUSE Factory, which now
> includes pango 1.23.0, and I get that compile error 

FWIW: this bug affects Ubuntu 9.04 ("Jaunty") as well, which is going to ship later this month and includes pango version 1.24.0.  There's a bug filed (linked to this bug) in Ubuntu's bug-tracker at https://bugs.launchpad.net/bugs/349914

Comment 19

[cajbir (:cajbir)]

It also affects Arch Linux, current version. When you do a pacman update you get pango version 1.24.0.

Comment 20

[Zack Weinberg (:zwol)]

pango 1.24.0 is now in Debian unstable as well.  Makoto Kato's patch gets me compiling again.

Comment 21

[Makoto Kato [:m_kato]]

Attachment: patch v3

remove unnecessary callback functions
after testing this, send review.

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 371367 [details] [diff] [review]
patch v3

Sorry, I missed some comments here.
(I thought I'd CC'd myself and Behdad, but I guess I didn't.)

This patch works around the compile error, but we should provide at least fcfontmap_class->new_font if we don't provide create_font.

That would simply involve changing gfx_pango_font_map_create_font to gfx_pango_font_map_new_font and modifying the parameters appropriately.

And we might as well change gfx_pango_font_map_context_substitute to gfx_pango_font_map_default_substitute and use default values from comment 15 where the information is not available for fcfontmap_class->default_subsitute.

Comment 23

[Makoto Kato [:m_kato]]

(In reply to comment #22)

Karl, do you have test case that context_substitute or create_font is called?

Even if pango is 1.14 (min version required gecko) in CentOS 5, these callbacks are never called on any case.
Although default_substitute (pango_fc_default_substitute) will call from pango_fc_font_map_get_patterns/pango_fc_font_map_get_resolution into pango, since gfx implementation has a lot of callback functions such as load_font and get_resolution, it is never called.

I would like to know the environment that context_substitute or create_font is really called.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to comment #23)
> Karl, do you have test case that context_substitute or create_font is called?

No.  As far as I have seen no current Pango version will call these methods, given the way we use Pango and the PangoFontMap methods that we override (as you point out).

However, the PangoFcFontMap API docs say that a PangoFcFontMap should provide these functions.  We create a PangoFcFontMap and pass it to Pango, so it would be reasonable for any future version of Pango or any shaper modules to expect these methods to be present.

For example, a call to pango_font_map_load_fontset with a fresh PangoContext that does not have a gfxPangoFontGroup on it will result in a call to these methods.
You can test this by replacing GetFontGroup(context) with NULL in gfx_pango_font_map_load_fontset.

Comment 25

[John Vivirito]

This bug doesn't only apply to Seamonkey does it?

Comment 26

[Zack Weinberg (:zwol)]

No, it's in core gfx, so it applies to everything.

Comment 27

[John Vivirito]

Zack thanks

Comment 28

[Behdad Esfahbod]

(In reply to comment #24)

> However, the PangoFcFontMap API docs say that a PangoFcFontMap should provide
> these functions.  We create a PangoFcFontMap and pass it to Pango, so it would
> be reasonable for any future version of Pango or any shaper modules to expect
> these methods to be present.

If your concern future pango versions, then I suggest adding a compile-time version check on pango and setting the new methods instead of the deprecated new_font() and default_substitute().

Comment 29

[Karl Tomlinson (:karlt)]

(In reply to comment #28)
> If your concern future pango versions, then I suggest adding a compile-time
> version check on pango and setting the new methods instead of the deprecated
> new_font() and default_substitute().

We can use the new methods, but, if we do that, we need to have pre- and post-1.24 versions of the methods (regardless of the compile-time version), and do a run-time Pango version check.

Comment 30

[Behdad Esfahbod]

(In reply to comment #29)
> (In reply to comment #28)
> > If your concern future pango versions, then I suggest adding a compile-time
> > version check on pango and setting the new methods instead of the deprecated
> > new_font() and default_substitute().
> 
> We can use the new methods, but, if we do that, we need to have pre- and
> post-1.24 versions of the methods (regardless of the compile-time version), and
> do a run-time Pango version check.

Thinking about it again, you're right.  If you don't need the context information anyway, just use the new_font() / default_substitute() callbacks.

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We need a fix for this, but won't block the release for it; if this API changed, worst case the linux distros will need to ship a patch to fix it if we don't get some kind of patch in.

Comment 32

[Karl Tomlinson (:karlt)]

Attachment: use new_font [Checkin: Comment 39 & 42]

Using new_font and default_substitute as discussed above.
This needs testing against Pango-1.24.  (I've tested Pango-1.22.4.)

Comment 33

[Frederic Bezies]

It works here with my ubuntu linux jaunty beta up-to-date.

fred@fredo-ubuntu:~$ apt-cache show libpango1.0-0
Package: libpango1.0-0
Priority: optional
Section: libs
Installed-Size: 980
Maintainer: Ubuntu Core Developers <[e-mail]>
Original-Maintainer: Sebastien Bacher <[e-mail]>
Architecture: amd64
Source: pango1.0
Version: 1.24.1-0ubuntu1

Comment 34

[Zack Weinberg (:zwol)]

WFM also with libpango 1.24.0-3 (debian unstable).

Comment 35

[nulldomain]

WFM with pango 1.24.1 on Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4pre) Gecko/20090415 Firefox/3.5b4pre.

Comment 36

[Henning Henkel]

Building Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4pre) Gecko/20090416 Lightning/1.0pre Shredder/3.0b3pre from Mercurial on ArchLinux with Pango 1.24.0-2 worked with the patch for me as well.

Comment 37

[John Vivirito]

I tried version 3 patch and it failed to apply in Sm 2 version 2 applies fine and fixes the FTB

Comment 38

[Daniel Holbert [:dholbert]]

(In reply to comment #37)
> I tried version 3 patch and it failed to apply in Sm 2

Note that "patch v3" is marked as obsolete -- the "use new_font" patch is the one that's got r+sr & [needs landing].  Give that one a try -- it applies fine in mozilla-central and mozilla-1.9.1.

Comment 39

[Daniel Holbert [:dholbert]]

"use new_font" patch landed in m-c:
http://hg.mozilla.org/mozilla-central/rev/4aed53dcf692

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 372739 [details] [diff] [review]
use new_font
[Checkin: Comment 39 & 42]

Requesting approval1.9.1 for this bug's patch (to fix a messy compile error on newer linux distros).

Note that comment 31 (in which this was set to blocking1.9.1- & wanted1.9.1+) indicates that this *needs* fixing on 1.9.1, but it just wouldn't hold the release.  Now we've got a fix, so it should hopefully land on 1.9.1 after baking a bit on mozilla-central.

Comment 41

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 372739 [details] [diff] [review]
use new_font
[Checkin: Comment 39 & 42]

a191=beltzner

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 372739 [details] [diff] [review]
use new_font
[Checkin: Comment 39 & 42]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9de5cf733d70