Code Mozilla TrueType Printing Code

VERIFIED FIXED in mozilla1.3beta

Status

()

Core
Internationalization
--
enhancement
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: kill this account, Assigned: Pete Zha)

Tracking

({intl})

Trunk
mozilla1.3beta
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
Write code to insert core font in the Postscript and to incrementally load the 
glyphs.
(Reporter)

Updated

16 years ago
Blocks: 144663

Updated

16 years ago
Keywords: intl
QA Contact: ruixu → kasumi
(Reporter)

Comment 1

16 years ago
I spent some time talking with Raph Levien. Based on this I am now leaning
away from incremental glyph loading. I think we should still use Type11. To 
do this we would need to render the page to a temporary file while we get
the list of needed glyphs. Once we have rendered all the pages to the temporary 
we would then create a complete postscript file by prepending the fonts.

 1) Render pages to a temporary file
 2) Output the header/preamble
 3) Output all the needed fonts
 4) Output the temporary file
 5) Output the postamble
  outputting any postamble

This would be more efficient that incremental glyph loading, generates Level 2
Postscript making it compatable with printers/Ghostscript in the field, and
is the form we would want to use if/when we output PFD (which supports
transparency) which is probably the way we would want to go in the long run.
(Assignee)

Updated

15 years ago
Blocks: 144669
(Assignee)

Comment 2

15 years ago
taking, patch will come soon.
This depends on bug 144669
Assignee: bstell → pete.zha
No longer blocks: 144669
Depends on: 144669
(Assignee)

Comment 3

15 years ago
Change to 1.3b since 1.3a already freezed
Target Milestone: --- → mozilla1.3beta

Comment 4

15 years ago
Created attachment 109511 [details] [diff] [review]
first patch

This patch can work(print Chinese webpage),but still need to refine.
(Reporter)

Comment 5

15 years ago
Created attachment 109512 [details]
review of patch

Louie and I have been conversing via email and AIM.

Comment 6

15 years ago
Created attachment 109525 [details] [diff] [review]
refined patch after bstell's comments

answer for the comments of bstell

01),02),03) fixed
04) Do you call the AddCIDCheckCode?
    I add after the ps header, that is, behind "EndProlog".
05) these kind of look like something I'd expect to be done with
    nsAutoCString and a subroutine

    +#define FIND_FIELD(dest, p)       \
    +  char * dest = p; 	       \
    +  while ((*p) && ((*p) != '-')) { \
    +	 p++;			       \
    +  }			       \
    +  if (*p) {		       \
    +	 *p++ = 0;		       \
    +  }			       \
    +  else {			       \
    +	 continue;		       \
    +  }
    +
    +#define SKIP_FIELD(p)	       \
    +  while ((*p) && ((*p) != '-')) { \
    +	 p++;			       \
    +  }			       \
    +  if (*p) {		       \
    +	 p++;			       \
    +  }			       \
    +  else {			       \
    +	 continue;		       \
    +  }
    +

    After use bstell's font seletion code, these routine has no use, delete it

06) could you comment on why you dropped the cast?

    -  mFontPS = nsFontPS::FindFont(aFont, NS_STATIC_CAST(nsIFontMetrics*,
this));
    +  mFontPS = nsFontPS::FindFont(aFont, this);

    since nsFontPS::FindFont always use nsFontMetricsPS, so we change the
argument of the interfacei, so don't need to cast when using it.

07),08) fixed  

09) I noticed you drop the super class initializer, could you comment on this?

    -nsFontPS::nsFontPS(const nsFont& aFont, nsIFontMetrics* aFontMetrics) :
    -mFontIndex(-1)
    +nsFontPS::nsFontPS(const nsFont& aFont, nsFontMetricsPS* aFontMetrics)

    FontIndex is part of nsFontPSAFM, so move the super class init from
nsFontPS to nsFontPSAFM.

10) I noticed you drop the super class initializer, could you comment on this?

    -nsFontPSAFM::nsFontPSAFM(const nsFont& aFont, nsIFontMetrics*
aFontMetrics) :
    -nsFontPS(aFont, aFontMetrics)
    +nsFontPS*
    +nsFontPSAFM::FindFont(const nsFont& aFont, nsFontMetricsPS* aFontMetrics)
     {
    actually I don't drop, since I change the code a little,
nsFontPSAFM::nsFontPSAFM move to other part in the patch(line386). 

11) what is the logic here?

    +  nsFontPSAFM* fontPSAFM = nsnull;
    +  if (fontIndex < 0)
    +	 delete afmInfo;
    +  else
    +	 fontPSAFM = new nsFontPSAFM(aFont, afmInfo, fontIndex, aFontMetrics);
    +  return fontPSAFM;
    +}

    This logic is same to the original code. Just a little change of the form.
It's about how to select a AFM font.

12),13) fixed

14) should we be using FT2ToType8CidFontName for the key?

    +  entry->GetFamilyName(familyName);
    +  entry->GetStyleName(styleName);
    +  ToLowerCase(familyName);
    +  ToLowerCase(styleName);
    no need to do, that will complicate the code

    using FT2ToType8CidFontName is good. But it need so extra code to do so.
Since the final fontname acts as an unique id for each font, use the code above
is enough. 

15),16),17),18) fixed

19) the result of this may be correct but the logic is obscure and fragile

    +  PRUint16 slant = aFont.style + 1;i

    change the value of slant in nsIFontCatalogService.idl, so let it match the
real font style value, thus, we don't need to map from font style value to the
definition in idl file.

20),21),22),23),24),25),26) fixed

27) pls pass a boolean, atom, or int instead of a string

    +	 psObj->show(unichars, len, "Type8");
   
    I add an argument for the interface of nsPostScriptObj:show(PRUnichar*.....


28),29),30),31) fixed

32) pls open a bug to implement these

    +#ifdef MOZ_MATHML
    +nsresult
    +nsFontPSTrueType::GetBoundingMetrics(const char*	     aString,
    +					  PRUint32	     aLength,
    +					  nsBoundingMetrics& aBoundingMetrics)
    +{
    +  return NS_ERROR_NOT_IMPLEMENTED;
    +}
    +
    +nsresult
    +nsFontPSTrueType::GetBoundingMetrics(const PRUnichar*   aString,
    +					  PRUint32	     aLength,
    +					  nsBoundingMetrics& aBoundingMetrics)
    +{
    +  return NS_ERROR_NOT_IMPLEMENTED;
    +}
    +#endif //MOZ_MATHML
    +
    +#endif //MOZ_ENABLE_FREETYPE2

    This problem still exists for freetype font displaying code in gtk module.
I will file a bug later.

33) pls open a bug to convert this to use ccmaps (or a hash)

    +void nsPSFontGenerator::AddToSubset(const PRUnichar* aString, PRUint32
aLength)
    +{
    +  for (PRUint32 i=0; i < aLength; i++) {
    +	 if (mSubset.FindChar(aString[i]) == -1 ) 
    +	   mSubset.Append(aString[i]);
    +  }  
    +}
    +
    +void nsPSFontGenerator::AddToSubset(const char* aString, PRUint32 aLength)

    +{
    +  PRUnichar unichar;
    +  for (PRUint32 i=0; i < aLength; i++) {
    +	 unichar = (PRUnichar)((unsigned char)aString[i]);
    +	 if (mSubset.FindChar(unichar) == -1 ) 
    +	   mSubset.Append(unichar);
    +  } 
    +}
    +
    +#ifdef MOZ_ENABLE_FREETYPE2

    Using nsString may have bad effiency, I will file a bug later.

34),35),36),37),38),39) fixed

40) why the change?

    -  nsCOMPtr<nsIFontMetrics> mFontMetrics;
    +  nsFontMetricsPS* 	mFontMetrics;
    
    This change is caused by bug 177258.	


41),42),43),44),45),46),47),48),49) fixed

Comment 7

15 years ago
Created attachment 109527 [details]
diff between refined patch and the old patch

this diff has big size (more than 1000 lines). Most part in this diff is caused
by rename of class name, such as rename "nsFontPSTrueType" to
"nsFontPSFreeType" and "body" to "tmpBody". bstell, don't be frightened by the
size of the diff :), you can skip a lot of parts.
(Reporter)

Comment 8

15 years ago
Comment on attachment 109525 [details] [diff] [review]
refined patch after bstell's comments

Please open bugs for #32 and #33.

Please be sure to check that you are not inserting tabs.
(You don't need to fix existing tabs.)

MathML support is fully implemented in the direct FreeType Gtk 
code.

> 32) pls open a bug to implement these
> 
>     +#ifdef MOZ_MATHML
>     +nsresult
>     +nsFontPSTrueType::GetBoundingMetrics(...)
>     +{
>     +  return NS_ERROR_NOT_IMPLEMENTED;
>     ...
> 
>     This problem still exists for freetype font displaying 
>     code in gtk module.

I think the following code is already there but otherwise this
is okay.

> Index: gfx/src/ps/Makefile.in
> ===================================================================
> ...
> +ifdef MOZ_ENABLE_FREETYPE2
> +INCLUDES        += $(FT2_CFLAGS)
> +endif

Nit: indentation

> +nsFontPSFreeType::nsFontPSFreeType(const nsFont& aFont,
> +                      nsFontMetricsPS* aFontMetrics)
> +  :nsFontPS(aFont, aFontMetrics)
> +{

Nit: spacing and parameter name

> +void nsTT2Type8Generator::GeneratePSFont(FILE * f)

Nit: odd indentation

> +  nsFontPSFreeType(const nsFont& aFont, nsFontMetricsPS* aFontMetrics);
> +  nsresult         Init(nsITrueTypeFontCatalogEntry* aEntry,
> +                        nsPSFontGenerator* aPSFontGen);

Nit: indentation

> +nsPostScriptObj::show(const PRUnichar* txt, int len,
> +               const char *align, int aType)

Please open the bugs and fix the nits and with that r=bstell@ix.netcom.com
Attachment #109525 - Flags: review+
(Reporter)

Updated

15 years ago
Attachment #109525 - Flags: superreview?(scc)
(Reporter)

Updated

15 years ago
Attachment #109511 - Attachment is obsolete: true

Comment 9

15 years ago
Created attachment 109613 [details] [diff] [review]
working patch

working patch following bstell's suggestion.
issue 32) and 33) have been filed as bug 185934 and bug 185935
Attachment #109525 - Attachment is obsolete: true
Attachment #109527 - Attachment is obsolete: true

Comment 10

15 years ago
Comment on attachment 109613 [details] [diff] [review]
working patch

move bstell' review+
Attachment #109613 - Flags: superreview?(scc)
Attachment #109613 - Flags: review+

Updated

15 years ago
Attachment #109525 - Flags: superreview?(scc)
(Reporter)

Updated

15 years ago
Blocks: 144666

Updated

15 years ago
Attachment #109613 - Flags: superreview?(scc) → superreview?(bryner)
Attachment #109613 - Flags: superreview?(bryner) → superreview+
(Assignee)

Updated

15 years ago
Blocks: 177258

Comment 11

15 years ago
check in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
So... this checkin added about 35KB of data to our static size.  I can't find
where they're hiding (see
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1042695300.26214.gz for
the log that shows the jump).  But is there any way we can make whatever data
tables are taking all that space smaller?  The ps module already has a gigantic
data segment; we want to be shrinking it, not growing it.
(Reporter)

Comment 13

15 years ago
This code is builds some tables then frees them. Other than that there are a
lot of fprintf to generate the postscript.

As far as I understand this there should not be any large tables

Does the line "+34440	kFCSCID" mean that kFCSCID is 34K?


----------- Output from codesize diff log ------------- 
  Overall Change in Size
  	Total:	     +47284 (+52392/-5108)
  	Code:	     +12684 (+17672/-4988)
  	Data:	     +34600 (+34720/-120)
  
  libgfxps.so
  	Total:	     +47380 (+52392/-5012)
  	Code:	     +12684 (+17672/-4988)
  	Data:	     +34696 (+34720/-24)
  	     +34472 (+34472/+0)	R (DATA)
  		     +34472 (+34472/+0)	UNDEF:libgfxps.so:R
  			     +34440	kFCSCID
  			        +16	_Q221nsIFontCatalogService34GetIID__21nsIFontCatalogService..0.iid
  			        +16
_Q227nsITrueTypeFontCatalogEntry40GetIID__27nsITrueTypeFontCatalogEntry..0.iid
good question.  ;) blythe?  any ideas what's up here?

Comment 15

15 years ago
Yeah, the "+34440 kFCSCID" should indicate the actual size change of that
symbol.  If a new symbol, then the actual size.

The size accuracy is the same as reported by /usr/bin/nm.

Let me know if this turns out not to be the case; it would be the first such
occurance, and I'd have to dig deeper.

Comment 16

15 years ago
codesighs just makes good guesses at codesize - it looks at the list of symbols
and their offsets, and guesses that the current symbol is as big as the space
between the current offset and the next one. Unfortunately, this means static
(non-public) symbols like static functions and static data, end up being rolled
into the previous symbol... which explains why a 128-bit CID would look like it
was 47k in size. there must be 47k of non-public data there.

Comment 17

15 years ago
Alec's right for win32, but the linux sizes are taken from /usr/bin/nm.

It might have similar faults.

Comment 18

15 years ago
so hopefully sometime soon (later today), I will try to get access to the tbox
machine and figure out why the CID is reported as being 34k in size.

Comment 19

15 years ago
True type printing is working on 01-21 trunk build / Linux RH7.2, mark this as
verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.