When new font install into the system, mozilla should update's it's cached font list and use it

VERIFIED FIXED in mozilla0.9.4

Status

()

Core
Layout
P1
normal
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Frank Tang, Assigned: Roy Yokoyama)

Tracking

({intl})

Trunk
mozilla0.9.4
x86
Windows 98
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: checked-in)

Attachments

(7 attachments)

(Reporter)

Description

17 years ago
Reproduce procedure
1. launch mozilla
2. open preference:font
3. look at the font list
4. install a new font into the system
5. do 2 and 3
expect result- the newly installed font display in the font list and we can use 
it inside mozilla
actual result- it won't show up.

Roy mention there are a window's message WM_FONTCHANGE will be send to the window 
while the font installer install it, we should somehow pass that informtion to 
GFX and update the font cache.

we need this so we can finish the "download font package on daemond" work.

Roy- can you add this?
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 1

17 years ago
the code which receive window's message is at widget/src/windows/nsWindow.cpp
(Assignee)

Updated

17 years ago
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 2

17 years ago
Roy's note:

Windows will send application WM_FONTCHANGES msg to notify the
font resource has been changed.  Upon receiving the msg, we 
need to update the global fontlist and nsPresContext.

Prefer using the XPCOM interface since the FontMetric is in gkgfxwin.dll and 
the widget is in gkwidget.dll.  Here are the things need to do:
- > add an interface method to nsFontEnumeratorWin service (say ::UpdateFontlist
()) to 
  update the global fontlist.  Use the nsGlobalFont.skip bit to identify 
  the removed font(s) from the system.  
  > use the similar mechanism found in nsFontMetricsWin.cpp/enumProc() to 
append the 
  new font info into the global fontlist.
- > update nsPresContext's device context font cache by using its existing 
  RegisterCallback mechanism (nsPresContext::PreferenceChanged()) which is 
triggered 
  by changing the "font.xxxx"  in Preference <nsIPref>.
  (see layout/base/src/nsPresContext.cpp#440)  

(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Comment 3

17 years ago
Focus on bug fix
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.1 → Future
(Assignee)

Comment 4

17 years ago
More info on this:
- we may or may not want to define a new METHOD 
  nsIFontEnumerator::UpdateFontList() to update the global font list.
  This METHOD may be valid only for Windows.  Other OSs may not want
  to iterate all the fonts; but only what's added/removed from the
  system.  WM_FONTCHANGE msy only indicates "SOMETHING HAPPENED"
  to the system font.

- instead of using nsGlobalFont.skip bit, it may be better 
  if we define a new bit to identify the added/removed font(s) 
  from the system.  Then check for the CMAP of new font.  
  Set the skip bit if the font has the same CMAP info or 
  removed from the system.

Options for updating the device context font cache:
- Use nsIPref's Callback mechanism to globally notify the font change
  and calls the FlushFontCache() for every device context.
- If DOMWindows contains any means of DC, then we can iterate each
  DOMWindows and call to update the font cache.
- There may be a list of DC registered in global context.
  Get the list of DCs and call to update the font cache.
- others..... 

Updated

17 years ago
Keywords: intl
(Assignee)

Comment 5

17 years ago
Created attachment 32782 [details] [diff] [review]
First cut: in-complete; but renders the idea

Comment 6

17 years ago
Due to interest from the very important embedding customer I'm setting --- or
Future target milestone to 0.9.1
Target Milestone: Future → mozilla0.9.1

Comment 7

17 years ago
Changed QA contact to teruko@netscape.com.
QA Contact: petersen → teruko

Comment 8

17 years ago
   PRUint8       skip;
   FONTSIGNATURE signature;
   int           fonttype;
+  PRBool        remove;

Rather than adding yet another bolean attribute, what about just combining
'skip' and 'remove' in a bitfield, say 'flags'. Hence, other bits can be easily
added as need arises. For example, I had hoped to set another 'truetype' bit to 
remember if a listed font is true type.

Updated

17 years ago
Blocks: 75664

Updated

17 years ago
Whiteboard: needed by 05/29/01
(Assignee)

Updated

17 years ago
Priority: -- → P1
(Assignee)

Comment 9

17 years ago
Created attachment 34393 [details] [diff] [review]
change for widget: WM_FONTCHANGE:
(Assignee)

Comment 10

17 years ago
Created attachment 34394 [details] [diff] [review]
Implement nsFontEnumerator::UpdateFontList(); and taking flags suggestion for nsFontEnumeratorFontListWin::

Comment 11

17 years ago
Need some synchronization: as part of the fix for bug 77265 (see the patch 
there), I added the flags data field as well.
(Assignee)

Comment 12

17 years ago
rbs: I agree.
I am not sure if putting 77265 to be a block for this bug is a good idea,
but I did just to keep track of things.
I also put 80756 as a blocker. shanjian owns it.
I definitely need his fix.

Can anyone /r= ?


Depends on: 77265, 80756

Comment 13

17 years ago
I would suggest to replace all instance of :
  if ((gGlobalFonts[i].flags & ID_GLOBALFONT_SKIP) != ID_GLOBALFONT_SKIP)
with
  if (!(gGlobalFonts[i].flags & ID_GLOBALFONT_SKIP))
That will be more concise and faster for CPU.
r=shanjian. 
(Assignee)

Comment 14

17 years ago
brendan: can you /sr=? 
(Assignee)

Comment 15

17 years ago
making 74899 to be a blocker of this.
Depends on: 74899
(Assignee)

Updated

17 years ago
Whiteboard: needed by 05/29/01 → needed by 05/29/01. Waiting for /sr=
The patch at http://bugzilla.mozilla.org/showattachment.cgi?attach_id=34394
(last patch in this bug) seems to be out of date.  Roy, can you cvs update,
merge any conflicts, and attach a new one?  Thanks,

/be

Comment 17

17 years ago
TM to 0.9.2 per PDT triage (it's OK to check it in by Friday or after 0.9.1
branch is made).
Target Milestone: mozilla0.9.1 → mozilla0.9.2
(Assignee)

Comment 18

17 years ago
Created attachment 34883 [details] [diff] [review]
All-in-one; update patch with shanjian's recommendation.
(Assignee)

Comment 19

17 years ago
brendan: can you /sr=? 

Comment 20

17 years ago
There is missing sort (as per bug 77265)
+NS_IMETHODIMP 
+nsFontEnumeratorWin::UpdateFontList()
+{
...
+  // set SKIP bit on for REMOVE fonts
+  SetGlobalFontSkipBit();
+  

>>>Call QuickSort here
   return NS_OK;
 }

Also, you might want to update the sorting criteria so that removed 
fonts are placed last. The code for this is below (since at the
beginning all fonts are available, i.e., the flag isn't set, the
existing behavior will be retained).

static int
CompareGlobalFonts(const void* aArg1, const void* aArg2, void* aClosure)
{
  const nsGlobalFont* font1 = (const nsGlobalFont*)aArg1;
  const nsGlobalFont* font2 = (const nsGlobalFont*)aArg2;

  // Sorting criteria is like a tree:
  // + Available fonts
  //    + TrueType fonts
  //       + non-Symbol fonts
  //       + Symbol fonts
  //    + non-TrueType fonts
  //       + non-Symbol fonts
  //       + Symbol fonts
  // + Removed fonts
  PRInt32 weight1 = 0, weight2 = 0; // computed as node mask 
  if (font1->flags & NS_GLOBALFONT_REMOVE)
    weight1 |= 0x4;
  if (font2->flags & NS_GLOBALFONT_REMOVE)
    weight2 |= 0x4;
  if (!(font1->flags & NS_GLOBALFONT_TRUETYPE))
    weight1 |= 0x2;
  if (!(font2->flags & NS_GLOBALFONT_TRUETYPE))
    weight2 |= 0x2; 
  if (font1->flags & NS_GLOBALFONT_SYMBOL)
    weight1 |= 0x1;
  if (font2->flags & NS_GLOBALFONT_SYMBOL)
    weight2 |= 0x1;

  return weight1 - weight2;
}

==============
On a different approach, I wonder what about just deleting the global
list and calling InitializeGlobalFonts() again. Seems like that will do the
job too (on Win32 at list). Is there any problems with that?
(Reporter)

Updated

17 years ago
Whiteboard: needed by 05/29/01. Waiting for /sr= → go back to implementation after review
(Assignee)

Comment 21

17 years ago
Created attachment 36224 [details] [diff] [review]
Delete and recreate gGlobalFonts
(Assignee)

Comment 22

17 years ago
rbs: can you review for me? thanks

Comment 23

17 years ago
You also need to delete the gGlobalFonts[i].name [like in FreeGlobals()]: 

193   if (nsFontMetricsWin::gGlobalFonts) {
194     //while all cmap is freed, gGlobalFonts's pointer should be freed
195     for (int i = 0; i < nsFontMetricsWin::gGlobalFontsCount; i++) {
196       delete nsFontMetricsWin::gGlobalFonts[i].name;
197     }
198     PR_Free(nsFontMetricsWin::gGlobalFonts);
199     nsFontMetricsWin::gGlobalFonts = nsnull;
200     gGlobalFontsAlloc = 0;
201     nsFontMetricsWin::gGlobalFontsCount = 0;
202   }

While at it, the comment in line 194 is a bit misleading. You should remove it 
too. The 'name' that is used for the gFontMaps is not the same 'name' in 
gGlobalFonts[i].name. The one in gFontMaps comes from GetNAME() and is allocated 
_inside_ GetCMAP(). (If you try printing it for example, you will see that
it is a very long string. It is an internal name that GDI uses to uniquely 
distinguish the font, and so it is a good one to use as the hashkey in 
gFontMaps.)

Let me know how it goes after your testing.
(Assignee)

Comment 24

17 years ago
Created attachment 36368 [details] [diff] [review]
adding delete nsFontMetricsWin::gGlobalFonts[i].name;

Comment 25

17 years ago
Looks clean and tidy with no more leaks. r=rbs
(Assignee)

Comment 26

17 years ago
brendan: can you /sr=? thanks
Whiteboard: go back to implementation after review → got /r=rbs; waiting for /sr=
(Reporter)

Comment 27

17 years ago
I think this is very critical for font download feature. Must have for moz0.9.2
+    for (int i = 0; i < nsFontMetricsWin::gGlobalFontsCount; i++) {
+      delete nsFontMetricsWin::gGlobalFonts[i].name;
+    }

This code is being duplicated.  Why don't you just use FreeGlobals in the file
and reinitialize from there?

+  return ERROR_CALL_NOT_IMPLEMENTED;

Shouldn't you be using NS_ERROR*?

+              PRBool fontInternalChange = PR_FALSE;  
+              pPrefs->GetBoolPref("font.internaluseonly.changed",
&fontInternalChange);
+              pPrefs->SetBoolPref("font.internaluseonly.changed",
!fontInternalChange);

you refer to this flushing the cache but I don't see the support code anywhere
in the tree to monitor that change.
(Assignee)

Comment 29

17 years ago
>Why don't you just use FreeGlobals in the file
>and reinitialize from there?
We don't want to destroy all the hashtables and release other
data.  We should only re-create the global font list.
Thus InitializeGlobalFonts(dc)) call; not InitGlobals(void).

>+  return ERROR_CALL_NOT_IMPLEMENTED;
>Shouldn't you be using NS_ERROR*?
I believe NS_ERROR_NOT_IMPLEMENTED is an appropriate "ERROR CODE" 
since other platforms may want to support in the future.  

>you refer to this flushing the cache but I don't see the support code anywhere
>in the tree to monitor that change.
nsPrefContext::Init() registers the font callback by using 
mPrefs->RegisterCallback("font.", PrefChangedCallback, (void*)this);
which calls mDeviceContext->FlushFontCache(); in the end.

(Assignee)

Comment 30

17 years ago
Sorry, I didn't realize I was using ERROR_CALL_NOT_IMPLEMENTED.
It should read NS_ERROR_NOT_IMPLEMENTED instead of ERROR_CALL_NOT_IMPLEMENTED.
I'll post a patch in a moment.
(Assignee)

Comment 31

17 years ago
Created attachment 37303 [details] [diff] [review]
Creating new patch to return correct ERROR CODE
(Assignee)

Comment 32

17 years ago
blizzard: can you super review? thanks
sr=blizzard
(Assignee)

Updated

17 years ago
Whiteboard: got /r=rbs; waiting for /sr= → got /r=rbs; /sr=blizzard; waiting for /a=

Updated

17 years ago
Blocks: 83989

Comment 34

17 years ago
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)
(Assignee)

Updated

17 years ago
Whiteboard: got /r=rbs; /sr=blizzard; waiting for /a= → Waiting for tree opening
(Reporter)

Updated

17 years ago
Whiteboard: Waiting for tree opening → sr=blizzard, a=asa. Waiting for tree opening
(Assignee)

Comment 35

17 years ago
checked-in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: sr=blizzard, a=asa. Waiting for tree opening → checked-in

Comment 36

17 years ago
Changed QA contact to ylong@netscape.com.
QA Contact: teruko → ylong
(Assignee)

Comment 37

17 years ago
ylong: Can you verify this bugs against today's trunk build?
We had a patch checked-in for 89493 last night which may affect this feature.

Here is what you need to do:
- Pick a machine without a CJK font (say NT-En without Ja-GlobalIME)
- launch browser
- visit Ja site (say www.netscape.com/ja)
- Ja font download dlgbox should come up.  You also see that the Ja page
   is shown ? marks and un-readable text. This is ok because you don't have the ja font just yet.
- After finishing the font download, the Ja page should AUTOMATICALLY update and display
  Ja text correctly.  You shouldn't need to press "Reload" nor shut down the browser.

If it doesn't update/display correctly AUTOMATICALLY, then this feature is broken.
Thanks

Comment 38

17 years ago
I checked it on 07-11-07 trunk build / WinNT-En without CJK font:

1. If I go http://home.netscape.com/ja or http://home.netscape.com/zh/tw
will pops-up a dialog for downloading Japanese or Chinese, which are correctly.  
However, if I go: http://home.netscape.com/ko or http://home.netscape.com/zh/cn
will pops-up 2 downloading dialogs - Korean and Japanese or Simp. Chinese and 
Japanese which are a little strenge. 

2. > - After finishing the font download, the Ja page should AUTOMATICALLY 
update and display Ja text correctly.  You shouldn't need to press "Reload" nor 
shut down the browser.
-- I checked it on http://www.netscape.com/ko, after I finish downloading, the 
Kerean characters apprear automatically but has characters overwrrapping.  If I 
reload the page, then will display fine.
(Assignee)

Comment 39

17 years ago
ReOpening. This is no longer working due to 89493 (see comment by Marc Attinasi 2001-07-09)
I used 20010724 trunk dbg build.
Setting the milestone to 0.9.4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla0.9.2 → mozilla0.9.4
(Assignee)

Comment 40

17 years ago
91250 is checked in.  
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 41

16 years ago
Fixed verified on 09-26 branch build / WinNT-EN.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.