91250 - Gecko does not response to WM_FONTCHANGE as expected.

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Hansoo Kim]

In a response to WM_FONTCHANGE msg broadcasted from NeedFontPackage() 
in nsIFontPackageHandler in usual embedding application, Gecko does not refresh 
( re-render ) any existing browser windows's content using right font just 
installed if the document has the charset which need the font just installed.

Comment 1

[Teruko Kobayashi]

Changed QA contact to myself.

Comment 2

[Roy Yokoyama]

This is related to 89493.
Teruko, can you change this status to CONFIRMED.  I don't
have the privillage. ;(

Comment 3

[Teruko Kobayashi]

Confirming.

Comment 4

[Roy Yokoyama]

Attachment: Process WM_FONTCHANGE only when the font list is changed.

Comment 5

[Roy Yokoyama]

[e-mail]: can you review (07/31/01 18:25)?

Hansoo: you may want to try this patch (07/31/01 18:25)
           in your local tree to see if it fixes your problem .

Patch explaination:
- remove the hack in nsPresContext.cpp where the page refresh 
   is disabled in case of "font.internaluseonly.changed"
- update device context font cache only when the system font 
   list is truly changed by WM_FONTCHANGE msg. (ie. We don't
   set "font.internaluseonly.changed" in every WM_FONTCHANGE msg)
- define NS_GLOBALFONT_REMOVE for nsGlobalFont.flags to keep
  truck of remved font
- identify if the font list has changed by maintaining the FontCount.

cc'ing [e-mail] 

Comment 6

[Roy Yokoyama]

One observation:  
MSWindows displays a message box saying 
"Can't delete the font. Used by other app" when I attempt to delete a font
while the browser is using the font to render the page.

However, above is not always the case. I CAN delete it most of the time !!!!

Comment 7

[rbs]

If you just want to count the fonts, you could do something simpler and retain
all of the previous code. In UpdateFontList() (i.e., the previous code), before
you destroy the global list, you first store away the current count. Then after
restoring you just compare the new count against the old count.

Comment 8

[Roy Yokoyama]

rbs: you are right.  Originally, I was playing with the new nsGlobalFont.flags;
but I ended up with just counting the
font list.  

Thanks for your suggestion.  It works well.  
I'll post the new patch in a minute.

Comment 9

[Roy Yokoyama]

Attachment: as per rbs's suggestion.

Comment 10

[rbs]

other things that you could do at your checkin:

  if (beforeFontCount != afterFontCount) 
+    *aDidChanged = PR_TRUE;
+  else
+    *aDidChanged = PR_FALSE;
+
===>
*aDidChanged = beforeFontCount != afterFontCount

spelling: s/aDidChanged/aHasChanged/

with that: r=rbs

Comment 11

[Roy Yokoyama]

Attachment: Adding rbs's suggestion TAKE II

Comment 12

[Roy Yokoyama]

waterson: can you /sr=?

This patch is to remove the hack in nsPresContext.cpp for the "font.internaluseonly.changed" (bug 89493)
It may impact the 3d Groove install.

cc'ing waterson, attinasi

Comment 13

[Chris Waterson]

1. Why not just

  boolean UpdateFontList()?

or better yet,

  boolean updateFontList()?

+  void UpdateFontList([retval] out boolean aHasChanged);

That yields the same C++ code, but makes it much easier to use from JS.


2. Um, the comment doesn't quite match the code here...

+  *aHasChanged = PR_FALSE; // always return true for now.

3. Finally, although this is a commendable optimization, won't we _still_ get
the crash if someone really _does_ add a font?

Comment 14

[Peter Lubczynski]

I tried this patch and it does regress bug 89493. Checking this patch in will
cause Shockwave 3D Groove install to fail. I think the root of the problem is
that the OBJECT frame can not currently  withstand a reframe. I have opened bug
90268 on this issue.

Comment 15

[Roy Yokoyama]

Thanks guys.

I am open to any suggestions. 
We need to update the global font list and fresh the pages.  

Can we call similar to nsBrowserWindow::ForceRefresh()
from nsWindow::ProcessMessage()?   Note that we
want to broadcast the refresh to all the instance of browsers.

Comment 16

[Chris Waterson]

Couple thoughts.

1. I'm really quite surprised that the plugin thinks its installing a new font.
Roy, perhaps you could use the test case in bug 89493 to verify that a new font
is really being installed? (Maybe the logic in the font cache is just not
working quite right.)

2. We should look at other apps that issue WM_FONTCHANGE messages (the only one
I know of is eXceed, which is available on NS internal servers somewhere), and
see why it thinks it's adding and removing system fonts every time you open or
close an X window.

3. If eXceed (and any other apps) really _are_ sending bonafide WM_FONTCHANGE
messages (i.e., a new font has become available), then maybe we should just
leave this alone and focus on fixing bug 90268. OTOH, if it turns out the
messages are spurious, then I think we can go ahead with checking this patch in
modulo the nsPresContext.cpp changes.

Make sense?

Comment 17

[Roy Yokoyama]

Chris, here is what I found so far.
> 1)you could use the test case in bug 89493 to verify 
I've verified that the plugin _IS_ installing a new font.
I see the font count to go up by one.  It is installing 
a font called "Times *".  
> 2) why it thinks it's adding and removing system fonts every time you open
orclose an X window.
Simply opening/closing app doesn't hit my break-point on WM_FONTCHANGE.
I used IE to visit www.shockwave.com and as soon as it attempts to 
install the 3D Groove plugin, my break-point on WM_FONTCHANGE
catched the font change and the "Times *" font is installed.

Conclusion:
3D Groove plugin is really installing a new font and thus 
my patch is acting correctly.   

I am not sure whatelse we can do here. Please let me know if 
anyone has any suggestions.





    

Comment 18

[Roy Yokoyama]

Hold on.   What is "Times *" font?  
Does anybody know?  I recall it's a temporary font.  Investigating....

rbs:  meanwhile, can we ignore the font containing '*' just like the vertical
font '@'?

Comment 19

[Peter Lubczynski]

The reframe caused by this patch destroys the plugin instance which I think is
the problem for plugins. Just updating the font cache and having the user
manully hit reload does not cause a problem. The Shockwave plugin does not want,
need, or even care  about a reframe when it installs a font. It just can not be
terminated. 

Like I said before, I think the real solution is to make the OBJECT frame
withstand a reframe. However, that not being a simple task, as a last resort
hack, this patch can be hacked to only skip the reframe in case the Shockwave
plugin is running.

As for EXCEED, I can't seem to get it to work on my W2K, but I found a copy on
this internal Netscape server:
\\SURGE\dist\sysapps\EXCEED

Comment 20

[rbs]

Yep, the problem of the OBJECT frame is understood and need fixing. That's okay.
But if the patch can help to discard frivolous WM_FONTCHANGE, that might be good
too.

Is eXceed installing a '*' font too?

Comment 21

[Roy Yokoyama]

Peter: understood. 
>this patch can be hacked to only skip the reframe 
>in case the Shockwave plugin is running.
One question, how do we do the above? 

Comment 22

[Roy Yokoyama]

rbs: can we do like below?   I am just throwing ideas.....

static int CALLBACK enumProc(const LOGFONT* logFont, const TEXTMETRIC* metrics,
  DWORD fontType, LPARAM closure)
{
  // XXX ignore vertical fonts
  if (logFont->lfFaceName[0] == '@') {
    return 1;
  }

>  // XXX ignore temporary fonts
>  PRUint32 len = strlen(logFont->lfFaceName);
>  if ((len != 0) && (logFont->lfFaceName[len-1] == '*')) {
>    return 1;
>  }
>

Comment 23

[Peter Lubczynski]

Hm....second thought, checking for Shockwave running won't be easy because there
is currently no way to do this globally. Perhaps there is some kind of public
flag or pref that can be set from plugins to prevent the reframe until the
OBJECT frame can be made to withstand it.

On the other hand, I dug up an OLD patch by Waterson in another bug that splits
up nsObjectFrame.cpp and tries to get the reframe to work:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=41015

Comment 24

[Roy Yokoyama]

No action on bug 90268 since 07/10/2001.  

waterson: Does your patch for reframe work? 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=41015

Comment 25

[Chris Waterson]

No, it doesn't. I'd like to get that working though...I'll retarget for 0.9.4.

Comment 26

[Roy Yokoyama]

Attachment: Updating the patch so that I don't loose the changes. (includes waterson's recommendations and search for '*' facename)

Comment 27

[rbs]

waterson, any idea of the font(s) that eXceed triggers at every window open and
every window close? That's pretty intriguing.

Comment 28

[Roy Yokoyama]

ftang and I  came to a possible solution that we don't need to refresh a page 
every time a new font is installed in the system. 
We refresh a page only when the user installs a new font for an empty _language_font_group_.

We know that the font language group is empty when the browser renders a garbage text.
The patch will maintains a language group bit before and after enumerating the font list.
If a new font language group is added or a font language group is removed, then we refresh the page.
I'll post a patch in a minute.

rbs, waterson, peter= let us know what you think.

Comment 29

[Roy Yokoyama]

Attachment: Using HaveFontFor() to detect the langGroup change.

Comment 30

[rbs]

Guys, I think it is getting unnecessarily too complicated. If there isn't a
simple solution, it might be best to fall-back to the ol'd good Reload button.
A single-click, et voila... 

After all, if someone has just gone through the pain of installing a new font,
hitting the Reload button to sit back and enjoy the new font sounds like a
refreshing breeze. 

Other applications even require a re-start to pick up a new font.

Let's look for a very simple and non-invasive solution. If that's isn't
possible, one might be content with the Reload key.

Comment 31

[Frank Tang]

rbs- what is WRONG with the newest patch? 
It is complicate because this IS a complicate problem itself. (some application 
install and uninstall font on the fly) 
Roy try to address this problem so some other users will have better experience. 
Could we focus on the valid and correctness of the fixes instead of it is 
necessary or not.  Thanks.
I believe you and me may know how to click reload, but once we implement a 
better font download installer, the font installation could be very easy and 
click a reload could be too much for normal users. 

roy:
in nsFontEnumeratorWin::UpdateFontList()
I think you should not 
if (NS_FAILED(rv)) return rv; 
in the function. in case it failed, you may want to assert and continue. and you 
should initial *updateFontList in the beginning of the function. 

Comment 32

[rbs]

ftang, your comments prompted me to have another read at the patch (I had 
just cruised over it earlier). Since installing a new font for an i18n langGroup
for which no font exists on the system is unlikely to happen when 3d Groove is
running, the patch might actually fix the problem. (However, I don't buy the
argument that 'click a reload could be too much for normal users', especially
given the fact that they have just been through the process of installing a new
font pack.) 

Here are some suggestions:

+  nsresult rv;
+  PRBool haveFontForLang = FALSE;
+  int charSetCounter = 0;
+  PRUint16 maskBitBefore = 0;
+  // iterate langGoup; skip DEFAULT
+  for (charSetCounter = 1; charSetCounter < eCharSet_COUNT; charSetCounter++) {

add: haveFontForLang = PR_FALSE

+    rv = HaveFontFor(gCharSetInfo[charSetCounter].mLangGroup,
&haveFontForLang);

remove: +    if (NS_FAILED(rv)) return rv;

+    if (haveFontForLang)  {
>>>>+      maskBitBefore |= gCharSetInfo[charSetCounter].mMask;
replace: maskBitBefore |= PR_BIT(charSetCounter);
         i.e, no need to add the 'mMask' member, derive the bit on the fly
         from the index of the charset
+    }
+  }

roy, do you mind also removing these comments (bit fields are common and
comments like these just clutter the code): 
+  // maskBit flags 
+  //  0 0 0 0 0 0 0 0 | 0 0 0 0 0 0 0 0   "DEFAULT"
+  //  - - ^ ^ ^ ^ ^ ^   ^ ^ ^ ^ ^ ^ ^ ^ 
+  //      | | | | | |   | | | | | | | +-- "ANSI"
+  //      | | | | | |   | | | | | | +---- "EASTEUROPE"
+  //      | | | | | |   | | | | | +------ "RUSSIAN"
+  //      | | | | | |   | | | | +-------- "GREEK"
+  //      | | | | | |   | | | +---------- "TURKISH"
+  //      | | | | | |   | | +------------ "HEBREW"
+  //      | | | | | |   | +-------------- "ARABIC"
+  //      | | | | | |   +---------------- "BALTIC"
+  //      | | | | | +-------------------- "THAI"  
+  //      | | | | +---------------------- "SHIFTJIS"
+  //      | | | +------------------------ "GB2312"   
+  //      | | +-------------------------- "HANGEUL"
+  //      | +---------------------------- "CHINESEBIG5"
+  //      +------------------------------ "JOHAB" 

Comment 33

[Brendan Eich [:brendan]]

rbs: one suggested change to your review comment:

+  for (charSetCounter = 1; charSetCounter < eCharSet_COUNT; charSetCounter++) {

add: haveFontForLang = PR_FALSE

+    rv = HaveFontFor(gCharSetInfo[charSetCounter].mLangGroup,
&haveFontForLang);

remove: +    if (NS_FAILED(rv)) return rv;

+    if (haveFontForLang)  {

There's no need to initialize haveFontForLang to PR_FALSE at its declaration,
then set it false again at the top of the loop.  Instead, clear it in this 'if
(haveFontForLang) {...}' then-statement block.

/be

Comment 34

[Roy Yokoyama]

Attachment: Taking ftang's and rbs's suggestions

Comment 35

[rbs]

With brendan tidying:
+  // iterate langGoup; skip DEFAULT
+  for (charSetCounter = 1; charSetCounter < eCharSet_COUNT; charSetCounter++) {
+    haveFontForLang = PR_FALSE;
+    rv = HaveFontFor(gCharSetInfo[charSetCounter].mLangGroup,
&haveFontForLang);
+    if (haveFontForLang)  {
+      maskBitBefore |= PR_BIT(charSetCounter);
+    }
+  }
+  

read (no need to initialize haveFontForLang when declared):

+  // iterate langGoup; skip DEFAULT
   haveFontForLang = PR_FALSE;  <-- where the same code is used twice
+  for (charSetCounter = 1; charSetCounter < eCharSet_COUNT; charSetCounter++) {
+    HaveFontFor(gCharSetInfo[charSetCounter].mLangGroup, &haveFontForLang);
+    if (haveFontForLang)  {
+      maskBitBefore |= PR_BIT(charSetCounter);
+      haveFontForLang = PR_FALSE;
+    }
+  }
+  

(Also noted undesirable tabs hanging in many places which you might want to
discard.) 

Comment 36

[Roy Yokoyama]

Attachment: Brendan and rbs's suggestion and removing tabs

Comment 37

[Roy Yokoyama]

rbs: can you /r=?
brendan: can you /sr= after rbs? 
Thanks

Comment 38

[Peter Lubczynski]

very nice, latest patch does not disturb bug 89493. Is the same true for eXceed?

Comment 39

[Brendan Eich [:brendan]]

Looks ok to me, r=[e-mail] and I delegate my sr= to rbs to give for
this patch.

/be

Comment 40

[rbs]

last nit: no need to attach the change. remove 'rv' since it is write-only in 
the patch (nobody uses it).
sr=rbs  [on behalf on brendan]

Comment 41

[Roy Yokoyama]

checked-in

Comment 42

[Roy Yokoyama]

topembed: checked-into MOZILLA_0_9_2_BRANCH tree.

Comment 43

[Teruko Kobayashi]

I can verify this in 8-21-11 trunk Win32 build, but I do not see fix in 
8-20-22-0.9.2ec build.  I reopen this.

Comment 44

[Teruko Kobayashi]

Installer may have some problem.  It did not install the right files.
After I installed it by N6SetupB.exe, I got the right files.
I am testing this, so I changed this as fixed.

Comment 45

[Teruko Kobayashi]

Verified as fixed in 8-20-22-0.9.2ec and 8-21-22-0.9.2ec Win32 build.

Comment 46

[Mike Connor [:mconnor]]

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