76848 - Regression in handling global fonts

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[rbs]

The has been a regression from the fix that was landed for bug 63965. From the
attachment 28633 [details] [diff] [review] on that bug, LoadGlobalFont() is not behaving as LoadFont():

!         //return LoadFont(aDC, gGlobalFonts[i].name);
!         return LoadGlobalFont(aDC, &(gGlobalFonts[i]));

I will attach screenshots of the above URL to demonstrate the problem.

Comment 1

[rbs]

Attachment: Good

Comment 2

[rbs]

Attachment: Bad

Comment 3

[rbs]

I have verified that the huge glyphs come the 'Lucida Sans Unicode' font, i.e., 
when FONT_HAS_GLYPH() returns true, it is the font currently picked in the
global list. The primary difference that I can see with LoadGlobalFont()
is that it is using 'nsFontWinA' whereas 'nsFontWin' is used elsewhere:

+ nsFontWin*
+ nsFontMetricsWin::LoadGlobalFont(HDC aDC, nsGlobalFont* aGlobalFontItem)
+ {
+   HFONT hfont = ::CreateFontIndirect(&(aGlobalFontItem->logFont));
+ 
+   if (hfont) {
+     if (mLoadedFontsCount == mLoadedFontsAlloc) {
+       int newSize = 2 * (mLoadedFontsAlloc ? mLoadedFontsAlloc : 1);
+       nsFontWinA** newPointer = (nsFontWinA**) PR_Realloc(mLoadedFonts,
+         newSize * sizeof(nsFontWinA*));
+       if (newPointer) {
+         mLoadedFonts = (nsFontWin**) newPointer;
+         mLoadedFontsAlloc = newSize;
+       }
+       else {
+         ::DeleteObject(hfont);
+         return nsnull;
+       }
+     }
...
}

Comment 4

[R.K.Aa.]

different from bug 76826 and the backed out code for bug 46863 ?

Comment 5

[rbs]

*** Bug 76826 has been marked as a duplicate of this bug. ***

Comment 6

[rbs]

Zooming to 10% doesn't change the size of the huge elements, but other elements 
change.

Comment 7

[Erik van der Poel]

nsFontWinA is supposed to be used only with nsFontMetricsWinA, which was created
to work around a bug in Japanese Windows 95.

However, the code Roger mentions above is only dealing with the size of
nsFontWinA*, so it is not really a big deal (since both nsFontWin* and
nsFontWinA* are 4 bytes).

The bug is probably somewhere else.

Comment 8

[rbs]

Right, the code is allocating 'nsFontWinA** newPointer' and then re-casting.
I had overlooked the cast:
+         mLoadedFonts = (nsFontWin**) newPointer;
So as per your comments: there is this bad usage of nsFontWinA + bug elsewhere.

Comment 9

[R.K.Aa.]

*** Bug 76897 has been marked as a duplicate of this bug. ***

Comment 10

[Jacek Piskozub]

Why is the severity "normal". This should be a blocker as it makes the browser
unusable (the first two sites I tried were jus unreadable). From what I've seen
before moving back to yesterday's build, I am sure it breaks some smoketests, as
well.

Comment 11

[Shanjian Li]

I found the problem. Logfont from global font is used directly. However, I forgot 
font size (lfHeight, lfWidth) is also in  LOGFONT structure. I will post a patch
right away.

Comment 12

[Shanjian Li]

Attachment: proposed fix

Comment 13

[Shanjian Li]

This bug might cause serious problem, please help review the bug. 

Erik, Could you give a sr=?

Fix explaination:
Same function "FillLogFont" is used for "LoadGlobalFont". I assigned lfCharset
and other stuff from global later. And this new logfont replace the logfont
from global item.

Comment 14

[rbs]

-> cvs diff -u ?

Comment 15

[Shanjian Li]

Attachment: same patch in diff -u

Comment 16

[rbs]

*** Bug 76665 has been marked as a duplicate of this bug. ***

Comment 17

[rbs]

I have read Shanjian Li's comments on bug 63965, particularly those dated 
"Shanjian Li 2001-03-22 13:46", "Shanjian Li 2001-03-23 13:20", and "Shanjian Li 
2001-04-03 08:42". Now, I understand the motivations of the addition of the
LoadGlobalFont() function, especially as I remember how I was bitten in
bug 46438 by the failures of GetGlyphOutline() due to similar weirdnesses.

To better understand what is going on here, one has to go back and start with
bug 63965. In the previous code, i.e., before the fix for that bug, the global
list of fallback fonts was constructed by asking the GDI to enumerate the list 
of fonts available. However, the information (i.e, logfont) that was returned by 
the GDI during this enumeration wasn't used later on when instantiating the
font. Rather, only the name of the font was added to a temporary logfont,
together with some other default initializations, and the resulting logfont was 
passed to the GDI to instantiate a hfont and retrieve the associated cmap, etc.
As bug 63965 has shown, there were troubles with this approach. And the
investigation carried there showed that the GDI wasn't happy, and also wanted 
some other attributes (e.g., the charset) that it returned during its earlier 
enumeration. So Shanjian Li's fix for that bug has consisted in exactly passing
these attributes within the newly added LoadGlobalFont(). Also, a change was
made in the way the global list is stored: the TrueType fonts were placed at the
head of the list to guarantee that FindGlobalFont() will always try the TrueType
fonts first.

Unfortunately, in the course of this fix, Shanjian Li forgot to explicitly
specify some other varying attributes that are needed to produce the desired
stylistic effects when instantianting a hfont, e.g, the font size (lfHeight, 
lfWidth). So the current proposed fix addresses these ommissions by filling the 
missing attributes.

r=rbs, based on this evaluation and understanding, though I haven't tested the
patch as I am currently behing a restricted firewall.

Comment 18

[rbs]

*** Bug 77037 has been marked as a duplicate of this bug. ***

Comment 19

[Shanjian Li]

Your understanding is absolutely correct. LOGFONT contains both information 
applied to font as a whole and to instantiation of a font. The second category
 of infor (size, style) should be refilled when a font is loaded to system for use.

Comment 20

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 21

[rbs]

Tested the patch and it worked.

Comment 22

[Shanjian Li]

fix checked in.

Comment 23

[Tracy Walker [:tracy]]

verified fixed on windows commercial build 2001-04-24-06

Comment 24

[rbs]

Re-opening. The handling of the font-weight was missed during the fix.

Comment 25

[rbs]

Here is the patch to fix the omission:

Index: nsFontMetricsWin.cpp
===================================================================
RCS file: /cvsroot/mozilla/gfx/src/windows/nsFontMetricsWin.cpp,v
retrieving revision 3.111
diff -u -r3.111 nsFontMetricsWin.cpp
--- nsFontMetricsWin.cpp        2001/06/08 23:36:11     3.111
+++ nsFontMetricsWin.cpp        2001/06/15 07:50:21
@@ -1920,7 +1920,9 @@
   LOGFONT logFont;
   HFONT hfont;

-  FillLogFont(&logFont, aGlobalFontItem->logFont.lfWeight);
+  PRUint16 weightTable = LookForFontWeightTable(aDC, aGlobalFontItem->name);
+  PRInt32 weight = GetFontWeight(mFont->weight, weightTable);
+  FillLogFont(&logFont, weight);
   logFont.lfCharSet = aGlobalFontItem->logFont.lfCharSet;
   logFont.lfPitchAndFamily = aGlobalFontItem->logFont.lfPitchAndFamily;
   strcpy(logFont.lfFaceName, aGlobalFontItem->logFont.lfFaceName);;

Comment 26

[rbs]

Actually, the fix is needed in two places:
nsFontMetricsWin::LoadGlobalFont()
nsFontMetricsWinA::LoadGlobalFont()

Comment 27

[Frank Tang]

rbs- shanjian just had a new baby last week. he will be on vacation and off line 
for two weeks to take care his new baby. He won't read email or bug report 
untill probably end of June. can you take care this one for him? I am willing to 
code review for you. Not 100% clear about your last patch. You mention that need 
to be changed in two place. Can you put a cvs diff -u here ?

Comment 28

[Frank Tang]

change status from "important to mozilla 0.9.2" to "shanjian on vacation, wait 
rbs to submit small fix"

Comment 29

[rbs]

Attachment: fix

Comment 30

[rbs]

What the patch is doing is replacing "aGlobalFontItem->logFont.lfWeight"
(a system default) by the actual font-weight obtained during style resolution.
The bug can be easily reproduced using a markup like:

<span style="font-weight:bold"> &NNNN; </span>

where "NNNN" is the Unicode number of an uncommon character, e.g., &FBAC;

Comment 31

[rbs]

ftang, what is up with this one? It would be nice to avoid the hassle of a late 
approval because that would tie me to keeping a branch tree until the r/sr/a etc 
is complete. This regression happens whenever there is an i18n character that is 
likely to send you to FindGlobalFont() -- once there 'font-weight:bold' wouldn't 
work.

Comment 32

[rbs]

Re-assigning to ftang for checkin - Due to the time-difference, I will not be in 
a suitable position to checkin given tommorrow's deadline for wrapping m0.9.2.

Comment 33

[Frank Tang]

move to moz0.9.3 nsbranch
I think we should got sr and check into trunk. and we should land this to 
moz0.9.2 after mozilla realease it.

Comment 34

[Frank Tang]

r=ftang

Comment 35

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 36

[Frank Tang]

we should land to trunk first. 

Comment 37

[Frank Tang]

ask yokoyama to help land to trunk

Comment 38

[Roy Yokoyama]

checked into trunk.

Comment 39

[lchiang]

adding vtrunk keyword to indicate the bug has been fixed on the trunk.  Bug left
open for tracking checkin to branch (nsbranch) when appropriate.

Once bug has been fixed on the branch also, pls remove vtrunk keyword.

Comment 40

[Frank Tang]

rbs, can you test this on branch?

Comment 41

[Frank Tang]

I have hard time to create a good test case which will show the difference with,
or without the patch. 
So.... I think we should not land this into moz0.9.2 branch for now. 
rbs- can you create a good test case that show the difference? if you can, can
you attach it to the bug ?

Comment 42

[rbs]

Sorry, I have other things in my plate.

Comment 43

[Frank Tang]

rbs- I am thinking moving this out of our m9.2 branch list. Since it is already 
checked into trunk, we can mark it fixed right now. What is the worst case if we 
don't fix this in the branch?

Comment 44

[rbs]

Frank, as I reported earlier, if you have any i18n text, say
<p style="font-weight:bold"> an i18n text </p> [so that some glyphs have to be 
fetched for sure with FindGlobalFont()] then these glyphs wont be bold without 
the fix.

Comment 45

[Frank Tang]

rbs, can you attach your test cases (which produce the gif file you attached )
as attachment? Thanks

Comment 46

[rbs]

It is available from the URL field:
http://lxr.mozilla.org/seamonkey/source/layout/mathml/tests/symbol.html

Comment 47

[Blake Ross]

Not going on the branch. Marking fixed.

Comment 48

[Chris Petersen]

Marking verified per last comments.