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

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Frank Tang]

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?

Comment 1

[Frank Tang]

the code which receive window's message is at widget/src/windows/nsWindow.cpp

Comment 2

[Roy Yokoyama]

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)  

Comment 3

[Roy Yokoyama]

Focus on bug fix

Comment 4

[Roy Yokoyama]

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..... 

Comment 5

[Roy Yokoyama]

Attachment: First cut: in-complete; but renders the idea

Comment 6

[Marek Z. Jeziorek]

Due to interest from the very important embedding customer I'm setting --- or
Future target milestone to 0.9.1

Comment 7

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 8

[rbs]

   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.

Comment 9

[Roy Yokoyama]

Attachment: change for widget: WM_FONTCHANGE:

Comment 10

[Roy Yokoyama]

Attachment: Implement nsFontEnumerator::UpdateFontList(); and taking flags suggestion for nsFontEnumeratorFontListWin::

Comment 11

[rbs]

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

Comment 12

[Roy Yokoyama]

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= ?

Comment 13

[Shanjian Li]

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. 

Comment 14

[Roy Yokoyama]

brendan: can you /sr=? 

Comment 15

[Roy Yokoyama]

making 74899 to be a blocker of this.

Comment 16

[Brendan Eich [:brendan]]

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

[Marek Z. Jeziorek]

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).

Comment 18

[Roy Yokoyama]

Attachment: All-in-one; update patch with shanjian's recommendation.

Comment 19

[Roy Yokoyama]

brendan: can you /sr=? 

Comment 20

[rbs]

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?

Comment 21

[Roy Yokoyama]

Attachment: Delete and recreate gGlobalFonts

Comment 22

[Roy Yokoyama]

rbs: can you review for me? thanks

Comment 23

[rbs]

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.

Comment 24

[Roy Yokoyama]

Attachment: adding delete nsFontMetricsWin::gGlobalFonts[i].name;

Comment 25

[rbs]

Looks clean and tidy with no more leaks. r=rbs

Comment 26

[Roy Yokoyama]

brendan: can you /sr=? thanks

Comment 27

[Frank Tang]

I think this is very critical for font download feature. Must have for moz0.9.2

Comment 28

[Christopher Blizzard (:blizzard)]

+    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.

Comment 29

[Roy Yokoyama]

>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.

Comment 30

[Roy Yokoyama]

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.

Comment 31

[Roy Yokoyama]

Attachment: Creating new patch to return correct ERROR CODE

Comment 32

[Roy Yokoyama]

blizzard: can you super review? thanks

Comment 33

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 34

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 35

[Roy Yokoyama]

checked-in

Comment 36

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 37

[Roy Yokoyama]

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

[Yuying Long]

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.

Comment 39

[Roy Yokoyama]

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

Comment 40

[Roy Yokoyama]

91250 is checked in.  

Comment 41

[Yuying Long]

Fixed verified on 09-26 branch build / WinNT-EN.