541924 - bad underline font list doesn't work

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The bad underline font list mechanism is completely broken on Windows.

Jan.7 2010 build works fine, but Jan.8 2010 build and later broken.

See bug 417014 for the detail of "bad underline font list".

Comment 1

[Jonathan Kew [:jfkthame]]

This will no doubt be a regression from the Windows patch in bug 493280.

Could you be more specific about the problems you're seeing? Using the testcase from bug 417014 and comparing a current trunk build to FF 3.5.5 on Vista, I'm not seeing differences. Does this issue apply only on certain versions of Windows, or only with a few of the fonts? A specific testcase for a problem font, together with screenshots comparing the expected result and the broken version would be nice.

Even better, could we get a reftest for bug 417014?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not sure you can test it on English Windows because you need to install MS PGothic or something.

You need to specify the bad font family to the input field. The bad fonts are listed in the pref. I'll post the screenshots.

I have no idea for the reftests because the bad fonts are not installed on the test machines, probably...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot (2010-01-07)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot (2010-01-24)

Comment 5

[Jonathan Kew [:jfkthame]]

Which font is used in your screenshots - is this MS PGothic?

Which version of Windows are you using? I'm not seeing the problem here (with today's Minefield nightly) on either XP or Vista. (This is with the USEnglish version of Windows, but the MS PGothic font and other Japanese fonts are available.)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #5)
> Which font is used in your screenshots - is this MS PGothic?

Yes, it's default setting's Japanese font.

> Which version of Windows are you using? I'm not seeing the problem here (with
> today's Minefield nightly) on either XP or Vista. (This is with the USEnglish
> version of Windows, but the MS PGothic font and other Japanese fonts are
> available.)

I'm using Windows 7.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to comment #6)
> I'm using Windows 7.

Interesting. I don't have a Win7 machine, but presumably bug 417014 must have affected earlier versions also, as it's nearly 2 years old.

Are you able to check this with XP and/or Vista as well? I'm puzzled why you're seeing this problem but I am unable to reproduce it here.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, but I'll go to bed, I'll check it tomorrow.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I can reproduce this bug on both WinXP and WinVista too.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

gfxGDIFontList::InitFontList() calls InitBadUnderlineList(), however, I think that we've not loaded alias family names at this time, i.e., English family names which are included in the list are not loaded on Japanese locale Windows. I think that this is the cause.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: patch v1: load the underline blacklist into a hashset and check all family names against it

Ok, I was able to reproduce the issue on English Windows by setting the system locale to Japanese.

This patch changes the implementation of the blacklist so that instead of marking the "bad" fonts as a batch, after loading the list of families (but before "extra" localized family names have been read), it stores the blacklist in a hash and checks family names as they're added to the tables. This allows the blacklist to match on either the primary or "extra" names, and appears to resolve the problem when running under the Japanese locale.

A tryserver build with this patch is available at
http://build.mozilla.org/tryserver-builds/[e-mail]-try-25be2663df4d

Comment 12

[John Daggett (:jtd)]

Comment on attachment 423801 [details] [diff] [review]
patch v1: load the underline blacklist into a hashset and check all family names against it

Looks good.

Comment 13

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/d96f5ba97233

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: followup patch - we should use our nsStringHashSet class here

It seems like it would be better style to use nsStringHashSet here. (No functional change involved.)

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

-> v.

Thank you.

Comment 16

[Jonathan Kew [:jfkthame]]

Pushed followup (comment 14):
http://hg.mozilla.org/mozilla-central/rev/5075372b8455