Closed Bug 69121 Opened 20 years ago Closed 19 years ago

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

Categories

(Core :: Internationalization, defect, P1)

x86
Windows 98
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.1

People

(Reporter: ftang, Assigned: tetsuroy)

References

Details

(Keywords: intl, Whiteboard: needed by 05/29/01)

Attachments

(20 files)

2.50 KB, patch
Details | Diff | Splinter Review
6.90 KB, patch
Details | Diff | Splinter Review
2.95 KB, patch
Details | Diff | Splinter Review
534 bytes, patch
Details | Diff | Splinter Review
604 bytes, patch
Details | Diff | Splinter Review
2.42 KB, patch
Details | Diff | Splinter Review
7.73 KB, patch
Details | Diff | Splinter Review
2.79 KB, patch
Details | Diff | Splinter Review
4.29 KB, patch
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
4.39 KB, patch
Details | Diff | Splinter Review
4.47 KB, patch
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
4.44 KB, patch
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
8.44 KB, patch
Details | Diff | Splinter Review
8.43 KB, patch
Details | Diff | Splinter Review
8.52 KB, patch
Details | Diff | Splinter Review
8.48 KB, patch
Details | Diff | Splinter Review
8.43 KB, patch
Details | Diff | Splinter Review
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.
Status: NEW → ASSIGNED
Summary: Implement nsIFontPackageHandler to download ja/ko/zh-TW/zh-CN font package → Implement nsIFontPackageHandler to download ja/ko/zh-TW/zh-CN font package
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.
Target Milestone: --- → mozilla0.9
Changed QA contact to yokoyama@netscape.com.
QA Contact: teruko → 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.
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
Attached patch headerSplinter Review
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?
Can someone review my patch?
>ftang: can you answer momoi-san's question?
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. 
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.
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!
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;
  }
nsIWindowWatcher::GetActiveWindow() will be available soon.
I'll wait until Dan checks into the tree.
Depends on: 76011
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) 

Got /r=ftang.
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
Target Milestone: mozilla0.9 → mozilla0.9.1
Depends on: 76522
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 * 
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
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
Keywords: intl
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
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
I'll add the files once the tree is open; so that I can attach a diff
instead of individual files. :)  
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
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
Sorry, I keep misspelling fontDwnldURL as fondDwnldURL.  That reminds me, why
not use a non-cybercrud name like fontDownloadURL or just fontURL?

/be
Attached patch Clean upsSplinter Review
Looks good, one last nit and sr=brendan@mozilla.org.  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
Adding nsbeta1 keyword.

Ftang - Please set the priority for this bug.
Keywords: nsbeta1
Blocks: 75664
can this be checked in to 0.9?
Whiteboard: needed by 05/29/01
0.9 is done, I think.

/be
sr=brendan@mozilla.org.

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
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.
Priority: -- → P1
r=ftang
let's not try to check this into 0.9 anymore (just 0.9.1 will be fine). thanks.
checkedin
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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 .
QA Contact: yokoyama → teruko
Verified in 7-23 branch build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.