Closed Bug 144668 Opened 20 years ago Closed 19 years ago

Code Mozilla TrueType Printing Code

Categories

(Core :: Internationalization, enhancement)

All
Linux
enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.3beta

People

(Reporter: bstell, Assigned: zhayupeng)

References

Details

(Keywords: intl)

Attachments

(2 files, 3 obsolete files)

Write code to insert core font in the Postscript and to incrementally load the 
glyphs.
Blocks: 144663
Keywords: intl
QA Contact: ruixu → kasumi
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.
Blocks: 144669
taking, patch will come soon.
This depends on bug 144669
Assignee: bstell → pete.zha
No longer blocks: 144669
Depends on: 144669
Change to 1.3b since 1.3a already freezed
Target Milestone: --- → mozilla1.3beta
Attached patch first patch (obsolete) — Splinter Review
This patch can work(print Chinese webpage),but still need to refine.
Attached file review of patch
Louie and I have been conversing via email and AIM.
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
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 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+
Attachment #109525 - Flags: superreview?(scc)
Attachment #109511 - Attachment is obsolete: true
Attached patch working patchSplinter Review
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 on attachment 109613 [details] [diff] [review]
working patch

move bstell' review+
Attachment #109613 - Flags: superreview?(scc)
Attachment #109613 - Flags: review+
Attachment #109525 - Flags: superreview?(scc)
Blocks: 144666
Attachment #109613 - Flags: superreview?(scc) → superreview?(bryner)
Attachment #109613 - Flags: superreview?(bryner) → superreview+
Blocks: 177258
check in
Status: NEW → RESOLVED
Closed: 19 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.
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?
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.
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.
Alec's right for win32, but the linux sizes are taken from /usr/bin/nm.

It might have similar faults.
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.
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.