241607 - [ps] Separate postscript generation from print job handling

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

[Kenneth Herron]

In the postscript printing module, the class that generates the postscript is
also responsible for managing temporary output files, setting a printing-related
environment variable, and running the actual print command. The code for
managing the print job is very convoluted, which makes it hard to contemplate
adding new features, such as interfacing with CUPS or just intelligently
constructing an lpr command.

Comment 1

[Kenneth Herron]

Attachment: Refactor print job handling into a separate class

This removes all of the code for temporary files, environment variables, and
running commands from nsPostScriptObj into a separate set of classes for
managing print jobs. I've defined an abstract class, nsIPrintJobPS, along with
separate classes for printing to files, pipes on unix, and via system() on VMS.
This should be more extensible than the current code.

A factory class initializes selects the right kind of print job object based on
the print job spec; the print job object in turn initializes the postscript
object. This is intended to handle cases such as duplex printing, where some
print job classes can handle it themselves but others would tell the PS object
to handle it.

There are some minor changes to mozilla's behavior. First of all, postscript
output is always written to a pair of temporary files and then combined at the
end of the print job; in the past, print-to-file jobs created the final output
file instead of one of the temp files. The new behavior avoids triggering bug
226600. Also, the new implementation will try to remove the MOZ_PRINTER_NAME
environment variable from the environment, or else set it to an empty string,
after each print job.

In nsPostScriptObj I've renamed the two fields that stored the temporary file
handles; this lead to a lot of trivial changes to a lot of fprintf()
statements. I've tried to avoid a lot of unnecessary changes here, but I did
change a few of these to fputs() or fputc() calls, and removed some trivial
white space from the postscript output.

Comment 2

[tor]

Why have a nsIPrintJobPS?  Do you intend for classes other than
nsPrintJobsPS to inherit it?

Along a somewhat similar lines, why seperate nsPrintJobPS and
nsPrintJobFilePS?

Why is the print job generated into two temporary files instead
of just one?

The file combiner uses 8192 bytes of stack space, which although
we regularly have >50 deep stacks, seems a bit excessive.

Comment 3

[Kenneth Herron]

(In reply to comment #2)
> Why have a nsIPrintJobPS?  Do you intend for classes other than
> nsPrintJobsPS to inherit it?

I wanted a base class with no implementation details in it. nsPrintJobPS
contains a particular implementation of temporary files. At some point we could
in theory have a print job implementation that has no use for this particular
temp-file implementation. But yes, I expect that in the near term, any new print
job classes will inherit from nsPrintJobPS.

> Along a somewhat similar lines, why seperate nsPrintJobPS and
> nsPrintJobFilePS?

A class that prints through lpr using a dynamically generated lpr command line
has no use for the Destination stuff added to nsPrintJobFilePS. Nor does a class
that prints through the CUPS API <http://cups.org/spm.html>. One of my goals for
this patch is to develop a framework for adding new features.
 
> Why is the print job generated into two temporary files instead
> of just one?

Postscript document structuring conventions. Procedures, arrays, fonts, etc.
defined in the document are normally placed at the beginning of the document
instead of being mixed in with the page content. This lets the user use tools to
reorder pages, remove pages from the print job, and the like without running the
risk of removing something that's needed elsewhere in the document.

In mozilla's case, the freetype code can embed fonts into the print job. It
doesn't know which fonts to embed until all of the page content is processed,
and the font data needs to go toward the top of the file. So the prolog,
including the font data, is written to one file, while the page-drawing commands
are written to a second file. Mozilla has worked this way ever since the
freetype stuff was added.

Comment 4

[Kenneth Herron]

(In reply to comment #3)
> I wanted a base class with no implementation details in it. nsPrintJobPS
> contains a particular implementation of temporary files. At some point we could
> in theory have a print job implementation that has no use for this particular
> temp-file implementation. But yes, I expect that in the near term, any new print
> job classes will inherit from nsPrintJobPS.

Actually, I take this back. To fully address bug 226600, I've been thinking
about a stub print job class for print-preview operations. Such a class wouldn't
have any use for the temporary-file implementation in nsPrintJobPS, so it would
probably inherit directly from nsIPrintJobPS.

In fact, at one point I considered submitting this refactoring plus the
print-preview job class as the fix for bug 226600, but I eventually decided it
would be better to introduce the new class hierarchy separate from any bug fixes.

Comment 5

[tor]

Comment on attachment 146953 [details] [diff] [review]
Refactor print job handling into a separate class

I'll leave the call on the stack usage for the sr. r=tor

Comment 6

[Kenneth Herron]

Comment on attachment 146953 [details] [diff] [review]
Refactor print job handling into a separate class

Dbaron, you were involved with earlier patches dealing with temp files and
environment variables. Could you sr this?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Roland, any comments before I approve this?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess my question is, should this print job support be specific to Postscript
or can we reuse it? Would we want to reuse it in a Cairo printing backend, for
example, so we don't have to implement CUPS-PS and CUPS-Cairo?

Comment 9

[Kenneth Herron]

(In reply to comment #8)
> I guess my question is, should this print job support be specific to Postscript
> or can we reuse it? Would we want to reuse it in a Cairo printing backend, for
> example, so we don't have to implement CUPS-PS and CUPS-Cairo?

My goal with this patch was to enable more sophisticated printjob support, but
it also moves us toward being able to replace the current postscript renderer or
supporting multiple renderers. I don't have specific plans, but I've done a
little research on developing a PDF or DVI renderer, and I try to keep this kind
of thing in mind.

I hadn't heard of cairo before now, but it seems the cairo PS renderer just
writes to a FILE*, so it should interface well with this print job object.
However, a full cairo-based printing system is still pretty far off.

Comment 10

[Roland Mainz]

Robert O'Callahan wrote:
> I guess my question is, should this print job support be specific to 
> Postscript or can we reuse it?

With the current design of the patch the print job handling cannot be reused by
other print modules, the job handling would be bound to the PostScript module
exclusively.

> Would we want to reuse it in a Cairo printing 
> backend,

There should be no "Cairo" printing _backend_ - that code should be implemented
in a way (for example like the POstScript plugin print code on Unix/Linux) that
it can be reused by gfx/src/ps/, gfx/src/gnomeprint and gfx/src/xprint/.
Hardcoding that only to the gfx/src/ps/ module isn't a great idea now that
multiple platforms have dropped support for it.

> for example, so we don't have to implement CUPS-PS and CUPS-Cairo?

Erm... please stop thinking about "CUPS". "CUPS" is _NOT_ the only print spooler
system on Linux (think about LPRng etc.) nor on Unix (where many have their own
spooler implementations).
Instead of a CUPS-centric spooler implementation there should be something which
works crossplatform.

Comment 11

[Roland Mainz]

Kenneth Herron wrote:
> (In reply to comment #8)
> > I guess my question is, should this print job support be specific to 
> > Postscript
> > or can we reuse it? Would we want to reuse it in a Cairo printing backend, 
> > for
> > example, so we don't have to implement CUPS-PS and CUPS-Cairo?
>
> My goal with this patch was to enable more sophisticated printjob support, but
> it also moves us toward being able to replace the current postscript renderer 
> or supporting multiple renderers.

Erm... multiple renderers for different renderers should be handled in
gfx/src/gtk/nsDeviceContextSpec.cpp, NOT in the PostScript code - and stuff like
Cairo should be implemented more like the plugin printing code to avoid that it
gets hardcoded to one print module.

> I don't have specific plans, but I've done a
> little research on developing a PDF or DVI renderer, and I try to keep this 
> kind of thing in mind.

A new renderer should live in a new dir (e.g. gfx/src/dvi/ or gfx/src/pdf/;
however PDF output is already handled by the Xprint module) and should be
completely independed from the PostScript module codebase.

> I hadn't heard of cairo before now, but it seems the cairo PS renderer just
> writes to a FILE*, so it should interface well with this print job object.

No, that should not be designed like that. The Cairo stuff should be added in a
way that multiple print backends can use it, including that both OpenGL output
(for Xprint since the Xprt server now has OpenGL support) and PostScript output
work for printing. The Unix/Linux plugin printing code could be used as basis.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it would be fairly easy, and useful, to separate the file/pipe/command
stuff from the actual two-temp-file postscript generation. The former could live
in gfx/src/x11shared, and eventually handle spoolers such as CUPS, and also
handle data coming from gfx/cairo or other sources (by exposing a FILE* that
anyone can write to, the same FILE* you currently pass to Assemble). The latter
would continue in gfx/ps.

Kenneth, does that make sense?

Comment 13

[Kenneth Herron]

In my copious free time I've been looking at how to separate the print job code
from PS and I've run across a snag regarding how to initialize the print job
object. Right now it initializes itself from an nsDeviceContextSpecPS which is
obviously PS-specific.

nsIDeviceContextSpec is a COM interface. The base interface doesn't define any
useful methods. There are two full implementations, one for gtk and one for
xlib, plus independent PS and xprint interfaces which basically define the
getters that each module needs. All of these interfaces are part of their
respective modules so there's no obvious interface for an independent class to use.

So initializing the print job object seems to require creating something like
nsDeviceContextSpecPrintJob, or else moving the printjob initialization logic
out of this class and into something (probably the print job factory) which
would have to remain in the PS module (and be replicated for other users of the
print job object).

Another possibility might be to merge all of the DeviceContextSpec classes into
a single class which wouldn't be tied to any particular gfx module. The GTK and
Xlib implementations are literally identical but for their class names; bug
130857 is about merging them into one class, but it's held up for some reason.
If we had a single DeviceContextSpec class it may be possible to dispose of the
PS and xprint classes as well, though there may be issues here that I'm not
aware of.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 146953 [details] [diff] [review]
Refactor print job handling into a separate class

Alright. If it's nontrivial to separate them, let's not bother for now. we can
refactor it later. Just check it in.

+nsPrintJobPS::Assemble(FILE *aDestination = nsnull)

Why the default nsnull? That doesn't seem to be useful.

Comment 15

[Kenneth Herron]

(In reply to comment #14)
> (From update of attachment 146953 [details] [diff] [review])
> Alright. If it's nontrivial to separate them, let's not bother for now. we can
> refactor it later. Just check it in.

Er, whoops. Actually, while reviewing roc's suggestion I realized we probably
don't need two temporary files after all, because we can defer generating the
prolog until after the body of the postscript document is finished, and just
write the prolog directly into the final output stream.

This is a substantial change from what's in the patch, so I think it's better to
just write a new patch and go through another round of review.


> +nsPrintJobPS::Assemble(FILE *aDestination = nsnull)
> 
> Why the default nsnull? That doesn't seem to be useful.
> 

Comment 16

[Kenneth Herron]

Attachment: Simpler patch

This is a simpler version of the previous patch. The printjob object doesn't
own the postscript object and it doesn't supply temporary files; it just
provides a file handle for the postscript object to write the document into.

The postscript object now uses only one temporary file, used for the document
body; it defers creating the document header until the end of the process when
it can write directly into the submission file handle.

There's a printjob class for print preview which is never instantiated. The
device context spec class needs some additional work to support this properly;
see bug 226600 comment 5. I'd like to do that separately from this patch.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 149117 [details] [diff] [review]
Simpler patch

+#include "nsType8.h"
Shouldn't this be #ifdef MOZ_ENABLE_FREETYPE2?

+  if (mPSObj)
     delete mPSObj;

could be just 'delete mPSObj;'

+  if (mPrintJob)
+    delete mPrintJob;
similar

+      NS_PRECONDITION(submitFP, "No print job submission handle");
should be NS_ASSERTION

+  fprintf(f, "%s",
why not fputs? Actually I don't understand why you're breaking up these
fprintfs...

other than that, looks great

Comment 18

[Kenneth Herron]

> +  fprintf(f, "%s",
> why not fputs? Actually I don't understand why you're breaking up these
> fprintfs...

I apologize; I should have explained that. It's an attempt to address bug 223213
by splitting up a long string constant into a handful of shorter ones. The
reason for using fprintf() is that I think it's slightly easier to read than the
fputs version:

    fputs(
        "..."
        [many lines]
        "...", fp)

The performance difference between |fprintf(f, "%s", string)| and |fputs(string,
f)| is trivial.

This part of the patch isn't very important to me. Bug 223213 is trivial from a
practical standpoint, so I always planned to fix it at some convenient time. I
could remove or change it to fputs() if you'd prefer.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds reasonable. Leave them as fprintfs then.

Comment 20

[tor]

Comment on attachment 149117 [details] [diff] [review]
Simpler patch

Sorry about the delay - r=tor.

Comment 21

[Kenneth Herron]

Checked in:

Checking in Makefile.in;
/cvsroot/mozilla/gfx/src/ps/Makefile.in,v  <--  Makefile.in
new revision: 1.52; previous revision: 1.51
done
Checking in nsDeviceContextPS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsDeviceContextPS.cpp,v  <--  nsDeviceContextPS.cpp
new revision: 1.67; previous revision: 1.66
done
Checking in nsDeviceContextPS.h;
/cvsroot/mozilla/gfx/src/ps/nsDeviceContextPS.h,v  <--  nsDeviceContextPS.h
new revision: 1.38; previous revision: 1.37
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsIPrintJobPS.h,v
done
Checking in nsIPrintJobPS.h;
/cvsroot/mozilla/gfx/src/ps/nsIPrintJobPS.h,v  <--  nsIPrintJobPS.h
initial revision: 1.1
done
Checking in nsPostScriptObj.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v  <--  nsPostScriptObj.cpp
new revision: 1.114; previous revision: 1.113
done
Checking in nsPostScriptObj.h;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.h,v  <--  nsPostScriptObj.h
new revision: 1.44; previous revision: 1.43
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsPrintJobFactoryPS.cpp,v
done
Checking in nsPrintJobFactoryPS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPrintJobFactoryPS.cpp,v  <--  nsPrintJobFactoryPS.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsPrintJobFactoryPS.h,v
done
Checking in nsPrintJobFactoryPS.h;
/cvsroot/mozilla/gfx/src/ps/nsPrintJobFactoryPS.h,v  <--  nsPrintJobFactoryPS.h
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsPrintJobPS.cpp,v
done
Checking in nsPrintJobPS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPrintJobPS.cpp,v  <--  nsPrintJobPS.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/gfx/src/ps/nsPrintJobPS.h,v
done
Checking in nsPrintJobPS.h;
/cvsroot/mozilla/gfx/src/ps/nsPrintJobPS.h,v  <--  nsPrintJobPS.h
initial revision: 1.1
done
Checking in nsRenderingContextPS.cpp;
/cvsroot/mozilla/gfx/src/ps/nsRenderingContextPS.cpp,v  <-- nsRenderingContextPS.cpp
new revision: 1.76; previous revision: 1.75
done

Comment 22

[Kenneth Herron]

I overlooked the |#ifdef MOZ_ENABLE_FREETYPE2| correction on the first checkin,
but it's in now. Resolving FIXED.