Gecko does not response to WM_FONTCHANGE as expected.

VERIFIED FIXED in mozilla0.9.4

Status

()

Core
Internationalization
VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: Hansoo Kim, Assigned: Roy Yokoyama)

Tracking

({intl, topembed})

Trunk
mozilla0.9.4
x86
Other
intl, topembed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: /r=rbs; /sr=brendan; checked-in)

Attachments

(7 attachments)

(Reporter)

Description

17 years ago
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.

Updated

17 years ago
QA Contact: andreasb → teruko

Comment 1

17 years ago
Changed QA contact to myself.
(Assignee)

Comment 2

17 years ago
This is related to 89493.
Teruko, can you change this status to CONFIRMED.  I don't
have the privillage. ;(
Target Milestone: --- → mozilla0.9.4

Comment 3

17 years ago
Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

17 years ago
Created attachment 44186 [details] [diff] [review]
Process WM_FONTCHANGE only when the font list is changed.

Updated

17 years ago
Keywords: intl
(Assignee)

Comment 5

17 years ago
rbs@maths.uq.edu.au: 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 rbs@maths.uq.edu.au 
(Assignee)

Comment 6

17 years ago
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

17 years ago
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.
(Assignee)

Comment 8

17 years ago
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.

(Assignee)

Comment 9

17 years ago
Created attachment 44255 [details] [diff] [review]
as per rbs's suggestion.

Comment 10

17 years ago
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


(Assignee)

Comment 11

17 years ago
Created attachment 44287 [details] [diff] [review]
Adding rbs's suggestion TAKE II
(Assignee)

Comment 12

17 years ago
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
Whiteboard: /r=rbs; need /sr=

Comment 13

17 years ago
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

17 years ago
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.
(Assignee)

Comment 15

17 years ago
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

17 years ago
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?
(Assignee)

Comment 17

17 years ago
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.





    
(Assignee)

Comment 18

17 years ago
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

17 years ago
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

17 years ago
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?
(Assignee)

Comment 21

17 years ago
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? 
(Assignee)

Comment 22

17 years ago
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

17 years ago
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
(Assignee)

Comment 24

17 years ago
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


Depends on: 90250

Comment 25

17 years ago
No, it doesn't. I'd like to get that working though...I'll retarget for 0.9.4.
(Assignee)

Comment 26

17 years ago
Created attachment 44615 [details] [diff] [review]
Updating the patch so that I don't loose the changes. (includes waterson's recommendations and search for '*' facename)

Comment 27

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

Comment 28

16 years ago
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.
(Assignee)

Comment 29

16 years ago
Created attachment 44856 [details] [diff] [review]
Using HaveFontFor() to detect the langGroup change.

Comment 30

16 years ago
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

16 years ago
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

16 years ago
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" 
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
(Assignee)

Comment 34

16 years ago
Created attachment 44997 [details] [diff] [review]
Taking ftang's and rbs's suggestions

Comment 35

16 years ago
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.) 
(Assignee)

Comment 36

16 years ago
Created attachment 45077 [details] [diff] [review]
Brendan and rbs's suggestion and removing tabs
(Assignee)

Comment 37

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

Comment 38

16 years ago
very nice, latest patch does not disturb bug 89493. Is the same true for eXceed?
Looks ok to me, r=brendan@mozilla.org and I delegate my sr= to rbs to give for
this patch.

/be

Comment 40

16 years ago
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]

Updated

16 years ago
Keywords: topembed
(Assignee)

Comment 41

16 years ago
checked-in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: /r=rbs; need /sr= → /r=rbs; /sr=brendan; checked-in
(Assignee)

Comment 42

16 years ago
topembed: checked-into MOZILLA_0_9_2_BRANCH tree.

Comment 43

16 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 44

16 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 45

16 years ago
Verified as fixed in 8-20-22-0.9.2ec and 8-21-22-0.9.2ec Win32 build.
Status: RESOLVED → VERIFIED
*** Bug 55194 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.