119927 - NeedFontPackage() should be called multiple times in a session

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P2)

Description

[Hansoo Kim]

In embedding cases, if the customized handler registered by embedding app failed
during the download and installation of font package, there is no way to 
attempt the second try b/c of no more notification.

For example, server timeout for downloading...
The user of embedding app would not know, unless told by some kind of msgbox,
s/he needs to restart the session.

Desired behavior is Gecko notifies embedding app any time it does not detect 
necessary font in the system which is needed for page rendering,
and let embedding app decide whether it's going to honor the notification or 
not.

Comment 1

[Hansoo Kim]

*****   Additional Comment #1 From Hansoo Kim 2001-11-16 12:49 -------

Roy, can you bring this new desired behavior to the team for a discussion ?
Thanks

Comment 2

[Hansoo Kim]

***** Additional Comment #2 From Roy Yokoyama 2001-11-19 14:53 -------

I'll post a patch in a minute

Comment 3

[Hansoo Kim]

***** Additional Comment #3 From Roy Yokoyama 2001-11-19 16:21 -------

On the other hand, my initial estimate was wrong.
This could potentially be a major change in the code.

There are number of global variables to keep track of initiated font download.  
http://lxr.mozilla.org/seamonkey/source/gfx/src/windows/nsFontMetricsWin.cpp#365
If we remove the check, then the embedding app will have 
number of notification msgs within the "same" page. 
(eg.  visiting www.netscape.com/ja will send 9 notifications 
for the different ja font varients in the single page 
before reload occures.)

I may be able to expose an API to re-initialize these
globa variables; but it may be non-trival.
Unfortunately, Frank (original dev) is out for few weeks
to get an accurate estimate.

Hansoo: is this a very critical feature?

Comment 4

[Hansoo Kim]

Attachment: created nsIFontMetrics::CheckFontLang(char * aLang, PRBool check)

Comment 5

[Hansoo Kim]

***** Additional Comment #4 From Roy Yokoyama 2001-11-19 18:12 -------

Created an attachment (id=64848): created nsIFontMetrics::CheckFontLang(char *
aLang, PRBool check)

I have exposed an API to set the global lang variable in nsIFontMetrics.
However, it's useless to the embedder unless we allow them to set per URL.

Comment 6

[Hansoo Kim]

***** Additional Comment #5 From Judson Valeski 2001-11-20 12:59 -------

minusing. 0.9.4 is not targeted for broad i18n distribution at this point.
please re-nominate if that's no longer the case.

Comment 7

[Hansoo Kim]

Attachment: Call NeedFontPackage() until it succeeds

Comment 8

[Hansoo Kim]

***** Additional Comment #6 From Roy Yokoyama 2001-11-20 17:47 -------

Created an attachment (id=64851)
Call NeedFontPackage() until it succeeds

Hansoo: can you try this patch?  You will get Notification until it return
success.

Comment 9

[Hansoo Kim]

Attachment: Call NeedFontPackage() until it succeeds

Comment 10

[Hansoo Kim]

***** Additional Comment #7 From Roy Yokoyama 2001-11-20 17:47 -------

Created an attachment (id=64852)
Call NeedFontPackage() until it succeeds

Hansoo: can you try this patch?  You will get the Notification msg 
until it gets success result.

Comment 11

[Hansoo Kim]

***** Additional Comment #8 From Roy Yokoyama 2001-12-04 11:06 -------

Patch sends the number of notification msgs per page until it succeeds.
It's not desirable.  Reporter wants to have one notification msg per page.

Comment 12

[Hansoo Kim]

Attachment: Check font for CJK langGroup on nsPresContext::UpdateCharSet()

Comment 13

[Hansoo Kim]

***** Additional Comment #9 From Roy Yokoyama 2001-12-10 18:34 -------

Created an attachment (id=64853)
Check font for CJK langGroup on nsPresContext::UpdateCharSet()

ftang: can you review?

Comment 14

[Hansoo Kim]

***** Additional Comment #10 From Roy Yokoyama 2001-12-13 10:28 -------

(From update of attachment 64853 [details] [diff] [review])
This patch works well for a URL without frame.	However, there still too many
notifications if the page is framed.

Comment 15

[Hansoo Kim]

***** Additional Comment #11 From Hansoo Kim 2001-12-27 16:26 -------

Roy,

Just a reminder...

Global variables for checking each lang group might
not work if user delete/install font manually
as we talked about.

mCheckLangJa,  etc.

Thanks.

Comment 16

[Hansoo Kim]

Attachment: employed state variable in FontPackageService for greater control of fontdownloading

Comment 17

[Hansoo Kim]

***** Additional Comment #12 From Roy Yokoyama 2002-01-08 15:00 -------

Created an attachment (id=64854)
employed state variable in FontPackageService for greater control of
fontdownloading

The patch is taking from ftang's suggestion expect minor
changes.  We use the existing nsFontPackageService::FontPackageHandled()
as an interface to set the status of fontdownload.

ftang: can you review?

Comment 18

[Hansoo Kim]

Attachment: Same as above except removing unnecessary code.

Comment 19

[Hansoo Kim]

***** Additional Comment #13 From Roy Yokoyama 2002-01-08 16:19 -------

Created an attachment (id=64855)
Same as above except removing unnecessary code.

ftang: can you review?

Comment 20

[Hansoo Kim]

***** Additional Comment #14 From Frank Tang 2002-01-10 13:59 -------

(From update of attachment 64855 [details] [diff] [review])
comment later. 

Comment 21

[Hansoo Kim]

------- Additional Comment #15 From Frank Tang 2002-01-10 13:59 -------

(From update of attachment 64855 [details] [diff] [review])
comment later. 

Comment 22

[Hansoo Kim]

***** Additional Comment #16 From Frank Tang 2002-01-10 14:01 -------

+static void CheckFontLangGroup(nsIAtom* lang1, nsIAtom* lang2, const char* 
lang3)
+{
+  if (lang1 == lang2) {
+    nsresult res = NS_OK;
+    nsCOMPtr<nsIFontPackageProxy> proxy =
do_GetService("@mozilla.org/intl/fontpackageservice;1", &res);
+    if (NS_SUCCEEDED(res) && proxy) {
+      proxy->NeedFontPackage(lang3);
+    }


this mean you will call do_GetService("@mozilla.org/intl/fontpackageservice;1",
&res); 
much more frequent then it was. you should cache nsIFontPackageProxy somewhere
and release it in FreeGlobal()

nsFontPackageService.cpp:
it seems you duplicate a lot of code, you should write a general state class and
create 4 object for each language group to do that. 

Comment 23

[Hansoo Kim]

Attachment: Cache the fontpkghandler and simplify the code

Comment 24

[Hansoo Kim]

***** Additional Comment #17 From Roy Yokoyama 2002-01-10 18:16 -------

Created an attachment (id=64857)
Cache the fontpkghandler and simplify the code

ftang: thanks for your comment.
I've modified to take your suggestions.

Please review at your earliest. Thanks

Comment 25

[Hansoo Kim]

***** Additional Comment #18 From Frank Tang 2002-01-11 08:55 -------

comment to http://bugzilla.mozilla.org/attachment.cgi?id=64857&action=view
1. you did right to put the release code into FreeGlobal, however, you should
NOT call the GetService in the init global. You should do that as late as
possible to lazy load it. This will prevent slow down the start up performance.
For example, if user never visit CJK page, the GetService in here should never
been called. You init gFontPackageProxy as null in the init global code and
check it is null or not in the CheckFontLangGroup code. If it is null, call
GetService.

2. you change the contract between gfx and FontPackageProxy, should we do that.
you used to pass "lang:ko" when you call gFontPackageProxy->NeedFontPackage, now
you pass "ko" when you do that. 
Is there any reason we should change that ? (as I said, we want the least
changes possible in the late game, symetic change in API consider big change)

3. in nsFontPackageHandler.cpp

+  if (NS_SUCCEEDED(rv))  {
+    // notify FontPackageService to the font download was successfull
+    fontService->FontPackageHandled(PR_TRUE, PR_FALSE, aFontPackID);
+  }
+  else  {
+    // notify FontPackageService to the font download failed
+    fontService->FontPackageHandled(PR_FALSE, PR_FALSE, aFontPackID);
+  }

should be one line:

+    fontService->FontPackageHandled(NS_SUCCEEDED(rv), PR_FALSE, aFontPackID);

Comment 26

[Hansoo Kim]

***** Additional Comment #19 From Hansoo Kim 2002-01-11 09:20 -------

Roy & Frank,

Sorry for the delay on updates.

This is where we are so far.

The fix for this bug is not going into the GM release of the current client
due to multiple reasons ( which we all know already ).

However, we are shooting to have this feature for next client release,
since we have more user populations and we have international clients
to consider for that release.

A formal request for this feature might follow as soon as
the branch for the next client is picked.

In the meantime, why don't we continue looking into this since it
might have a performace hit ? 
We can try different approches so that we can provide good benchmark data
w/ and w/o patchs to EDT later for review.

What do you think ?

Thank you in advance.

HK ( Hansoo Kim )

Comment 27

[Hansoo Kim]

***** Additional Comment #20 From [e-mail] 2002-01-11 09:44 -------

We would like this bug fixed in 0.9.8, so if you would like further details, 
please contact me directly, so we can take this discussion offline.
Thanks.

Comment 28

[Roy Yokoyama]

Attachment: 1) lazy load of gFontPackageProxy 2) fixing the contract 3) Simplify nsFontPackageHandler.cpp

ftang: can you review?

Comment 29

[Frank Tang]

Comment on attachment 69386 [details] [diff] [review]
1) lazy load of gFontPackageProxy 2) fixing the contract 3) Simplify nsFontPackageHandler.cpp

r=ftang

Comment 30

[Roy Yokoyama]

Comment on attachment 69386 [details] [diff] [review]
1) lazy load of gFontPackageProxy 2) fixing the contract 3) Simplify nsFontPackageHandler.cpp

marking as 'has-review'

Comment 31

[Roy Yokoyama]

brendan: can you /sr=?

Briefly:
Instead of using the static variable in /gfx to maintain the state 
of font download (eg. static BOOL gCheckJAFont = PR_FALSE;),
we use a nsFontPackageService::FontPackageHandled() API to allow 
resetting of the font download.

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 69386 [details] [diff] [review]
1) lazy load of gFontPackageProxy 2) fixing the contract 3) Simplify nsFontPackageHandler.cpp

>+static void CheckFontLangGroup(nsIAtom* lang1, nsIAtom* lang2, const char* lang3)
>+{
>+  if (lang1 == lang2) {
>+    nsresult res = NS_OK;
>+    if (!gFontPackageProxy) {
>+      res = nsServiceManager::GetService("@mozilla.org/intl/fontpackageservice;1",
>+                    NS_GET_IID(nsIFontPackageProxy), (nsISupports**) &gFontPackageProxy);
>+      if (NS_FAILED(res)) {
>+        NS_ASSERTION(0, "cannot get the font package proxy");

Use NS_ERROR("...") instead of NS_ASSERTION(0, "...").

>+        return;
>+      }
>+    }
>+
>+    char fontpackageid[256];

256 is generous, no big deal but what if lang3 were longer?  Do we want dynamic
allocation here, for true dynamic range?

>+    PR_snprintf(fontpackageid, sizeof(fontpackageid), "lang:%s", lang3);
>+    res = gFontPackageProxy->NeedFontPackage(fontpackageid);
>+    NS_ASSERTION(NS_SUCCEEDED(res), "cannot notify missing font package ");
>   }
> }
>+nsresult nsFontPackageService::CallDownload(const char *aFontPackID, PRInt8 iState, PRInt8 *oState)

i for in and o for out, eh?  Unorthodox (Gecko style used in most Mozilla C++
is a for argument :-).

>+  if (PL_strcmp(aFontPackID, "lang:ja") == 0) {
>+    rv = CallDownload(aFontPackID, mJAState, &mJAState);
>+  }
>+  else if (PL_strcmp(aFontPackID, "lang:ko") == 0) {
>+    rv = CallDownload(aFontPackID, mKOState, &mKOState);
>+  }
>+  else if (PL_strcmp(aFontPackID, "lang:zh-TW") == 0) {
>+    rv = CallDownload(aFontPackID, mZHTWState, &mZHTWState);
>+  }
>+  else if (PL_strcmp(aFontPackID, "lang:zh-CN") == 0) {
>+    rv = CallDownload(aFontPackID, mZHCNState, &mZHCNState);
>+  }

Just use strcmp, no need for PL_strcmp any longer (if ever there was).

>+                nsCOMPtr<nsIFontPackageService> proxy = do_GetService("@mozilla.org/intl/fontpackageservice;1", &rv);
>+                if (NS_SUCCEEDED(rv) && proxy) {

Unless you want to return rv (I didn't check the code below the context of this
diff), just null-test proxy -- sorry if you need rv, just wanted to point out
the redundancy of the if condition's clauses here.

Fix these minor issues and nits, and sr=[e-mail].

/be

Comment 33

[Roy Yokoyama]

Attachment: incorporating brendan's recommendations

>no big deal but what if lang3 were longer?  Do we want dynamic
>allocation here, for true dynamic range?
lang3 is internal contract id. I don't think we need to
do extra step to use the dynamic allocation.

Comment 34

[Roy Yokoyama]

checked in

Comment 35

[R.K.Aa.]

nsFontPackageService.cpp:43:32: nsIFontEnumerator.h: No such file or directory
nsFontPackageService.cpp: In method `nsresult 
nsFontPackageService::CallDownload (const char *, signed char, PRInt8 
*)':
nsFontPackageService.cpp:109: `nsIFontEnumerator' undeclared (first use 
this function)
nsFontPackageService.cpp:109: (Each undeclared identifier is reported 
only once for each function it appears in.)
nsFontPackageService.cpp:109: template argument 1 is invalid
nsFontPackageService.cpp:109: warning: ISO C++ forbids declaration of 
`fontEnum' with no type
nsFontPackageService.cpp:109: cannot convert `const 
nsGetServiceByContractID' to `int' in initialization
nsFontPackageService.cpp:112: base operand of `->' is not a pointer
gmake[5]: *** [nsFontPackageService.o] Error 1
gmake[5]: Leaving directory `/home/dark2/DISK/TREE2/mozilla/intl/locale/src'
gmake[4]: *** [libs] Error 2
gmake[4]: Leaving directory `/home/dark2/DISK/TREE2/mozilla/intl/locale'
gmake[3]: *** [libs] Error 2
gmake[3]: Leaving directory `/home/dark2/DISK/TREE2/mozilla/intl'
gmake[2]: *** [tier_9] Error 2
gmake[2]: Leaving directory `/home/dark2/DISK/TREE2/mozilla'
gmake[1]: *** [default] Error 2
gmake[1]: Leaving directory `/home/dark2/DISK/TREE2/mozilla'
gmake: *** [build] Error 2
[dark2@localhost mozilla]$

Comment 36

[Roy Yokoyama]

I forgot to modify makefile.in

Comment 37

[Xiaobin Lu]

I think the patch for the fix misses the unix make file change. So the Unix 
build will fail without finding the nsIFontEnumerator.h.

Comment 38

[Roy Yokoyama]

linux build fix is already checked into the trunk the same day.
(it's not just attached to this bug.  I discovered it after.)

Comment 39

[Teruko Kobayashi]

Verified as fixed in 43C AOL client.