144668 - Code Mozilla TrueType Printing Code

Status: VERIFIED FIXED

(Core :: Internationalization, enhancement)

Description

[kill this account]

Write code to insert core font in the Postscript and to incrementally load the 
glyphs.

Comment 1

[kill this account]

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.

Comment 2

[Pete Zha]

taking, patch will come soon.
This depends on bug 144669

Comment 3

[Pete Zha]

Change to 1.3b since 1.3a already freezed

Comment 4

[Louie Zhao]

Attachment: first patch

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

Comment 5

[kill this account]

Attachment: review of patch

Louie and I have been conversing via email and AIM.

Comment 6

[Louie Zhao]

Attachment: 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

[Louie Zhao]

Attachment: 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.

Comment 8

[kill this account]

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=[e-mail]

Comment 9

[Louie Zhao]

Attachment: working patch

working patch following bstell's suggestion.
issue 32) and 33) have been filed as bug 185934 and bug 185935

Comment 10

[Louie Zhao]

Comment on attachment 109613 [details] [diff] [review]
working patch

move bstell' review+

Comment 11

[Louie Zhao]

check in

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[kill this account]

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

good question.  ;) blythe?  any ideas what's up here?

Comment 15

[Garrett Arch Blythe]

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

[Alec Flett]

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

[Garrett Arch Blythe]

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

It might have similar faults.

Comment 18

[Garrett Arch Blythe]

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

[Yuying Long]

True type printing is working on 01-21 trunk build / Linux RH7.2, mark this as
verified.