NeedFontPackage() should be called multiple times in a session

VERIFIED FIXED in mozilla0.9.9

Status

()

P2
critical
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: hansoodev, Assigned: tetsuroy)

Tracking

Trunk
mozilla0.9.9
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Need /sr=)

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
*****   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
(Reporter)

Comment 2

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

I'll post a patch in a minute
(Reporter)

Comment 3

17 years ago
***** 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?
(Reporter)

Comment 4

17 years ago
Created attachment 64848 [details] [diff] [review]
created nsIFontMetrics::CheckFontLang(char * aLang, PRBool check)
(Reporter)

Comment 5

17 years ago
***** 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.

(Reporter)

Comment 6

17 years ago
***** 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.
Keywords: edt0.9.4-, topembed
Priority: -- → P2
Whiteboard: tETA-11/21
Target Milestone: --- → mozilla0.9.8
(Reporter)

Comment 7

17 years ago
Created attachment 64851 [details] [diff] [review]
Call NeedFontPackage() until it succeeds
(Reporter)

Comment 8

17 years ago
***** 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.
(Reporter)

Comment 9

17 years ago
Created attachment 64852 [details] [diff] [review]
Call NeedFontPackage() until it succeeds
(Reporter)

Comment 10

17 years ago
***** 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.
(Reporter)

Comment 11

17 years ago
***** 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.
(Reporter)

Comment 12

17 years ago
Created attachment 64853 [details] [diff] [review]
Check font for CJK langGroup on nsPresContext::UpdateCharSet()
(Reporter)

Comment 13

17 years ago
***** 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?

(Reporter)

Comment 14

17 years ago
***** 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.

(Reporter)

Comment 15

17 years ago
***** 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.
(Reporter)

Comment 16

17 years ago
Created attachment 64854 [details] [diff] [review]
employed state variable in FontPackageService for greater control of fontdownloading
(Reporter)

Comment 17

17 years ago
***** 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?
(Reporter)

Comment 18

17 years ago
Created attachment 64855 [details] [diff] [review]
Same as above except removing unnecessary code.
(Reporter)

Comment 19

17 years ago
***** 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?

(Reporter)

Comment 20

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

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

(Reporter)

Comment 21

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

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

(Reporter)

Comment 22

17 years ago
***** 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. 

(Reporter)

Comment 23

17 years ago
Created attachment 64857 [details] [diff] [review]
Cache the fontpkghandler and simplify the code
(Reporter)

Comment 24

17 years ago
***** 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
(Reporter)

Comment 25

17 years ago
***** 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);

(Reporter)

Comment 26

17 years ago
***** 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 )
(Reporter)

Comment 27

17 years ago
***** Additional Comment #20 From njbegelman@aol.com 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.
(Assignee)

Updated

17 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Assignee)

Updated

17 years ago
Keywords: edt0.9.4-, topembed → nsbeta1
Whiteboard: tETA-11/21
(Assignee)

Comment 28

17 years ago
Created attachment 69386 [details] [diff] [review]
1) lazy load of gFontPackageProxy 2) fixing the contract 3) Simplify nsFontPackageHandler.cpp

ftang: can you review?
Attachment #64848 - Attachment is obsolete: true
Attachment #64851 - Attachment is obsolete: true
Attachment #64852 - Attachment is obsolete: true
Attachment #64853 - Attachment is obsolete: true
Attachment #64854 - Attachment is obsolete: true
Attachment #64855 - Attachment is obsolete: true
Attachment #64857 - Attachment is obsolete: true

Updated

17 years ago
QA Contact: mdunn → teruko

Comment 29

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

r=ftang
(Assignee)

Comment 30

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

marking as 'has-review'
Attachment #69386 - Flags: review+
(Assignee)

Comment 31

17 years ago
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.


Whiteboard: Need /sr=
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=brendan@mozilla.org.

/be
Attachment #69386 - Flags: superreview+
(Assignee)

Comment 33

17 years ago
Created attachment 69779 [details] [diff] [review]
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.
Attachment #69386 - Attachment is obsolete: true
(Assignee)

Comment 34

17 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 35

17 years ago
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]$
(Assignee)

Comment 36

17 years ago
I forgot to modify makefile.in

Comment 37

17 years ago
I think the patch for the fix misses the unix make file change. So the Unix 
build will fail without finding the nsIFontEnumerator.h.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 38

17 years ago
linux build fix is already checked into the trunk the same day.
(it's not just attached to this bug.  I discovered it after.)
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 39

17 years ago
Verified as fixed in 43C AOL client.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.