Closed
Bug 91250
Opened 23 years ago
Closed 23 years ago
Gecko does not response to WM_FONTCHANGE as expected.
Categories
(Core :: Internationalization, defect)
Tracking
()
VERIFIED
FIXED
mozilla0.9.4
People
(Reporter: hansoodev, Assigned: tetsuroy)
References
Details
(Keywords: intl, topembed, Whiteboard: /r=rbs; /sr=brendan; checked-in)
Attachments
(7 files)
14.03 KB,
patch
|
Details | Diff | Splinter Review | |
8.47 KB,
patch
|
Details | Diff | Splinter Review | |
8.41 KB,
patch
|
Details | Diff | Splinter Review | |
9.29 KB,
patch
|
Details | Diff | Splinter Review | |
12.63 KB,
patch
|
Details | Diff | Splinter Review | |
9.50 KB,
patch
|
Details | Diff | Splinter Review | |
9.52 KB,
patch
|
Details | Diff | Splinter Review |
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•23 years ago
|
QA Contact: andreasb → teruko
Comment 1•23 years ago
|
||
Changed QA contact to myself.
Assignee | ||
Comment 2•23 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
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 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•23 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 !!!!
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•23 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•23 years ago
|
||
Comment 10•23 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•23 years ago
|
||
Assignee | ||
Comment 12•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 years ago
|
||
No, it doesn't. I'd like to get that working though...I'll retarget for 0.9.4.
Assignee | ||
Comment 26•23 years ago
|
||
Comment 27•23 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•23 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•23 years ago
|
||
Comment 30•23 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•23 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•23 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"
Comment 33•23 years ago
|
||
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•23 years ago
|
||
Comment 35•23 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•23 years ago
|
||
Assignee | ||
Comment 37•23 years ago
|
||
rbs: can you /r=? brendan: can you /sr= after rbs? Thanks
Comment 38•23 years ago
|
||
very nice, latest patch does not disturb bug 89493. Is the same true for eXceed?
Comment 39•23 years ago
|
||
Looks ok to me, r=brendan@mozilla.org and I delegate my sr= to rbs to give for this patch. /be
Comment 40•23 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]
Assignee | ||
Comment 41•23 years ago
|
||
checked-in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: /r=rbs; need /sr= → /r=rbs; /sr=brendan; checked-in
Assignee | ||
Comment 42•23 years ago
|
||
topembed: checked-into MOZILLA_0_9_2_BRANCH tree.
Comment 43•23 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•23 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
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 45•23 years ago
|
||
Verified as fixed in 8-20-22-0.9.2ec and 8-21-22-0.9.2ec Win32 build.
Status: RESOLVED → VERIFIED
Comment 46•21 years ago
|
||
*** 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.
Description
•