257381 - [ps] Refactor printer list logic for the postscript module

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement)

Description

[Kenneth Herron]

Right now the logic for finding the printers supported by the postscript
printing module is intertwined with the logic for initializing the print dialog.
As part of generally improving the PS module's support for different kinds of
printers, this logic should be moved into the PS module and accessed through an API.

Comment 1

[Kenneth Herron]

Attachment: Add a printer list API

This takes the existing logic for building the PS printer list (including the
recently checked in CUPS support) and moves it into a separate class, which is
stored in the gfx/src/ps directory. The class is pretty rudimentary because I'm
just refactoring here; it's intended to be a foundation for future
improvements. I think the only practical difference is that CUPS printers are
identified as "CUPS/printername" instead of "PostScript/printername".

This patch also removes an unnecessary member variable "mGlobalNumPrinters"
from the device context spec global printers class.

Comment 2

[timeless]

i was planning to use nsISimpleEnumerator and have a class which joined the
available enumerators. - I needed this for ps+windows or ps+beos ...

Comment 3

[Kenneth Herron]

Comment on attachment 157364 [details] [diff] [review]
Add a printer list API

Oops, forget this patch. The print-to-pipe print job class assumes every
printer name starts with "PostScript/", so that needs to be addressed.

Comment 4

[Kenneth Herron]

Attachment: More extensive patch

This patch reorganizes the code in a different way, and adds support for
printing directly through the CUPS API.

A while back we added a class (stored in nsPaperPS.cpp) which had to be
accessed by both the ps component and the gtk/xlib components. Because the two
component libraries couldn't be linked against each other, this class is linked
into the shared library built in the gfx/src directory. The current patch adds
two more classes--a printer list class and a CUPS wrapper class--which are used
by both ps and gtk/xlib; rather than continuing to add code to the gfx/src
library, I've moved all three of these classes into a separate directory,
gfx/src/psshared. In this directory we build a shared library which is linked
with the ps and gtk/xlib components.

This patch also adds a print job submission class for printing to CUPS printers
directly through the CUPS API. Once all the other pieces were in place, this
was a pretty simple addition.

Comment 5

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

This looks good, but it looks like the new files in psshared aren't included in
the patch.

Also, mkaply is not a super-reviewer. I can give r+sr.

Comment 6

[Mike Kaply [:mkaply]]

> This looks good, but it looks like the new files in psshared aren't included in
> the patch.

I was wondering that myself.

I don't see any reference to the CUPS code.

Comment 7

[Kenneth Herron]

Attachment: More extensive patch v2

Oops, sorry about that. This one should be complete. I've left out the text of
nsPaperPS.cpp and nsPaperPS.h, but they would be moved from gfx/src/ps to
gfx/src/psshared as part of the checkin.

In this patch I've also changed the name of the printer list class from
nsPSPrinterManager to nsPSPrinterList. I'm having a hard time coming up with an
appropriate name for this class.

Comment 8

[Kenneth Herron]

Comment on attachment 158029 [details] [diff] [review]
More extensive patch v2

I'll just set r? for now. Roc, if you're comfortable giving r+sr that'd be
great. I'd like to get this in for the next alpha if possible.

Comment 9

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

Comment on attachment 158029 [details] [diff] [review]
More extensive patch v2

+    struct {
+	 const char * const symName;
+	 void **symPtr;
+    } syms[] = {

Make it static and outside the function, that should be smaller/faster.

+#ifndef nsCUPSShim_h___
+#include "nsCUPSShim.h"
+#endif

I wouldn't bother with this sort of thing.

> static enum PrinterType

Can't you elide 'enum'?

+    if (0 == memcmp(aName, NS_POSTSCRIPT_DRIVER_NAME,
NS_POSTSCRIPT_DRIVER_NAME_LEN))
+	 pt=kTypePS;
+    else if (0 == memcmp(aName, NS_CUPS_PRINTER, NS_CUPS_PRINTER_LEN))

These aren't really safe. They might read beyond the end of the string and
cause a fault. Actually I think it would best if you just had the
nsPSPrinterList::GetPrinterType(const nsACString& aName) version and used our
string methods.

+	  * @param aList Upon return, this is populated with the list of

Please mention that these are UTF8 strings (also in GetPrinterType).

Comment 10

[Kenneth Herron]

Attachment: Updated patch

This should address the review comments.

Comment 11

[Kenneth Herron]

This is checked in and (mostly) cleared tinderbox. Resolving FIXED.

Comment 12

[Wolfgang Rosenauer [:wolfiR]]

is there a reason why CUPS is not tried by default?
1_7_BRANCH and AVIARY_BRANCH are using CUPS by default (at least on Linux) and
1.8 uses a pref which isn't set anywhere. How about adding the pref and enable
it by default?