195038 - Fix the remaining Symbol Font support issue

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Katsuhiko Momoi]

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

Comment 1

[Michael Buckland]

Discussed in edt.  Minusing because currently no embedding customer requests for
this, consequently not a high priority.

Comment 2

[Paul Wyskoczka]

adt: nsbeta1-

Comment 3

[rbs]

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

Comment 4

[rbs]

-> taking

Comment 5

[rbs]

Attachment: fix, take 2

same fix, with an added one-liner that jshin forgot in 205387 comment 24.
[see @@ -5289,6 +5303,7 @@]

Comment 6

[rbs]

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.

Comment 7

[Jungshik Shin]

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

[rbs]

Attachment: 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...

Comment 9

[rbs]

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

jshin, care to take another look?

Comment 10

[Jungshik Shin]

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

[rbs]

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

Comment 12

[Jungshik Shin]

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.

Comment 13

[rbs]

Checked in.