Closed Bug 73273 Opened 24 years ago Closed 24 years ago

gFontsHaveConverters needs to become gCharsetFontsProperties

Categories

(Core :: MathML, defect)

x86
Windows 95
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla0.9.1

People

(Reporter: rbs, Assigned: rbs)

References

Details

(Whiteboard: fixinhand (py8ieh:reverify if applicable))

Attachments

(7 files)

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} };
Blocks: 71408
Attached patch proposed patch (deleted) — Splinter Review
Attached file makefiles (deleted) —
[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.]
Summary: gFontsHaveConverters needs to become gEncodingProperties → gFontsHaveConverters needs to become gEncodingProperties
Of note: @@ -1291,55 +1242,51 @@ is just whitespace (change of indentation) because one block level {...} is removed. There is no change there.
Want to allow your much sought-after r/sr on this, guys?
Summary: gFontsHaveConverters needs to become gEncodingProperties → gFontsHaveConverters needs to become gCharsetFontsProperties
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."));
+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 :-)
@@ -2918,6 +2862,7 @@ **** [to be ignored - left over of debug/inspection] nsFontWin* nsFontMetricsWin::FindFont(HDC aDC, PRUnichar aChar) { +NS_ASSERTION(aChar!=0x007D, "Break");
Whiteboard: fixinhand
Target Milestone: --- → mozilla0.9
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.
Target Milestone: mozilla0.9 → mozilla0.9.1
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
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.
Attached patch Updated patch (deleted) — Splinter Review
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.
Status: NEW → ASSIGNED
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
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.
Attached patch Latest (deleted) — Splinter Review
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.
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.]
brendan, can you sr= this little gem for me to check in. Thanks.
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
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.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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!
Status: RESOLVED → VERIFIED
Whiteboard: fixinhand → fixinhand (py8ieh:reverify if applicable)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: