Closed Bug 352174 Opened 18 years ago Closed 18 years ago

need font-family resolver (cannot resolve alias of font family / is not checking whether the specified font family is installed the system)

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Whiteboard: [blocking1.9a1+])

Attachments

(9 files, 12 obsolete files)

GetTextFace() test program
1.35 KB, text/plain
Details
Testcase
1.76 KB, text/html; charset=UTF-8
Details
GTK2 part(rv1.1)
26.41 KB, patch
Details | Diff | Splinter Review
Mac part (rv1.1)
3.78 KB, patch
Details | Diff | Splinter Review
BeOS part that is same as gtk2 platform (rv1.1)
14.39 KB, patch
Details | Diff | Splinter Review
Patch rv1.1.1
77.58 KB, patch
pavlov
: review+
vlad
: review+
Details | Diff | Splinter Review
XP part (rv1.1.1)
10.59 KB, patch
Details | Diff | Splinter Review
Win part (rv1.1.1)
13.59 KB, patch
Details | Diff | Splinter Review
Testcase
149 bytes, text/html
Details 
I think that we need the "font-family resolver" that is converter the given font family to actual font family. E.g., on all platforms, we need to check whether the font is existing on the system. I think that by this checking we can fix bug 352059. And by fontconfig, a font family may be an alias for the multiple fonts. I think that if we can resolve the alias, we can fix completely the bug 339513.

I'll create the patch after bug 339513.
I've tried making us do checks for font existance on windows and it breaks too many sites that depend on the behavior of the windows font mapper.  I don't think we want to do this, at least on windows.
(In reply to comment #1)
> I've tried making us do checks for font existance on windows and it breaks too
> many sites that depend on the behavior of the windows font mapper.  I don't
> think we want to do this, at least on windows.

Really? I seems that the old gfx code checked it.

http://lxr.mozilla.org/mozilla/source/gfx/src/windows/nsFontMetricsWin.cpp#2464
> 2460 nsFontWin*
> 2461 nsFontMetricsWin::LoadFont(HDC aDC, const nsString& aName, PRBool aNameQuirks)
> 2462 {
> 2463   LOGFONT logFont;
> 2464   HFONT hfont = CreateFontHandle(aDC, aName, &logFont);
> 2465   if (hfont) {
> 2466     HFONT oldFont = (HFONT)::SelectObject(aDC, (HGDIOBJ)hfont);
> 2467     char name[sizeof(logFont.lfFaceName)];
> 2468     if (::GetTextFace(aDC, sizeof(name), name) &&
> 2469         !strcmpi(name, logFont.lfFaceName)) {


CreateFontIndirect() followed by GetTextFace() in most circumstances returns the same thing passed in -- not the new or correct font name.  I don't think I've ever seen a case where CFI() actually doesn't return a valid object.  If the name doesn't match the windows font mapper will find something that matches the characteristics of the font.
> CreateFontIndirect() followed by GetTextFace() in most circumstances returns
> the same thing passed in -- not the new or correct font name.
It doesn't return the same font in some environment, especially in non-English locale. For example, bug 342143 and bug 349469 are caused by this discrepancy.
> I don't think I've ever seen a case where CFI() actually doesn't return a valid object.
Yeah, it will always return the valid HFONT because it simply copy the LOGFONT into kernel. But we can't predict what will be actually selected.
> If the name doesn't match the windows font mapper will find something that matches the characteristics of the font.
That behavior violates the CSS 2.1 spec. We should follow the way that legacy gfx did.
> I've tried making us do checks for font existance on windows and it breaks too
> many sites
When did you try? Bug 346786 would fix the most cases.
Why can legacy-gfx based Mozilla render many sites fine if that check breaks them?
> -- not the new or correct font name.
True, you should try the next candidate if CFI() and GetTextFace() didn't agree (as nsFontMetricsWin did) because it means the font doesn't exist.
> True, you should try the next candidate if CFI() and GetTextFace() didn't agree
> (as nsFontMetricsWin did) because it means the font doesn't exist.
> 

I've never see a case where CFI() and GetTextFace() don't agree on the name of the font.  If you can point me at one, I'll take a look.
(In reply to comment #7)
> I've never see a case where CFI() and GetTextFace() don't agree on the name of
> the font.  If you can point me at one, I'll take a look.
See bug 342143 and bug 349469.
Bug 342143 testcase:
CFI()=aaaaa -> GetTextFace()=ＭＳ Ｐゴシック
Bug 349469 testcase:
CFI()=NotARealFontNamePlease -> GetTextFace()=ＭＳ Ｐゴシック
Note that this is just an example. We can't predict what is returned from GetTextFace(). For example, bug 342143 testcase will return Symbol on some environment (per reporter).

That said, I'll withdraw my idea because GetTextFace() couldn't handle CJK localized font name correctly.
CFI()=MS Mincho -> GetTextFace()=ＭＳ 明朝
CFI()=MingLiU -> GetTextFace()=MingLiU
CFI()=ＭＳ 明朝 -> GetTextFace()=ＭＳ 明朝
CFI()=細明體 -> GetTextFace()=MingLiU
*** Bug 354349 has been marked as a duplicate of this bug. ***
Blocks: 354349
*** Bug 354349 has been marked as a duplicate of this bug. ***
No longer blocks: 354349
Status: NEW → ASSIGNED
Summary: need font-family resolver → need font-family resolver (cannot resolve alias of font family / is not checking whether the specified font family is installed the system)
This bug should be linux only.  We aren't going to be able to fix this on windows in any way similar to how you would fix it on linux.
(In reply to comment #11)
> This bug should be linux only.  We aren't going to be able to fix this on
> windows in any way similar to how you would fix it on linux.

We need the platform dependent code for each OS, but I think that we should checking (calling the resolver) in |gfxFontGroup::ForEachFontInternal| that is XP code. Because the font existing checking is needed on all platforms. But some platforms may not have the alias font family (I don't know Mac). However, even if so, we can check whether the font exists. (I think that we should get the result by callback function. If the callback function is not called, the system doesn't have the font family. And if the font family is the alias for 2 or more fonts, the resolver will call the times, simply.)

Of course, if the patch grows up, I will separate the bug.
See nsIDeviceContext::GetLocalFontName(aFontName, localName, aliased)
it checks the aliasing.

>I've never see a case where CFI() and GetTextFace() don't agree on the name of
>the font.  If you can point me at one, I'll take a look.

This _presumes_ the existence of the font. Whereas you want to _move on_ to the next font in the CSS font-family list. See e.g., bug 27005.

If the CSS has "font-family: nonexistingfont, verdana", and you do a ::SelectObject(DC, "nonexistingfont"), the font-mapper will always fallback and set some random default font in the DC, and thus GetTextFace() will give something different, whereas you instead want to move on to the next font in the CSS list (verdana) so that the implementation abides by the CSS spec.
see bug 35824 for some instructive comments about aliasing so that you don't re-invent the wheel.
Legacy GFX hard-coded mappings between localized names and english names because Win9x didn't support both localized names and english family names (see bug 231426).
I want a more generic solution because Win9x support was dropped.
Attached patch Patch rv0.1 (work in progress) (obsolete) — — Splinter Review 
This patch works only on Windows.
I'll append other platform code. (this patch cannot build on other platforms.)
This is the wrong approach to take, at least on windows.  You're much better off just creating the font objects and checking in gfxWindowsFont::MakeHFont and setting a flag to ignore that font early on.  The way you've done it makes us have to do an iteration of the hashtable of fonts, then indexOf the array you built from the hashtable -- all on every single time through.  And you'll do this for all the fonts listed, including those not used.  Creating a gfxWindowsFont is almost free since everything about them is done lazily.

I'm not sure how this is even going to work though for i18n fonts since they can have > 1 name without adding a list like we had before (which I'm not going to take in this code...)
(In reply to comment #8)

> That said, I'll withdraw my idea because GetTextFace() couldn't handle CJK
> localized font name correctly.
> CFI()=MS Mincho -> GetTextFace()=ＭＳ 明朝
> CFI()=MingLiU -> GetTextFace()=MingLiU
> CFI()=ＭＳ 明朝 -> GetTextFace()=ＭＳ 明朝
> CFI()=細明體 -> GetTextFace()=MingLiU

Does this happen even with GetTextFaceW()?
 
(In reply to comment #15)
> Legacy GFX hard-coded mappings between localized names and english names

It's not hard-coded, but the mapping table is stored in a properties file that can be changed if necessary. Anyway, I agree we should do without it if we can.
Blocks: 342143
(In reply to comment #17)
> This is the wrong approach to take, at least on windows.  You're much better
> off just creating the font objects and checking in gfxWindowsFont::MakeHFont
> and setting a flag to ignore that font early on.  The way you've done it makes
> us have to do an iteration of the hashtable of fonts, then indexOf the array
> you built from the hashtable -- all on every single time through.  And you'll
> do this for all the fonts listed, including those not used.  Creating a
> gfxWindowsFont is almost free since everything about them is done lazily.

No, I think that you misread my patch. On Windows, this patch doesn't use |gfxPlatform::ResolveFontName| that is using hashtable. Instead of it, it uses |gfxWindowsPlatform::ResolveFontName| for checking alias. If the cost of this function is expensive, we should check the hashtable of gfxWindowsPlatform before current checking.

Please recheck my patch.
(In reply to comment #19)
> No, I think that you misread my patch. On Windows, this patch doesn't use
> |gfxPlatform::ResolveFontName| that is using hashtable. Instead of it, it uses
> |gfxWindowsPlatform::ResolveFontName| for checking alias. If the cost of this
> function is expensive, we should check the hashtable of gfxWindowsPlatform
> before current checking.
> 
> Please recheck my patch.
> 

Sorry -- you're right, but EnumFontFamiliesExW is very expensive and you don't want to use it.  gfxWindowsPlatform already has a hashtable with all the fonts in it that would be much cheaper to look things up using.  I still think that you want to do this lazily in MakeHFont and not in ForEachFont()
Attached patch Patch rv0.1.1 (obsolete) — — Splinter Review 
Adding the comment in |gfxPlatform::ResolveFontName|.
Attachment #240842 - Attachment is obsolete: true
(In reply to comment #20)
> I still think that you want to do this lazily in MakeHFont and not in ForEachFont

Why? I think that the redundant |gfx*Font| raises the cost of Uniscribe/Itemizing rendering. (Especially, if the text has some characters that is not in the fonts of the platform. I.g., if the text is garbled. E.g, Mojibake. That is often occurred in CJK web pages.)

And I think that the way is very lengthy in Linux/BeOS. On these platforms, a font name is the alias to *some* fonts.
(In reply to comment #22)
> I think that the redundant |gfx*Font| raises the cost of
> Uniscribe/Itemizing rendering. (Especially, if the text has some characters
> that is not in the fonts of the platform. I.g., if the text is garbled. E.g,
> Mojibake. That is often occurred in CJK web pages.)

Sorry, this paragraph is wrong. Please ignore this.
For the present, I'll write the code for Linux/BeOS in current way. But if the code can be changed to simply by the way of Stuart, we can change the way in that time.
(In reply to comment #18)
> (In reply to comment #8)
> > That said, I'll withdraw my idea because GetTextFace() couldn't handle CJK
> > localized font name correctly.
> > CFI()=MS Mincho -> GetTextFace()=ＭＳ 明朝
> > CFI()=MingLiU -> GetTextFace()=MingLiU
> > CFI()=ＭＳ 明朝 -> GetTextFace()=ＭＳ 明朝
> > CFI()=細明體 -> GetTextFace()=MingLiU
> 
> Does this happen even with GetTextFaceW()?
Yes, here's my test program. I defined UNICODE/_UNICODE macro.
Note that I tested this on Japanese system locale. If you are using Korean locale, GetTextFace() will return "MS Mincho" instead of "ＭＳ 明朝" and it will return "굴림체" instead of "GulimChe".
(In reply to comment #20)
> gfxWindowsPlatform already has a hashtable with all the fonts in it
All localized font names are missing from that "all font list" on your (English) locale. And English names for Japanese fonts (for example, "MS Mincho") and localized font names for non-Japanese fonts are missing from it on our (Japanese) locale. And English names for Korean fonts (e.g. "GulimChe") and localized font names for non-Korean fonts would be missing on Jshin's (Korean) locale.
EnumFontFamiliesExW with specific font name is required to save their missing names. Otherwise we would have to introduce ugly mapping table hack again.
Of course, there is room for improvement from the performance perspective. If you found missing names, you could add them to the hashtable immediately for future reference.
Attached file Testcase — 
Current trunk build would fail some tests.
Bon Echo (that is, non-cairo build) would pass the all tests.
Attached patch Patch rv0.2 (work in progress) (obsolete) — — Splinter Review 
This is using the hashtable and caching the alias names.
Attachment #240895 - Attachment is obsolete: true
Blocks: 334728
And I remembered a thing that is FontLink. After Win2k supports the FontLink. But our code doesn't support. But I think that my patch may be able to implement it easily.(Of course, that is another bug, not in this scope.)
Attached patch Patch rv0.3 (work in progress) (obsolete) — — Splinter Review 
This can build on Linux too. But the performance is very very slowly.
And this patch can resolve the alias, but cannot check the non-existing font...
Attachment #240934 - Attachment is obsolete: true
Attached patch Patch rv0.5 (work in progress) (obsolete) — — Splinter Review 
The font list cache doesn't work fine...
Attachment #241108 - Attachment is obsolete: true
Attached patch Patch rv0.6 (work in progress) (obsolete) — — Splinter Review 
This patch makes the cache for font list. If this patch doesn't have new issues(other than performance), I'll finish this work.
Attachment #241487 - Attachment is obsolete: true
Flags: blocking1.9?
Attached patch Patch rv0.7 (work in progress) (obsolete) — — Splinter Review 
This patch has a better performance, this is caching the alias font table in the hash table. If this patch doesn't have new issue, I'll add the codes for Mac/BeOS only for building.
Attachment #241741 - Attachment is obsolete: true
Attached patch Patch rv1.0 (obsolete) — — Splinter Review 
This has BeOS/Mac code. But I cannot test on BeOS.
Attachment #243199 - Attachment is obsolete: true
Comment on attachment 243222 [details] [diff] [review]
Patch rv1.0

O.K. The Japanese testers cannot find any new problems. Let's go to review process.

This patch is adding the |ResolveFontName| method in gfxPlatform. This method gets a spacified font name. This should check whether the font is existing in the system. If the font is existing in the system, it calls the callback function with the *actual* font name.(In many cases, the actual name is a specified name.) But if the specified font is an alias name for multiple fonts, it should call the callback function with *all* actual font names.(The function will be called at the count of actual fonts.)

Windows:

Normally, if the specified font name is existing, it returns by callback function. But if the specified name doesn't find in the font list, it will check by |EnumFontFamiliesExW|.

Linux & BeOS:
In first time, we should cache the system font list for performance. And we should check whether the installed font are changed, by |FcConfigUptoDate|.

If the specified font is existing in the system font list cache, it will return by callback function. Otherwise, it will be checked by fontconfig APIs. If the specified name is alias for single font, the *specified* name will return by callback function. Because if a font name has two names, theses names have each settings(e.g., anti-alias). We should use the specified name for it instead of regular name.

And the last case, it is "alias for multiple fonts" case.

I cannot find the way of getting alias name list in the system. Therefore, I create the new pref that name is "font.alias-list". If the specified font is including the pref, it will be resolved to actual fonts. And they will be returned by callback function.

Note that the cost for resolving the alias is very expensive. Therefore, I made the caching code. It makes great performance for the resolving.

Mac:

I don't write the code for mac in this bug. We should implement it after this bug.



By this patch, the performance of the intl text rendering is very slow down on Linux and BeOS if the specified font is alias font. But the result of rendering is correct and beautiful. We should use this patch, and I'll fix the performance problem in bug 357637. I think that we can remove the pango itemizing and pango shaping if the text doesn't have a complex script.
Attachment #243222 - Attachment description: Patch rv1.0 RC → Patch rv1.0
Attachment #243222 - Flags: review?(pavlov)
Comment on attachment 243222 [details] [diff] [review]
Patch rv1.0

Vlad:

See comment 36 for this patch.
Attachment #243222 - Flags: review?(vladimir)
*** Bug 359067 has been marked as a duplicate of this bug. ***
Comment on attachment 243222 [details] [diff] [review]
Patch rv1.0

a few things...

on windows, we should cache name misses so that we don't   EnumFontFamiliesExW more than once on the same font name if it just doesn't exist.

I don't really follow the font.alias-list stuff in all.js or in the gtk or beos code.

can you repost this without the beos code?  submit it as a seperate patch or something?  keeping with the tier 1 platforms would help make smaller patches and make them easier to review.

I didn't really go through the mac or gtk code very well yet.  Will continue to go through that code
Attachment #243222 - Flags: review?(pavlov) → review-
Attached patch Patch rv1.1 (obsolete) — — Splinter Review 
I'll attach separated patches.
Attachment #243222 - Attachment is obsolete: true
Attachment #244389 - Flags: review?(pavlov)
Attachment #243222 - Flags: review?(vladimir)
(In reply to comment #39)
> I don't really follow the font.alias-list stuff in all.js or in the gtk or beos
> code.

I think that we should use customizable alias font name list. Because on fontconfig, all distributors (and also users) can customize by the .conf files. E.g., Fedora Core has "Sans" that is used for system font (e.g., buttons and menus) , therefore, we need to resolve this alias. If there are distribution dependent alias name, we should also add the names for the list. If we don't use the list, distributors need to create the patch. And the users of the distributions, cannot user unofficial builds, e.g., nightly build of trunk. This is not good things for us.

Of course, if we can get the alias names from fontconfig APIs, we should use the way and we should remove the ugly prefs. But I don't know the way, now. I'll continue to find the way after this bug...
Attached patch Patch rv1.1.1 — — Splinter Review 
Sorry. Fix some nits.
Attachment #244389 - Attachment is obsolete: true
Attachment #244413 - Flags: review?(pavlov)
Attachment #244389 - Flags: review?(pavlov)
other parts were not changed.
Attachment #244391 - Attachment is obsolete: true
Flags: blocking1.9? → blocking1.9+
Whiteboard: [blocking1.9a1+]
Comment on attachment 244413 [details] [diff] [review]
Patch rv1.1.1

And also this patch should be reviewed by vlad.
Attachment #244413 - Flags: review?(vladimir)
Comment on attachment 244413 [details] [diff] [review]
Patch rv1.1.1

i think this is mostly fine.  there are a few minor things i want to change later but they can wait.
Attachment #244413 - Flags: review?(pavlov) → review+
Vlad:

would you also review the patch?
Comment on attachment 244413 [details] [diff] [review]
Patch rv1.1.1

Stuart's review of this should be fine; I didn't see anything that jumped out at me.
Attachment #244413 - Flags: review?(vladimir) → review+
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
This caused about 1.6K of leaks on nye tbox (SeaMonkey tree):
--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
gfxFontList                                      32          -
nsVoidArray                                      16          -
gfxFont                                        1848     175.00%
nsStringBuffer                                 2056      29.80%
nsTArray_base                                   704       2.33%
... and the assertion added at line 558 of gfxPlatformGtk.cpp is also hit during normal startup.  brad would be orange if it built with cairo.

###!!! ASSERTION: What's this case?: 'result == 1'
Attached file Testcase — 
I still have this problem on some sites. See testcase. First font (Arial) i have installed but not second (Arial CE) and there is a problem for me.

Problematic sites:
* http://www.cdr.cz/
* http://aktualne.centrum.cz/
(In reply to comment #56)
> ... and the assertion added at line 558 of gfxPlatformGtk.cpp is also hit
> during normal startup.  brad would be orange if it built with cairo.
> 
> ###!!! ASSERTION: What's this case?: 'result == 1'
> 

(In reply to comment #57)
> Created an attachment (id=246193) [edit]
> Testcase
> 
> I still have this problem on some sites. See testcase. First font (Arial) i
> have installed but not second (Arial CE) and there is a problem for me.
> 
> Problematic sites:
> * http://www.cdr.cz/
> * http://aktualne.centrum.cz/
> 

please file new bugs for the regressions.
Blocks: 361532
Depends on: 361700
No longer blocks: 361532
Depends on: 361532
Depends on: 362682
No longer blocks: 352059
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: