69121 - Implement nsIFontPackageHandler to download ja/ko/zh-TW/zh-CN font package

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

[Frank Tang]

roy- we should add this to mozilla
I already implement the nsIFontPackageService and nsIFontPackageProxy code and 
the trigger code in gfx (window only) so when the user hit a ja/ko/zh-TW/zh-CN 
page w/o the font it will call the nsIFontPackageHandler if it is registered. We 
need to implement one in the application layer (not inside Gecko) and call 
xpinstall to download the packages. 
I will put more info about where to find the package here. 

I think we should put the urls into a property files so Netscape can point to a 
differnt location. We probably should also put the human readable string into a 
different string bundle so we can localize it.

Comment 1

[Roy Yokoyama]

Here is how IE5 behaves by visiting a non-supported URL:
- pop up a msgbox saying "would like to download the language pack?"
- after clicking yes, IE downloads it and installs it automatically.

Comment 2

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 3

[Roy Yokoyama]

Upon completion of downloading, we may want to display a dlgbox saying completed
 and the user may set a default font to be the downloaded font in the preferences.

Comment 4

[Roy Yokoyama]

ftang/momoi-san: I am ready for a patch; however,I need a URL which points
to a font downloading site.  Can you provide this?  Thanks

Comment 5

[Roy Yokoyama]

Attachment: Calls DefaultFontPackageHandler ->NeedFontPackage()

Comment 6

[Roy Yokoyama]

Attachment: implementation of nsFontPackageHandler

Comment 7

[Roy Yokoyama]

Attachment: header

Comment 8

[Roy Yokoyama]

Attachment: Define fontdownload URL in properties file

Comment 9

[Katsuhiko Momoi]

Question: As for the downloading URLs, it seems that 
you have decided to have a page for each of the 
GIME modules. Why does each document reside under .../win/en/...
directory? Why "en" in particular?

Comment 10

[Roy Yokoyama]

Can someone review my patch?
>ftang: can you answer momoi-san's question?

Comment 11

[Katsuhiko Momoi]

I guess one reason why you would want "en" under "win" is
that the info page under it is in English and there could
be other lang directroies, e.g. ja, zh-CN, zh-TW, etc.
We can then find a volunteer to translate into these
and other languages. 

Comment 12

[Roy Yokoyama]

Attachment: Take2: property file dif

Comment 13

[Roy Yokoyama]

Attachment: Take2: nsFontPackageService dif

Comment 14

[Roy Yokoyama]

Attachment: Take2: improved nsFontPackageHandler.cpp

Comment 15

[Roy Yokoyama]

Attachment: Take2: improved nsFontPackageHandler.h

Comment 16

[Roy Yokoyama]

New and improved patches to bring up a new browser 
in the event of missing font.
 04/04/01 17:57 Take2: property file dif   (text/plain) 
 04/04/01 17:57 Take2: nsFontPackageService dif   (text/plain) 
 04/04/01 17:58 Take2: improved nsFontPackageHandler.cpp   (text/plain) 
 04/04/01 17:58 Take2: improved nsFontPackageHandler.h   (text/plain) 

mscott: can you please review the patch.

Comment 17

[Scott MacGregor]

Hey Roy, while we used the windows mediator hack to get things working I don't
think it's a solution we can check in. I'm pretty sure the owners of gfx (that's
where the font package handler is right?) aren't going to be happy with adding a
dependency on something in xpfe\appshell. Also, this will never work in the
embedding case because the windows mediator is only built as part of 6.5 not
part of embedding.

So we still need a more permament solution I think. I just saw some email about
some sort of window watcher which is an embedding interface. Danm owns that. It
might be worth talking to him to see if it can do what you need here. 

Also you'll want to get a review from the module owner before you need the super
review from me. Good luck!

Comment 18

[Roy Yokoyama]

Danm:  mscott recalls you may have an embedding interface version of window 
Mediator.  My patch (see below) does some hacky stuff to get the parent
window.  Can you provide me with more info?


NS_IMETHODIMP nsFontPackageListener::GetInterface(const nsIID & aIID, void * 
*aInstancePtr)
{
  nsresult rv = NS_OK;

  NS_ENSURE_ARG_POINTER(aInstancePtr);
  rv = QueryInterface(aIID, aInstancePtr);
  if NS_SUCCEEDED(rv) {
    return rv;
  }
  else {
    // get interface of top most window
    nsCOMPtr<nsIWindowMediator> windowMediator(do_GetService
(kWindowMediatorCID));
    NS_ENSURE_TRUE(windowMediator, NS_ERROR_FAILURE);

    nsCOMPtr<nsIDOMWindowInternal> parentWindow;
    windowMediator->GetMostRecentWindow(nsnull, getter_AddRefs(parentWindow));
    if (!parentWindow) return NS_ERROR_FAILURE;
    rv = parentWindow->QueryInterface(aIID, aInstancePtr);
    return rv;
  }

Comment 19

[Roy Yokoyama]

nsIWindowWatcher::GetActiveWindow() will be available soon.
I'll wait until Dan checks into the tree.

Comment 20

[Roy Yokoyama]

Attachment: Using nsIWindowWatcher.

Comment 21

[Roy Yokoyama]

Attachment: Using nsIWindowWatcher.

Comment 22

[Roy Yokoyama]

By using the nsIWindowWatcher, I could clean up lots of code.
To summarize, here is what I need to check in: 
 04/04/01 17:57 Take2: property file dif   (text/plain) 
 04/04/01 17:57 Take2: nsFontPackageService dif   (text/plain) 
 04/17/01 17:02 Using nsIWindowWatcher.   (text/plain) 
 04/17/01 17:03 Using nsIWindowWatcher.   (text/plain) 

Comment 23

[Roy Yokoyama]

Attachment: Oops, cleaning up the memory leak of nsTextFormatter::smprintf()

Comment 24

[Roy Yokoyama]

Got /r=ftang.

Comment 25

[Brendan Eich [:brendan]]

Use nsAutoString for absUrl, at the least:

  nsString  absUrl;
  rv = createURLString(aFontPackID, absUrl);
  if (NS_FAILED(rv)) return rv;

  nsCOMPtr<nsIWindowWatcher>
windowWatch(do_GetService("@mozilla.org/embedcomp/window-watcher;1"));
  NS_ENSURE_TRUE(windowWatch, NS_ERROR_FAILURE);

  nsCOMPtr<nsIDOMWindow> dialog;
  char* url = absUrl.ToNewCString();
  rv = windowWatch->OpenWindow(nsnull, url, 
              "_blank",
              "chrome,centerscreen,titlebar,resizable", 
              nsnull, 
              getter_AddRefs(dialog));
  nsMemory::Free(url);

But since you then use absUrl only to generate a new C string by chopping chars
down from PRUnichar to char, why use an nsAutoString or nsString?  Can you not
use an nsCAutoString the whole way through the above code?

/be

Comment 26

[Roy Yokoyama]

Attachment: As per brendan's suggestion: Removing nsString and using char absUrl[FONTPACKAGE_URL_SIZE];

Comment 27

[Roy Yokoyama]

Attachment: Change createURL() to accept char *

Comment 28

[Roy Yokoyama]

Note to myself: list of latest patch file list
04/04/01 17:57 Take2: property file dif
04/04/01 17:57 Take2: nsFontPackageService dif   
04/24/01 12:31 As per brendan's suggestion: Removing nsString and using char 
absUrl[FONTPACKAGE_URL_SIZE];
04/24/01 12:32 Change createURL() to accept char * 

Comment 29

[Brendan Eich [:brendan]]

FONTPACKAGE_URL_SIZE is magic, insofar as createURLString knows its aURL param
points to that much space (including space for the terminating \0).  But it gets
worse: the initial strcpy(aURL, &aPackID[5]) assumes aURL has enough room for
the nul-terminated string at aPackID+5, and at that point, we know nothing about
the upper bound on aPackID's length (we know only that it's >5 chars in length).

Without a guarantee that aPackID can't be long enough to overrun the passed-in
buffer, this code is potentially exploitable by security attacks, _a la_ the
Morris worm.  I would at least pass in the buffer size (aURLSize?) and use that
to clamp all copies, ToCStrings, etc. into aURL.

/be

Comment 30

[Brendan Eich [:brendan]]

But why take chances?  How about using an nsXPIDLCString in the caller, and a
proper XPCOM out param (char **) in the callee, who would dynamically allocate a
new C string and store its address on success in *aURL.

/be

Comment 31

[Roy Yokoyama]

Attachment: Using nsXPIDLCString, very cool

Comment 32

[Roy Yokoyama]

Attachment: Using nsXPIDLCString, very cool

Comment 33

[Brendan Eich [:brendan]]

It's kinda pessimal to initialize rv = NS_ERROR_INVALID_ARG -- why not set it to
NS_OK, and do

  if ((aPackID == NULL) || (strlen(aPackID) <= 5))
    return NS_ERROR_INVALID_ARG;

(I fixed the style here to have a space after the if keyword, as elsewhere.) 
But since createURLString is a private method, I wonder why you need the null
check here and for aURL (NS_ENSURE_ARG_POINTER).  At most, NS_ASSERTION should
suffice.  It's not as if this is a public API.

Also, I see no need for bHasStringBundle (eww, Hungarian notation!), when you
can just test bundle and fontDwnldURL (which is leaked in your patch if it gets
set by GetStringFromName -- use an XPIDLString) for non-null values:

  nsCOMPtr<nsIStringBundle> bundle;
  nsXPIDLString fontDwnldURL;

  // get the font downloading URL from properties file
  strings->CreateBundle(FONTPACKAGE_REGIONAL_URL, nsnull,
                        getter_AddRefs(bundle));
  if (bundle) {
    bundle->GetStringFromName(NS_LITERAL_STRING("fontDownloadURL").get(),
                              getter_Copies(fontDwnldURL));
  }

The remaining code would look like this (comments on changes from your patch are
prefixed by ***):

  // construct a font downloading URL
  PRUnichar *urlString = nsnull;

  // aPackID="lang:xxxx", strip "lang:" and get the actual langID
  const char *langID = &aPackID[5];
*** Don't cast away const from aPackID here, there's no need.
*** Also, I put the comment before the declaration, to match style elsewhere,
*** and I got rid of the space after the * and before landID.

  if (fontDwnldURL.get()) {
    urlString = nsTextFormatter::smprintf(fontDwnldURL, langID);
  }
  else {
    NS_ERROR(
    "No item is found. Check chrome://global-region/locale/region.properties");
*** Use NS_ERROR(...) or NS_WARNING(...), not NS_ASSERTION(0, ...).
*** But do you really want to mask errors creating a string bundle or getting
*** a string from it?  Wouldn't it be better to propagate the bundle creation
*** error to createURLString's caller, NeedFontPackage?

    // if no StringBundle, default to
    // "http://www.mozilla.org/projects/intl/fonts/win/en/package_%1$s.html"
    urlString = nsTextFormatter::smprintf(
                    NS_LITERAL_STRING(FONTPACKAGE_COMMAND_DEFAULT_URL).get(),
                    langID);
*** Don't declare nsString local variables: locals have storage class auto, so
*** use nsAutoString.  But here, you don't need to copy the NS_LITERAL_STRING
*** into an nsString or nsAutoString, just to call .GetUnicode().  Given the
*** NS_LITERAL_STRING, you can .get() a pointer to its const Unicode chars.
  }

//if (urlString) {
//  nsAutoString aCharset(urlString);
//  *aURL = aCharset.ToNewCString();
*** Good, nsAutoString here -- but why make two copies (once from urlString into
*** aCharset, then a second one into *aURL)?  Also, aCharset is named as if it's
*** an argument (leading a), but it's a local variable.  And, if you couldn't
*** allocate urlString, you should fail with NS_ERROR_OUT_OF_MEMORY.  Here's my
*** version:

  if (!urlString) {
    rv = NS_ERROR_OUT_OF_MEMORY;
  }
  else {
    *aURL = nsCRT::strdup(urlString);
    rv = NS_OK;	// only if rv wasn't initialized to NS_OK...
  }

  // free urlString
  nsTextFormatter::smprintf_free(urlString);
*** Too bad nsTextFormatter doesn't use the XPCOM shared allocator (nsMemory);
*** if it did, we could just do *aURL = urlString; and not make yet another
*** copy.  Please file a bug against nsTextFormatter?

  return rv;

I hope this is clear.  Bugzilla's textarea is crimping my style!

/be

Comment 34

[Brendan Eich [:brendan]]

Also, once you have cvs add'ed all new files, you can cvs diff -uN to make an
all-in-one patch, instead of attaching individual files.

/be

Comment 35

[Roy Yokoyama]

I'll add the files once the tree is open; so that I can attach a diff
instead of individual files. :)  

Comment 36

[Brendan Eich [:brendan]]

cvs add without a subsequent cvs commit is ok to do while the tree is closed. 
Ditto cvs remove.  This applies to regular files only!  Directories get added
immediately, no commit required; ditto remove (once empty).  Beware.

/be

Comment 37

[Roy Yokoyama]

Attachment: All-in-one diff: as per brendan's suggestion for createURLString(..)

Comment 38

[Roy Yokoyama]

Attachment: Ahhhh, forgot to pass the nsresult back to NeedFontPackage()

Comment 39

[Brendan Eich [:brendan]]

Almost there, a few things to fix (nits, mostly):

1.  Remove deadwood:

+#define FONTPACKAGE_URL_SIZE 256

and later, in the ctor and dtor, some boilerplate C-style comments:

+
+  /* member initializers and constructor code */

and

+  /* destructor code */

2.  Wrong nsresult and no assertion for public API sanity check:

+NS_IMETHODIMP nsFontPackageHandler::NeedFontPackage(const char *aFontPackID)
+{
+  nsresult rv = NS_ERROR_FAILURE;
+
+  // no FontPackage is passed, return
+  if (!aFontPackID)
+    return rv;

How about NS_ERROR_INVALID_POINTER or INVALID_ARG?  Also, assert first that it's
not null, to catch insane callers in debug builds so we can cure them.  If you
like its conciseness and don't mind the hidden return statement, use the
NS_ENSURE_ARG_POINTER macro to do both the assertion and the
if-asserted-condition-is-false-return-error:

  NS_ENSURE_ARG_POINTER(aFondPackID);

and don't initialize rv, let it get set later by a real result code from the
createURLString call (you could move its declaration down till it's needed,
that's usually good style).

3.  Typo in these assertions:

+  NS_ASSERTION(aPackID, "illegal vaule- null ptr- aPackID");
+  NS_ASSERTION(aURL, "illegal vaule- null ptr- aURL");

These still seem vacuous to me, cuz createURLString is private and there's only
one call to it, but they don't hurt.  Just thought you'd want to fix the typos.
BTW, is createURLString uncapitalized to indicate that it's private?  C++ style
for identifiers, public or private, is usually InterCaps (capitalized).

4.  Create string bundle failure should not be suppressed.  Instead of:

+  strings->CreateBundle(FONTPACKAGE_REGIONAL_URL, nsnull,
getter_AddRefs(bundle));
+  if (bundle) {
+    bundle->GetStringFromName(NS_LITERAL_STRING("fontDownloadURL").get(), 
+                              getter_Copies(fontDwnldURL));
+  }

Do this:

+  rv = strings->CreateBundle(FONTPACKAGE_REGIONAL_URL, nsnull,
+                             getter_AddRefs(bundle));
+  if (NS_FAILED(rv))
+    return rv;
+
+  nsXPIDLString fontDwnldURL;
+  bundle->GetStringFromName(NS_LITERAL_STRING("fontDownloadURL").get(), 
+                            getter_Copies(fontDwnldURL));

This also relieves you of having to declare rv early and initialize it to NS_OK;
let it be declared right before its first use, here.  I also show fondDwnldURL
being declared just before its use as an out param to the GetStringFromName
call.  In the same vein, move

+
+  // construct a font downloading URL
+  PRUnichar *urlString = nsnull;

down into the if (fontDwnldURL.get()) {...} then-statement block where it is
used and should be scoped.  Although you might rather cast out the failure to
find a string in the bundle with an early return:

+  if (!fontDwnldURL.get()) {
+    NS_ERROR("No item is found. Check
chrome://global-region/locale/region.properties");
+    return NS_ERROR_FAILURE;
+  }
+
+  // aPackID="lang:xxxx", strip "lang:" and get the actual langID
+  const char *langID = &aPackID[5]; 
+  urlString = nsTextFormatter::smprintf(fontDwnldURL.get(), langID);
+  if (!urlString) {
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  *aURL = ToNewUTF8String(nsLocalString(urlString));
+  nsTextFormatter::smprintf_free(urlString);
+  return NS_OK;

I fixed the overindented NS_ERROR call here, and used early return to cast out
error cases throughout.  If you prefer to use rv and avoid early returns, then
the just push declarations into the narrowest block scope containing all their
uses, and fix that overindented NS_ERROR.

5.  In nsFontPackageService.cpp, you've added else-after-return and an early
return in the else clause, which doesn't make sense, and which leaves the
printfs unreachable:

+
+    // create default handler
+    mHandler = new nsFontPackageHandler;  // assignment to
nsCOMPtr<nsIFontPackageHandler>
+    if (mHandler)  {
+      return mHandler->NeedFontPackage(aFontPackID);
+    }
+    else  {
+      NS_ASSERTION(0, "no default handler for font package.");
+      return NS_ERROR_FAILURE;
+    }
+
 #ifdef _DEBUG
     printf("We need to download the font package for %s\n", aFontPackID);
     printf("But we don't have a nsIFontPackageHandler set up yet.\n");

Plus, you're using NS_ASSERTION(0, ...) again instead of NS_ERROR(...) or (what
I think is better) asserting the condition in the if statement.  And why are you
using NS_ERROR_FAILURE (and an assertion) for what is really
NS_ERROR_OUT_OF_MEMORY, which can happen and should not cause an assertion? 
Assertions should indicate inconsistency in the state of the program that is not
expected.

I'm not sure what the control flow should be here -- maybe those printfs can be
removed altogether?

/be

Comment 40

[Brendan Eich [:brendan]]

Sorry, I keep misspelling fontDwnldURL as fondDwnldURL.  That reminds me, why
not use a non-cybercrud name like fontDownloadURL or just fontURL?

/be

Comment 41

[Roy Yokoyama]

Attachment: Clean ups

Comment 42

[Brendan Eich [:brendan]]

Looks good, one last nit and sr=[e-mail].  Instead of:

+  if (mHandler)
+    return mHandler->NeedFontPackage(aFontPackID);
+
+  // create default handler
+  mHandler = new nsFontPackageHandler;
+  if (!mHandler) 
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  return mHandler->NeedFontPackage(aFontPackID);

Save some few instructions (or optimizer cycles) and (more important) reader
cycles:


+  if (!mHandler) {
+    // create default handler
+    mHandler = new nsFontPackageHandler;
+    if (!mHandler) 
+      return NS_ERROR_OUT_OF_MEMORY;
+  }
+  return mHandler->NeedFontPackage(aFontPackID);

/be

Comment 43

[Roy Yokoyama]

Attachment: check for if (!mHandler) instead of if (mHandler)

Comment 44

[Jaime Rodriguez, Jr.]

Adding nsbeta1 keyword.

Ftang - Please set the priority for this bug.

Comment 45

[Marek Z. Jeziorek]

can this be checked in to 0.9?

Comment 46

[Brendan Eich [:brendan]]

0.9 is done, I think.

/be

Comment 47

[Brendan Eich [:brendan]]

sr=[e-mail].

I thought this was only 0.9.1, not 0.9, or I'd have recorded the sr= sooner (too
busy with 0.9 stuff).

/be

Comment 48

[Roy Yokoyama]

This patch works well with Mojo; but embedding is having little problem with
unparented window.open() [ thus 86522 blocker ].  danm is working on for the fix.

Comment 49

[Frank Tang]

r=ftang

Comment 50

[Marek Z. Jeziorek]

let's not try to check this into 0.9 anymore (just 0.9.1 will be fine). thanks.

Comment 51

[Roy Yokoyama]

Attachment: No change: except to reflect nsIStringBundle::CreateBundle() API change done on April 27.

Comment 52

[Roy Yokoyama]

checkedin

Comment 53

[Roy Yokoyama]

can you mark this as verified if you see the FontDownload dlgbox?  
This is very old bug. I verified the code is still in the trunk .

Comment 54

[Teruko Kobayashi]

Verified in 7-23 branch build.