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

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

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 ;-)

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.)

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

add comment

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.

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 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?

This bug also causes bustage on Ubuntu 9.04 Beta.

(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.

(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 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.

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.

Karl, anything additional needed with your patch?

(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

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

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

remove unnecessary callback functions after testing this, send review.

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.

(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.

(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.

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

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

Zack thanks

(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().

(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.

(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.

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.

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

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 <ubuntu-devel-discuss@lists.ubuntu.com> Original-Maintainer: Sebastien Bacher <seb128@debian.org> Architecture: amd64 Source: pango1.0 Version: 1.24.1-0ubuntu1

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

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.

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.

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

(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.

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

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 on attachment 372739 [details] [diff] [review] use new_font [Checkin: Comment 39 & 42] a191=beltzner

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