Missing/incomplete nsIFontEnumerator::HaveFontFor method




17 years ago
17 years ago


(Reporter: cls, Assigned: Frank Tang)




Firefox Tracking Flags

(Not tracked)



(1 attachment)



17 years ago
The bewildered tinderbox has been red since ftang's check ins this morning.  I
looked at the checkins and I noticed that a) there was no bug number for the
checkin, b) only windows has a complete implementation of the method and c) none
of the other toolkits (xlib, qt) even have the dummy implementation.  I'm still
waiting for OS/2 to go red.  Was it really necessary for this change to go in
this incomplete the morning that the tree is closing for a release?


17 years ago
Severity: critical → blocker
Keywords: pp

Comment 1

17 years ago
I have to echo chris's view here... a single sr= is not sufficient (r= and sr=
are what's specified), and neither is it ok to bust other platforms (especially
just prior to a tree closure).

What's the justification for not backing these changes out?

Comment 2

17 years ago
Stub added to OS2 and BeOS
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 3

17 years ago
Changed QA contact to ftang@netscape.com since QA cannot verify this.
QA Contact: teruko → ftang
I just added the stub (copied from GTK) to the xlib port as well (after getting
approval from cls and blizzard on IRC).

Comment 5

17 years ago
Xprint (X11 print system; add --with-xprint to Linux|Solaris2.7/2.8 builds)
seems to suffer from the same problem (I filed bug 68441 for this) - can anyone
of the gurus here have a look into the problem, please ?

Shared library module libgfxxprt.so is rejected like this:
-- snip --
Load FAILED with error: ld.so.1: ./mozilla-bin: fatal: relocation error: file
symbol __1cSnsFontEnumeratorXPLHaveFontFor6Mpkcpi_I_: referenced symbol not
-- snip --

% dem __1cSnsFontEnumeratorXPLHaveFontFor6Mpkcpi_I_ 
__1cSnsFontEnumeratorXPLHaveFontFor6Mpkcpi_I_ == unsigned
nsFontEnumeratorXP::HaveFontFor(const char*,int*)

Comment 6

17 years ago
Fixed tis issue myself... :-)
diff for
-- snip --
mozilla-2001-02-10-08-Mtrunk/mozilla/gfx/src/xprint% diff -c -w
nsFontMetricsXP.cpp.original nsFontMe
*** nsFontMetricsXP.cpp.original        Sun Feb 11 04:15:42 2001
--- nsFontMetricsXP.cpp Sun Feb 11 04:21:09 2001
*** 2983,2985 ****
--- 2983,3002 ----
    // XXX still need to implement aLangGroup and aGeneric
    return EnumerateAllFonts(aCount, aResult);
+ /* gisburn: copy&paste clone from nsFontEnumeratorXlib::HaveFontFor in
+  * mozilla/gfx/src/xlib/nsFontMetricsXlib.cpp
+  */
+ nsFontEnumeratorXP::HaveFontFor(const char* aLangGroup, PRBool* aResult)
+ {
+   *aResult = PR_FALSE;
+   *aResult = PR_TRUE; // always return true for now.
+   // Finish me - ftang
+   return NS_OK;
+ }
-- snip --

Can anyone check this in, please ?

Comment 7

17 years ago
r=cls.  Still need a sr= and then an a= from drivers@mozilla.org.  We need to
get this into the 0.8 branch too. 
Maybe we should also fix photon and qt?  Then we should have all implementations
of nsIFontEnumerator fixed.
BTW, is this really going to be implemented in the other ports?  If not, it
should probably not be in nsIFontEnumerator, so we don't have to write unused
code for all the other ports.

Reopening bug, since there are still 3 ports where the stub hasn't been checked in.
Resolution: FIXED → ---
So is Windows the only platform with a non-stub impl?  How about we use #ifdefs
to provide a stub for all other platforms, in some XP file, instead of forcing a
long march through the ports?

Or are non-stub impls for other platforms really forthcoming, soon?  Frank, what
is the plan?


Comment 11

17 years ago
<Insert token comment about how non-XP checkins are A Bad Thing(tm).>

diff -u -r1.2 nsIFontEnumerator.idl
--- nsIFontEnumerator.idl       2001/02/06 12:48:44     1.2
+++ nsIFontEnumerator.idl       2001/02/16 10:13:31
@@ -47,9 +47,11 @@
   void EnumerateFonts(in string aLangGroup, in string aGeneric,
     out PRUint32 aCount, [retval, array, size_is(aCount)] out wstring aResult);
+#ifdef XP_WIN32
     @param  aLangGroup language group
     @return bool do we have a font for this language group
   void HaveFontFor(in string aLangGroup, [retval] out boolean aResult);

Priority: -- → P1
If it's only going to be used on Windows, there's no need for it to be in the
IDL at all, since the windows usage is from within the windows code where it's
used as an nsFontEnumeratorWin...

ftang:  Do you intend to implement this on other platforms, or is it only for
the Windows code?

Comment 13

17 years ago
>ftang:  Do you intend to implement this on other platforms, or is it only for
the Windows code?

My answer is YES. I will implement the real code for Linux and Mac. And I will
call it from JavaScript from Pref.

Comment 14

17 years ago
can you #ifdef in idl ?

Comment 15

17 years ago
1. I really think adding #ifdef code in idl and these kind of API is bad2
2. I don't mind add stub for other port but I do not feel comfortable to check
in some code that I didn't compile in those platform. I can add them if mozilla
allow me to check in without compile on qt, phanton, etc. (I do build my stub on
Mac, Linux before I check in because I have these build environment in  my cube).

Brendan- what do you think? Which rule you want me to break? ignore ports, or
check in code which I didn't compile ? :)

ftang: earlier I had suggested using a combination of #ifdefs and stubs.  There
is no need to make an identical stub for every platform you or a platform
champion has not yet supported.  As for adding a stub that might not compile
(presumably you are worried about a typo), it can't be any worse than not having
the stub in the first place, and toasting a bunch of platforms.


Comment 17

17 years ago
I agree with Frank that it would be better to avoid ifdefs inside the IDL file,
and just have a stub in each platform implementation file. I don't think it
should be a requirement for Frank (or anybody else) to compile on all of the
platforms before checking in. Just copy the stub to all platforms, check in,
and watch tinderbox (for those platforms that have one).

An ifdef/stub combo might be possible too, but I personally don't think it's
worth the trouble.
Just to re-iterate:

Erik's right, how to stub and share stubs is up to the person who adds the
stubs, and http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Ports is
there to keep an eye on, and always has been.  There is no rule that you have to
compile your code on all ports platforms *before* checking it in.

Created attachment 25575 [details] [diff] [review]
the rest (I think) of the necessary stubs


17 years ago
Blocks: 49947

Comment 20

17 years ago
sr=erik for attachment id 25575

David, it's great that you're doing this, but Frank, maybe it would have been
nice if you did this in the first place?
ftang - Any interest in reviewing this and checking it in?  I agree that I
probably shouldn't be the one doing this.  It also should have been done two
weeks ago, since this is causing build bustage for a number of people and
forcing them to each fix it on their own.
*** Bug 69465 has been marked as a duplicate of this bug. ***

Comment 23

17 years ago
check in fix for qt , phonton and xprint.
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.