rbs Assignee
	Description • 24 years ago

Currently, fonts for which we have converters are hard-coded in nsFontMetricsWin
as showned below, and then a linear search is used whenever it has to be 
determined whether a converter exists for a font. 

Not only is the linear sarch sub-optimal, but the overall approach doesn't scale
well -- given that to add a new converter requires re-compilation.

It would be much better to keep this information externally, in a 
res/fonts/encoding.properties, and let the built-in hashing mechanism of
the properties API take care of the rest (search, etc.)

I am thinking of setting up the encoding.properties file along the lines:
encoding.symbol.ttf = Adobe-Symbol-Encoding
encoding.cmsy10.ttf = x-ttf-cmsy
...etc

This is sufficient to fix the Win32 context. The Win32 code could then retrieve 
a converter by just looking up: 'encoding.' + fontname + '.ttf';

Erik, if you think the approach might be of some interest in the Linux context 
too, and have some wider thoughts, the scheme could be extended without problems
to various situations (besides the encoding prefix). Hence this property file 
could be used on all platforms for other data that don't necessarily need to be
hard-coded.

static nsFontHasConverter gFontsHaveConverters[] =
{
/* Adobe' Symbol font */
  {"Symbol", "Adobe-Symbol-Encoding"},
#ifdef MOZ_MATHML
/* TeX's Computer Modern fonts (Symbol and Extension) */
  {"CMSY10", "x-ttf-cmsy"},
  {"CMEX10", "x-ttf-cmex"},
/* Mathematica fonts */
  {"Math1",          "x-mathematica1"},
  {"Math1-Bold",     "x-mathematica1"},
  {"Math1Mono",      "x-mathematica1"},
  {"Math1Mono-Bold", "x-mathematica1"},

  {"Math2",          "x-mathematica2"},
  {"Math2-Bold",     "x-mathematica2"},
  {"Math2Mono",      "x-mathematica2"},
  {"Math2Mono-Bold", "x-mathematica2"},

  {"Math3",          "x-mathematica3"},
  {"Math3-Bold",     "x-mathematica3"},
  {"Math3Mono",      "x-mathematica3"},
  {"Math3Mono-Bold", "x-mathematica3"},

  {"Math4",          "x-mathematica4"},
  {"Math4-Bold",     "x-mathematica4"},
  {"Math4Mono",      "x-mathematica4"},
  {"Math4Mono-Bold", "x-mathematica4"},

  {"Math5",          "x-mathematica5"},
  {"Math5-Bold",     "x-mathematica5"},
  {"Math5 Bold",     "x-mathematica5"}, // typo in the Mathematica pack?
  {"Math5Mono",      "x-mathematica5"},
  {"Math5Mono-Bold", "x-mathematica5"},
  {"Math5Mono Bold", "x-mathematica5"}, // typo in the Mathematica pack?
/* MathType Extra */
  {"MT Extra",       "x-mtextra"},
#endif
  {nsnull, nsnull}
};

	rbs Assignee
	Comment 4 • 24 years ago

[Although not (yet) relevant to Linux -  I also updated the makefile.in]

On Win32 - I fixed the XXX comment about the dangling converter by simply
switching to nsCOMPtr.

[BTW, Erik - I have noted that something wasn't quite right with what Roy did,
but I haven't done anything about it. I will file a separate bug against Roy
later. Here is a little summary of the problem. Your idea of the re-use was if
the "map" was identical. Roy's implementation simply checked if the "name" was
in the hash - but this check is redundant and irrelevant because it is already
done upon entering GetCMAP. It has not connection with the intended re-use. 
Unfortunately, we missed this problem when reviewing.]

	rbs Assignee
	Comment 5 • 24 years ago

Of note: @@ -1291,55 +1242,51 @@ is just whitespace (change of indentation)
because one block level {...} is removed. There is no change there.

	rbs Assignee
	Comment 6 • 24 years ago

Want to allow your much sought-after r/sr on this, guys?

	kill this account
	Comment 7 • 24 years ago

I'm not an expert in windows fonts so I can't really review the code for 
correct functioning but I'll just add some general comments:

 +static nsCOMPtr<nsIPersistentProperties> gCharsetFontsProperties; 

Probably should to avoid a global nsCOMPtr as I've heard this can cause problems 
on some systems; see David Baron's comments in 
http://bugzilla.mozilla.org/show_bug.cgi?id=55021

You might want to use the string macro NS_LITERAL_STRING; eg:
 name.AssignWithConversion("charset.");
vs
 name.Assign(NS_LITERAL_STRING("charset."));

	rbs Assignee
	Comment 8 • 24 years ago

+static nsCOMPtr<nsIPersistentProperties> gCharsetFontsProperties; 
I thought things will be ok by explicitly zeroing the pointer in FreeGlobals():

@@ -164,6 +169,9 @@
 
   gInitialized = 0;
 
+  // destroy the special resolver of fonts' encoding names
+  gCharsetFontsProperties = nsnull;
+

I will be attaching another patch without the controversial static :-)

> name.Assign(NS_LITERAL_STRING("charset."));
I will update as requested.

BTW, this usage pattern is so widespread that I have been wondering why there
isn't ASSIGN_LITERAL_STRING -- or something similar -
cc:ing scc/waterson if my request can be of any effect :-)

	rbs Assignee
	Comment 10 • 24 years ago

@@ -2918,6 +2862,7 @@ **** [to be ignored - left over of debug/inspection]
 nsFontWin*
 nsFontMetricsWin::FindFont(HDC aDC, PRUnichar aChar)
 {
+NS_ASSERTION(aChar!=0x007D, "Break");

	rbs Assignee
	Comment 11 • 24 years ago

Shifting to next milestone. I am disappointed that I haven't been able to make 
further progress on this due to lack of r/sr while the code was hanging here
for quite some time.

	Brendan Eich [:brendan]
	Comment 12 • 24 years ago

rbs: sorry, I probably should have found this patch awaiting sr= -- I've been
trying to cover for erik.  Who should r= it?  I'm cc'ing myself, will keep up
and sr= after r= is in the bug.

/be

	Erik van der Poel
	Comment 13 • 24 years ago

Hi Roger,

Sorry about the delay. I had a look at the patch. Regarding the following:

  if (NS_FAILED(rv)) return NS_ERROR_FAILURE;

Perhaps it should just return rv? Otherwise you lose the reason why it failed.
I.e. is there any reason not to propagate the error code?

I thought I saw some discussion about removing the "res" directory, so it might
be better to move the property file to some other location. I'm sorry, but I
can't be more specific than that. Who's the packaging owner?

Also, I wonder if mozilla/intl is really the right location for the source of
the property file. Shouldn't it just be under mozilla/gfx/src/windows? Or
perhaps under mozilla/gfx/src if it might be used on platforms other than
Windows (e.g. Unix TrueType, OS/2).

I believe kmcclusk or one of his peers normally doles out r= for gfx, but since
I wrote so much of nsFontMetricsWin.cpp, I guess I could give r=erik. Cc'ing
kmcclusk in case he feels differently.

	rbs Assignee
	Comment 15 • 24 years ago

I revised the patch to:

1) sync with the tree

2) propagate rv

3) move the property file to mozilla/gfx/src/windows, so that each platform
could be free to customize the content as they see fit.

Re: 'res'. I thought it was the protocol 'res' that was deprecated in favor of 
'resource'. However, if the 'res' directory is scheduled to go away too, then
the property file will be relocated along with the other massive relocations.
So it should be okay for the time being.

	Erik van der Poel
	Comment 17 • 24 years ago

OK, looks good.

I only have one minor concern now. "charsetFonts" sounds a bit strange. How
about "fontEncodings"? Just a suggestion. (Not a requirement for r=erik.)

Thanks!

r=erik

	rbs Assignee
	Comment 18 • 24 years ago

Agree. That's what I initially thought. But as I was setting up the file in
intl, I noted that the local custom there was to use charset[Something], e.g.,
charsetData.properties. But now that the setup is in gfx, I feel somewhat free
to substitute as appropriate:

'charset' -> 'encoding'
'gCharsetFontsProperties' -> 'gFontEncodingProperties'

This might help in highlighting the distinction with usual document charsets.

	rbs Assignee
	Comment 20 • 24 years ago

I attached the latest diff in which I made the substitution. The property file
is the same, except that it is now called 'encoding.properties' and its content
has s/charset/encoding/g.

	rbs Assignee
	Comment 21 • 24 years ago

OK, ready to go; In the end, I renamed the file to 'fontEncoding.properties'.
Got r=erik, seeking sr= to check-in. [I will be providing another patch for
bug 74494 afterwards.]

	rbs Assignee
	Comment 22 • 24 years ago

brendan, can you sr= this little gem for me to check in. Thanks.

	Brendan Eich [:brendan]
	Comment 23 • 24 years ago

One problem only:

+  nsAutoString uriStr;
+  uriStr.Assign(NS_LITERAL_STRING("resource:/res/fonts/encoding.properties"));
+  if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), uriStr))) {

No point in copying to an nsAutoString, then to pass it to NS_NewURI(nsIURI*
*result, nsAReadableString& spec, ...).  I would like it if you could just pass
NS_LITERAL_STRING directly, but IIRC there's some portability problem with that.

But notice that that overloading of NS_NewURI, from
netwerk/base/public/nsNetUtil.h, simply converts its spec param to UTF8.  So why
not use the other overloading, that takes a const char* spec, which is presumed
to be UTF8 already?  Then you could pass the C string literal and avoid both
NS_LITERAL_STRING and nsAutoString.

Fix that, and sr=brendan@mozilla.org.

/be

	rbs Assignee
	Comment 24 • 24 years ago

Fixed as per your tips. Indeed, it is much better and speedy to just use the 
other overloaded variant... Landed on the m0.9.1 trunk. Resolving as FIXED.

	Hixie (not reading bugmail)
	Comment 25 • 24 years ago

How do I, er, verify this? Seems to be a code-level bug. I'm going to rubber-
stamp verify this for now to get it off my radar, if you can explain how I 
would verify this I will actually go ahead and do so. Thanks!

	rbs Assignee
	Comment 26 • 24 years ago

Yep, it is a code-level stuff:
http://lxr.mozilla.org/seamonkey/search?string=gFontEncodingProperties