Fix the remaining Symbol Font support issue

RESOLVED FIXED in mozilla1.6beta



16 years ago
14 years ago


(Reporter: momoi, Assigned: rbs)



Windows 2000
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)




(1 attachment, 2 obsolete attachments)



16 years ago
** Observed with the current trunk build **

The final fix in Bug 94319 addressed many fonts that contain symbolic
characters but the Symbol font was left out of the solution for technical reasons.
It is solvable according to the engineers involved (ftang & shanjian) but
requires additional engineerign time.

I think it might be a good idea to create a new bug to adrdess the remaining
Symbol Font fix and decided to file this bug. 

The test case and current patch are in Bug 94319.
Nominating for nsbeta1 and topembed.
Blocks: 194560

Comment 1

16 years ago
Discussed in edt.  Minusing because currently no embedding customer requests for
this, consequently not a high priority.
Keywords: topembed → topembed-

Comment 2

16 years ago
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-

Comment 3

15 years ago
Created attachment 132976 [details] [diff] [review]
possible fix

basic ideas in the fix:

1) The patch uses a little '0' or '1' prefix in the hashtable to differentiate
the quirky behavior from the standards behavior: @see GetNAME()
For example,
 1Symbol -> maps to the quirky ccmap when the font is symbolic.
 0Symbol -> maps to the standards (Unicode) ccmap.

Hence all non-symbolic font such as Times New Roman, etc, are kept in the form
'0'+name in the font map hashtable. In summary, they are left alone.

[FYI: something for people hacking the Windows font code... In reality,
GetNAME() is a _long_ system/internal name of the font. We use it to uniquely
distinguish fonts in the hashtable. It is not usually the short typeface name
that people are familiar with. Think of it as a variant of the XLFD name on
Unix. On my Win2K for example, GetNAME() for the Symbol font gives
"Monotype:Symbol version 1.6 (Microsoft)". Hence the prefixed version that goes
in the hashtable is "1Monotype:Symbol version 1.6 (Microsoft)".]

2) Wide fonts (Korean, Tamil) are left alone. The patch doesn't allow them to
be used as quirky fonts.

3) The familyNameQuirks is passed as argument in LoadFont() for two
reasons. First, in order to propagate it to GetConverter(). This creates the
usual cascading effect. Secondly, the handling of generic fonts and global
fonts must not be affected by the quirks. FindLocalFont() is the one that
examines local fonts, i.e., those specified in <font face="..."> or
"font-family: ...". It is only in FindLocalFont() that the quirks flag is
passed down to LoadFont().


15 years ago
Blocks: 33127

Comment 4

15 years ago
-> taking
Assignee: bobj → rbs

Comment 5

15 years ago
Created attachment 133037 [details] [diff] [review]
fix, take 2

same fix, with an added one-liner that jshin forgot in 205387 comment 24.
[see @@ -5289,6 +5303,7 @@]
Attachment #132976 - Attachment is obsolete: true

Comment 6

15 years ago
Comment on attachment 133037 [details] [diff] [review]
fix, take 2

r=? jshin? 

Note: As in the initial bug 94319, this fix doesn't extend to those
'A'-functions that are used to work-around an OS bug on Win95-Japanese. I
wonder if the marketshare still warrants to keep those functions around.
Attachment #133037 - Flags: review?(jshin)

Comment 7

15 years ago
Comment on attachment 133037 [details] [diff] [review]
fix, take 2

>-GetConverter(const char* aFontName, nsIUnicodeEncoder** aConverter, PRBool* aIsWide = nsnull)
>+GetConverter(const char* aFontName, PRBool aNameQuirks,
>+  nsIUnicodeEncoder** aConverter, PRBool* aIsWide = nsnull)
> {
>   *aConverter = nsnull;
>+  if (aNameQuirks) {
>+    if (aIsWide)
>+      *aIsWide = PR_FALSE;
>+    return GetDefaultConverterForTTFSymbolEncoding(aConverter);

  Wouldn't this make 'wide' fonts regarded as symbol fonts when specified via
'font' tag(quirky mode on)? Could you make it behave as regular unicode fonts
(at least)?

Comment 8

15 years ago
Created attachment 133204 [details] [diff] [review]
patch - take3

Good catch... here goes another attempt: the spec is in comment #3, all I want
is to get the code to do it right...
Attachment #133037 - Attachment is obsolete: true


15 years ago
Attachment #133037 - Flags: review?(jshin)

Comment 9

15 years ago
Comment on attachment 133204 [details] [diff] [review]
patch - take3

jshin, care to take another look?
Attachment #133204 - Flags: review?(jshin)

Comment 10

15 years ago
Comment on attachment 133204 [details] [diff] [review]
patch - take3

r=jshin with the following change

>@@ -713,6 +713,9 @@
>     p += 2;
>     // encoding: 1 == Unicode; 0 == symbol
>     if ((platform == 3) && ((encoding == 1) || (!encoding)) && (name == 3)) {
>+      if (aIsSymbolEncoding) {
>+        *aIsSymbolEncoding = encoding == 0;
>+      }

  |encoding| is not refered to	after the above block so that it'd be better to
just set |*aIsSymbolEncoding| to PR_FALSE.

Comment 11

15 years ago
Note the earlier |(encoding == 1) || (!encoding)|
I wish to capture the case where |encoding == 0|.


15 years ago
Attachment #133204 - Flags: superreview?(roc)

Comment 12

15 years ago
Comment on attachment 133204 [details] [diff] [review]
patch - take3

should have marked r here.

BTW, as for my comment, sorry I misread the code. you're right.
Attachment #133204 - Flags: review?(jshin) → review+
Attachment #133204 - Flags: superreview?(roc) → superreview+


15 years ago
Component: Internationalization → Layout: Fonts and Text
Target Milestone: --- → mozilla1.6beta

Comment 13

15 years ago
Checked in.
Last Resolved: 15 years ago
Resolution: --- → FIXED


14 years ago
Blocks: 94319
You need to log in before you can comment on or make changes to this bug.