116158 - Enhance the workaround for bug 88554

Status: VERIFIED FIXED

(Core Graveyard :: Printing: Xprint, enhancement, P2)

Description

[Roland Mainz]

The (very simple) workaround for bug 88554 ("Xprint module should avoid using
GFX fonts unless there is no other option...") makes all CSS fonts look like
"serif" - which is slighly bad for some cases...

Comment 1

[Roland Mainz]

Swapping QA <--> Owner

Comment 2

[Roland Mainz]

Attachment: Patch for 2001-12-17-08-trunk

The patch allows some "common" PostScript font names to be used in the CSS spec
passed at nsFontMetricsXlib::Init() time.

Comment 3

[Roland Mainz]

Attachment: New patch for 2001-12-17-08-trunk per timeless's comments

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 62367 [details] [diff] [review]
New patch for 2001-12-17-08-trunk per timeless's comments

>+  nsString savedName,
>+           filteredName;

These should probably be nsAutoString. This code isn't called much so it's not
going to make much difference, but it's good to get into the habit.

>+    char *inbuffer,
>+         *outbuffer;

outbuffer is superfluous, why not append/assign (..WithConversion) directly
into filteredName?

Better yet would be to re-write the entire thing using Mozilla string classes,
you'd be able to avoid the conversion and allocation costs, but I won't make
you go through that since this looks like a semi-temporary hack. outbuffer
I would like to see gone, though.


>+        if (first)
>+        {
>+          strcpy(outbuffer, name);

So this would be filteredName.Assign(NS_ConvertASCIItoUCS2(name));

>+        {
>+          strcat(outbuffer, ",");
>+          strcat(outbuffer, name);
>+        }

And this would be 

       filteredName.Append( NS_LITERAL_STRING(",") +
	   NS_ConvertASCIItoUCS2(name) );

>+#ifdef DEBUG
>+    printf("nsFontMetricsXlib::Init: in list = '%s', filtered list '%s'\n", 
>+           NS_ConvertUCS2toUTF8(mFont->name).get(), 
>+           NS_ConvertUCS2toUTF8(filteredName).get());
>+#endif /* DEBUG */

Is this really generally useful output? If not it should be deleted or changed
to #ifdef DEBUG_gisburn or whatever your username is on your system.

Comment 5

[Roland Mainz]

Attachment: Better patch for 2001-12-29-08-trunk per dveditz's comments

[e-mail] wrote:
> >+  nsString savedName,
> >+	       filteredName;
> 
> These should probably be nsAutoString. This code isn't called much so it's
not
> going to make much difference, but it's good to get into the habit.

Fixed.

> >+	char *inbuffer,
> >+	     *outbuffer;
> 
> outbuffer is superfluous, why not append/assign (..WithConversion) directly
> into filteredName?

Fixed.

> Better yet would be to re-write the entire thing using Mozilla string
classes,
> you'd be able to avoid the conversion and allocation costs, but I won't make
> you go through that since this looks like a semi-temporary hack.

Unfortunately there is no strtok()/strtok_r()-like function in our string
classes, therefore I have to use strtok_r() - I do not see a way around it (I
have to copy the inbuffer as strtok_r() writes '\0' terminators into that
buffer).

> outbuffer
> I would like to see gone, though.

It's dead, really... :)

[snip] 
> >+#ifdef DEBUG
> >+	printf("nsFontMetricsXlib::Init: in list = '%s', filtered list '%s'\n",

> >+	       NS_ConvertUCS2toUTF8(mFont->name).get(), 
> >+	       NS_ConvertUCS2toUTF8(filteredName).get());
> >+#endif /* DEBUG */
> 
> Is this really generally useful output? If not it should be deleted or
changed
> to #ifdef DEBUG_gisburn or whatever your username is on your system.

I changed that to DEBUG_gisburn ...

Additionally I moved the hack out of nsFontMetricsXlib.cpp and moved it into
nsDeviceContextXP.cpp 's nsFontCacheXp class (minor cleanup in 
nsFontMetricsXlib.cpp).

Comment 6

[Daniel Veditz [:dveditz]]

You still define outbuffer, although you no longer use it. I'd like to see an r=
on the new patch from a module owner or peer for the gfx code because you're
moving things around I'm not that familiar with this code. Unlike your initial
patch this one puts the code in a new routine (nsFontCacheXp::GetMetricsFor),
and I can't see who calls this new method. 

Comment 7

[Roland Mainz]

dveditz wrote:
> You still define outbuffer,

Ouch.
I'll file a new patch...

> although you no longer use it. I'd like to 
> see an r= on the new patch from a module owner or peer for the gfx code

Please take a look at http://bugzilla.mozilla.org/survey.cgi?action=list ...
search for Xprint module - guess who's the module owner ? :)

I can explain what I am doing here...
Is a r=timeless sufficient then ?

> because you're moving things around I'm not that familiar with this code. 
> Unlike your initial patch this one puts the code in a new routine 
> (nsFontCacheXp::GetMetricsFor),

Which overrides |nsFontCache::GetMetricsFor()| (I originally wrote that code :),
see bug 81311 ("nsFontCache cannot be shared between multiple toolkits") ...)
See explanation below ...

> and I can't see who calls this new method. 

The method is called by layout code (which calls 
DeviceContextImpl::GetMetricsFor() which calls the nsFontCache::GetMetricsFor())
to obtain |nsIFontMetrics| objects from the font cache - if there are no such
objects the font cache tries to create new objects which fits the demands
specified by the |nsFont| argument. That's where the old hack was - in
|nsFontmetricsXlib::Init()|. However - the code there was really a (BAD!) hack
which caused - in very rare conditions - an endless loop where the font cache
tried to create a |nsIFontMetrics| with a given spec but the only got a object
with a different |nsFont|-spec (e.g. filtered by the old hack) which was not
found in a later call to GetMetricsFor() (layout seems to have some nice
assumtions about that).

Therefore I moved the "hack" into the nsFontCacheXp::GetMetricsFor() method to
filter the |nsFont| argument _before_ calling the font cache lookup to avoid
this endless loop.

Pro:
- The "hack" now works prefectly without causing any problems
- The hack is not a "hack" anymore
- We're now using a wider range of fonts (incl. "courier" and "helvetica"...)
Contra:
- Minor(=less than %1 of total printing time) slowdown of layout phase
(partially caused by the fact that we now have to handle more fonts :) while
printing with Xprint module (but we are still faster than the PostScript
module... :) because we now filter on each call of GetMetricsFor() instead of
only for creating nsFontMetricsXlib objects

Comment 8

[Roland Mainz]

Attachment: Patch for 2002-01-04-08-trunk

Comment 9

[timeless]

Comment on attachment 63839 [details] [diff] [review]
Patch for 2002-01-04-08-trunk

get's=>gets

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 63839 [details] [diff] [review]
Patch for 2002-01-04-08-trunk

timeless has re-reviewed so I'm happy. sr=dveditz

Comment 11

[rods (gone)]

r=rods

Comment 12

[Roland Mainz]

Patch checked in
(http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1010631000&maxdate=1010631540&who=timeless%25mac.com),
marking bug as FIXED.

Comment 13

[Masaki Katakai]

I'm now verifying the Xprt bugs, however, I'm not sure how to
verify this bug.

Roland,

could you give the exact instruction how to reproduce
this problem?

Comment 14

[Roland Mainz]

katakai:
See http://bugzilla.mozilla.org/show_bug.cgi?id=88554#c10 for details. That bug
contains an example PostScript job, too...

Marking VERIFIED per comment in bug 88554...