92596 - better font download handler on window

Status: RESOLVED WONTFIX

(Core :: Internationalization, defect, P2)

Description

[Frank Tang]

we should implement better font download handler on window, it should 
1. download all different fonts, not just CJK, we should add
cyrillic, baltic, thai, vietnamese, central european, arabic, hebrew, greek,
trukish, 

2. required now shotdown.

Comment 1

[Frank Tang]

we could implement this by feeding page with meta charset to ie
we can fine ie from 
regestry 
to find the path of Internet explorer
we first look at 

ieclsid  value = "HKEY_CLASSES_ROOT:InternetExplorer.Application:CLSID"
and use that value to find in 
"HKEY_CLASSES_ROOT:CLSID:$ieclsid:LocalServer32"

Comment 2

[Frank Tang]

I think we should implement this code in
mozilla/xpfe/components/winhook
as nsWinFontDownloadHandler

Comment 3

[Frank Tang]

Attachment: patch (include add two new files under mozilla/intl/locale/src/windows

Comment 4

[Frank Tang]

what happen to this patch?

Comment 5

[Frank Tang]

roy can you try this patch in your system ? and also code review it ?

Comment 6

[Roy Yokoyama]

ftang: I have few questions and suggestions
QUESTIONS:
1) what is >#ifdef XP_WIN  ??? Only define in MS Windows XP platform?
2) Need error checking.  Instead of :
   +    nsWinFontPackageHandler* fp = new nsWinFontPackageHandler;
   +    if(fp->Work())
   +    {
   How about:
   +    nsWinFontPackageHandler* fp = new nsWinFontPackageHandler;
   +    if (!fp) 
   +      return NS_ERROR_OUT_OF_MEMORY;
   +    if(fp->Work())
   +    {
3) Sorry, Frank.  I know you came to me and ask about this; but
   please use CreateProcess() API instead of obsolete WinExec().
4) Should we use nsCString instead of nsCAutoString?  nsCAutoString,
   by definition, uses stack based allocation. 
   +class nsWinFontPackageHandler : public nsIFontPackageHandler
   +{
   .........
   +  nsCAutoString mIEPath;
   +  nsCAutoString mIEVersion;
   +};
5) Can we not to use char buffer[4096]?  I rather like to see 
   XPIDLString, nsCString, etc. We may want to ask Scott Collins for his 
opinion.
   +void nsWinFontPackageHandler::InitIEPath()
   +{
   +    ...
   +    char buffer[4096] = {0};
   +    DWORD len;  

SUGGESTIONS:
1) Can we use more generic away?  Instead of Windows platform centric, 
   I believe, we should support other platforms as well.  Mac and Unix
   would like to have this feature. 
2) Instead of rely on IE, we should have our own font files to download. 
   Then we don't need to shutdown Windows and restart. (required by 
   the current GlobalIME implementation.) 

   

Comment 7

[Frank Tang]

move to m94

Comment 8

[Frank Tang]

move to m0.9.5

Comment 9

[msanz]

moving to 0.9.4 nsbranch+ because of dependencies with bugscape 6879. At least
the  JA part of it should be fixed,

Comment 10

[Jaime Rodriguez, Jr.]

Msanz - Should this still be assigned to FTang? Will he be able to fix it in
time for this release? Should we reassign or later this one?

Comment 11

[msanz]

leave it until he is back. There is also another dependency that I just found
with bugscape 5393. I want him to asses the whole group.

Comment 12

[Asa Dotzler [:asa]]

0.9.4 is out the door.

Comment 13

[Jaime Rodriguez, Jr.]

Adding correctness Status Whiteboard, correct/expected behavior does not occur.

Comment 14

[selmer (gone)]

Why would we not ship if this isn't fixed?  We obviously didn't have this in
6.1, so it must be possible to ship...

Comment 15

[Jaime Rodriguez, Jr.]

Agree with Selmer's comments. PDT-

Comment 16

[Frank Tang]

move nsbranch+ to nsbranch-

Comment 17

[Frank Tang]

move to m0.9.6

Comment 18

[Frank Tang]

ok, design spec put in http://www.mozilla.org/projects/intl/fontdow2.html

Comment 19

[Frank Tang]

have problem with CreateProcess() 

It work if I use 
          nsCAutoString command;
          command.Assign(mIEPath);
          command.Append(" -k ");
          command.Append(tmpfile);
          const char* pCmd = command.get();
          UINT ret = WinExec(pCmd, SW_HIDE);

But it does not launch IE if I use the following:


          nsCAutoString command;
          command.Assign(mIEPath);
          command.Append(" -k ");
          command.Append(tmpfile);
          const char* pCmd = command.get();
          STARTUPINFO startupInfo;
          PROCESS_INFORMATION processInfo;
          ::memset(&startupInfo, 0, sizeof(startupInfo));
          startupInfo.cb = sizeof(startupInfo);
          startupInfo.dwFlags = STARTF_USESHOWWINDOW;
          startupInfo.wShowWindow = SW_HIDE;
          WORD ret = ::CreateProcess(mIEPath, (char*) pCmd, NULL, NULL,
                         FALSE, 
                         CREATE_DEFAULT_ERROR_MODE ,
                         NULL, NULL,
                         &startupInfo, 
                         &processInfo);

Roy: What should i do ?

Comment 20

[Frank Tang]

Attachment: v2 of the patch (do not include the calling part yet)

Comment 21

[Frank Tang]

ok, here is the new window font package installer. I will work on that part.

roy- can you review the patch. To see that take effect, apply the patch and also 
change the following temp code in your local tree:

Z:\mozilla\intl\locale>cvs diff -u src
cvs server: Diffing src
Index: src/nsFontPackageService.cpp
===================================================================
RCS file: /cvsroot/mozilla/intl/locale/src/nsFontPackageService.cpp,v
retrieving revision 1.6
diff -u -r1.6 nsFontPackageService.cpp
--- nsFontPackageService.cpp    2001/10/02 21:30:26     1.6
+++ nsFontPackageService.cpp    2001/10/25 22:59:52
@@ -79,7 +79,8 @@
     nsresult rv;

     // create default handler
-    mHandler = do_CreateInstance("@mozilla.org/locale/default-font-package-hand
ler;1", &rv);
+    //mHandler = do_CreateInstance("@mozilla.org/locale/default-font-package-ha
ndler;1", &rv);
+    mHandler = do_CreateInstance("@mozilla.org/locale/win-font-package-handler;
1", &rv);
     if (NS_FAILED(rv)) return rv;
   }
   return mHandler->NeedFontPackage(aFontPackID);


Probably we should put "default-font-package-handler;1" into a pref?

the design paper of this work can be found at
http://www.mozilla.org/projects/intl/fontdow2.html

Comment 22

[Frank Tang]

any comment about the review, roy?

Comment 23

[Roy Yokoyama]

ftang: I have finished applying the patch on my WinXP; but I need to
test little more.   I guess your patch looks reasonable if we decided to
use IE for downloading the font.  (It's against my wish of being Mozilla rely
on IE .....)

Comment 24

[Roy Yokoyama]

ftang: it works fine on my WinXP. As your document stays we need
ExitProcess() to end the process created by CreateProcess()

Comment 25

[Arthur Barrett]

I might be missing something, but doesn't implementing this mean that 
installing IE5 is as pre-requisite to installing Mozilla (if you want semi-auto 
font downloads for international pages)?

Comment 26

[timeless]

Comment on attachment 55143 [details] [diff] [review]
v2 of the patch (do not include the calling part yet)

+  NS_ASSERTION(NS_SUCCEEDED(res), "cannot get IE version");

You can't ASSERT for this.  The absense of IE must at most be a non fatal
warning.

Comment 27

[nhottanscp]

move to 0.9.8

Comment 28

[Frank Tang]

mass move to m9.9

Comment 29

[Frank Tang]

mark this as future for now.

Comment 30

[Frank Tang]

what a hack. I have not touch mozilla code for 2 years. I didn't read these bugs
for 2 years. And they are still there. Just close them as won't fix to clean up.

Comment 31

[Travis Chase]

Mass Re-open of Frank Tangs Won't fix debacle. Spam is his responsibility not my own

Comment 32

[Travis Chase]

Mass Re-assinging Frank Tangs old bugs that he closed won't fix and had to be
re-open. Spam is his fault not my own

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

bug 352049 killed the font download dialog, marking all bugs about it wontfix.