324060 - Using paper sizes from CUPS in GTK and PostScript modules

Status: NEW

(Core :: Printing: Output, defect)

Description

[Arne John Glenstrup]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060119 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060119 Firefox/1.5

Only standard paper sizes are available in the print dialog,
which can prevent printing entirely on printers (e.g. label
printers) without any of these paper sizes.
When using CUPS, the actual paper sizes for each printer can
be obtained, e.g. in the nsPaperPS object, and displayed for
the user.
I'll attach 2 patches that attempt to do 4 things (sorry!)
1a) Add CUPS paper sizes for GTK and PostScript (tested).
1b) It seems that some of the preference usage in for
   instance nsDeviceContextSpecG.cpp is not in sync with current
   preference settings; I've tried to remedy that, too.
1c) When interacting directly with CUPS (in nsPrintJobCUPS),
   the page size is transmitted (print command is not used
   in this case, and can thus NOT transfer information on
   media size!)
2) Add CUPS paper sizes for Qt (UNTESTED!)

Reproducible: Always

Steps to Reproduce:
1. Using CUPS, open print dialog
2. Click "Properties..."
3. Click "Paper Size:"

Actual Results:  
Only the 6 standard paper sizes

Expected Results:  
All paper sizes supported by the current printer, as specified by CUPS

Comment 1

[Arne John Glenstrup]

Attachment: Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing

Patch
1a) adding support for importing paper sizes from CUPS into GTK and PostScript printing
1b) fixing some preference names that seemed to be out of sync with present-day preference usage
1c) adding support for sending the paper size directly to CUPS when printing

Comment 2

[Arne John Glenstrup]

Attachment: Untested patch adding support for importing paper sizes from CUPS into Qt

Untested patch adding support for importing paper sizes from CUPS into Qt.
Note that it depends on patch 209020.

Comment 3

[Kenneth Herron]

Comment on attachment 209020 [details] [diff] [review]
Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing

I have some quick comments on this patch.

>--- gfx/src/gtk/nsDeviceContextGTK.cpp	23 Aug 2005 02:11:31 -0000	1.132
>+++ gfx/src/gtk/nsDeviceContextGTK.cpp	19 Jan 2006 22:14:03 -0000
>@@ -573,18 +573,45 @@ NS_IMETHODIMP nsDeviceContextGTK::GetDev
>     // default/PS
>     static NS_DEFINE_CID(kCDeviceContextPS, NS_DEVICECONTEXTPS_CID);
>   
>     // Create a Postscript device context 
>     nsCOMPtr<nsIDeviceContextPS> dcps(do_CreateInstance(kCDeviceContextPS, &rv));
>     NS_ASSERTION(NS_SUCCEEDED(rv), "Couldn't create PS Device context.");
>     if (NS_FAILED(rv)) 
>       return NS_ERROR_GFX_COULD_NOT_LOAD_PRINT_MODULE;
>+
>+    const char* aPrinterName;
>+    rv = spec->GetPrinterName(&aPrinterName);
>+    if (NS_FAILED(rv))
>+      rv = dcps->SetSpec(aDevice);
>+    else {
>+      nsXPIDLCString
>+        fullPrinterName, /* Full name of printer incl. driver-specific prefix */ 
>+        printerName;     /* "Stripped" name of printer */
>+      fullPrinterName.Assign(aPrinterName);
>+      printerName.Assign(aPrinterName);
>   
>-    rv = dcps->SetSpec(aDevice);
>+      PrintMethod type = pmInvalid;
>+      rv = nsDeviceContextSpecGTK::GetPrintMethod(printerName, type);
>+      if (NS_FAILED(rv))
>+        return rv;
>+
>+      /* "Demangle" postscript printer name */
>+      if (type == pmPostScript) {
>+        /* Strip the printing method name from the printer,
>+         * e.g. turn "PostScript/foobar" to "foobar" */
>+        PRInt32 slash = printerName.FindChar('/');
>+        if (kNotFound != slash)
>+          printerName.Cut(0, slash + 1);
>+      }
>+      
>+      rv = dcps->SetSpec(aDevice, fullPrinterName.get (), printerName.get ());
>+    }
>+

None of this should be necessary. First of all, the format of postscript-module printer names is a private implementation detail of the PS module. It shouldn't be necessary to embed PS name-parsing rules into other classes. (It might be nice to hide them in the print dialog, but that's not the issue here.)

Second, |spec| here (from which you get the printer name) is an alias for |aDevice| which is already passed to the PS device context. The PS device context can extract and parse the printer name from the argument it's already getting; you don't need to do that here.

Third, it doesn't look like you're doing anything useful with fullPrinterName and printerName anyway. You're just passing them to nsDeviceContextPS, which just passes them to nsPostScriptObj, which just ignores them.

>diff -U8 -p -r1.68 nsDeviceContextSpecG.cpp
>--- gfx/src/gtk/nsDeviceContextSpecG.cpp	19 Oct 2005 16:37:28 -0000	1.68
>+++ gfx/src/gtk/nsDeviceContextSpecG.cpp	19 Jan 2006 22:14:04 -0000
>@@ -682,36 +682,36 @@ NS_IMETHODIMP nsDeviceContextSpecGTK::Cl
> /* Get prefs for printer
>  * Search order:
>  * - Get prefs per printer name and module name
>  * - Get prefs per printer name
>  * - Get prefs per module name
>  * - Get prefs
>  */
> static
>-nsresult CopyPrinterCharPref(nsIPref *pref, const char *modulename, const char *printername, const char *prefname, char **return_buf)
>+nsresult CopyPrinterCharPref(nsIPref *pref, const char *modulename, const char* prefix, const char *printername, const char *prefname, char **return_buf)

It looks like the changes to |CopyPrinterCharPref()| are to let you look up prefs starting with "print.tmp.printerfeatures" as well as "print.printer_". As I understand it, the "print.tmp.printerfeatures" pref branch is actually created by the device context spec to communicate settings to the print dialog. It shouldn't be necessary to read any of those prefs while setting up for the print dialog.

>--- gfx/src/ps/nsDeviceContextPS.cpp	23 Aug 2005 02:11:33 -0000	1.74
>+++ gfx/src/ps/nsDeviceContextPS.cpp	19 Jan 2006 22:14:04 -0000
>@@ -129,17 +129,19 @@ nsDeviceContextPS::~nsDeviceContextPS()
> [...]
>+    const char* paperName = nsnull;
>+    psSpec->GetPaperName(&paperName);
>+    if (paperName) mPrintJob->SetPaperName(paperName);

This shouldn't be necessary either. the psSpec argument is passed to the print job class factory, which passes it to the print job class during initialization. If the print job class needs the paper name, it can get it then.

Comment 4

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

Comment on attachment 209022 [details] [diff] [review]
Untested patch adding support for importing paper sizes from CUPS into Qt

zack isn't doing reviews. Let's defer this one until the first one is landed.

Comment 5

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

Comment on attachment 209020 [details] [diff] [review]
Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing

Please address kherron's comments and then resubmit. When you do resubmit, ask for review from kherron and sr from me. Thanks!

Comment 6

[Arne John Glenstrup]

Attachment: Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing

(In reply to comment #3)
Thanks for your excellent comments, Kenneth---I entirely agree.

This new patch is substantially smaller, but achieves the same
goal---which is great, of course.

You'll find that I've also made a change to
nsPrinterEnumeratorGTK::GetDefaultPrinterName, because I found that
it gets called (by nsGlobalWindow::Print) in the sense "tell me which
printer the user thinks mozilla will print to."  Originally, it
simply returned the first printer in the list, which is the system
default printer.  As far as I can gather, this means that the user
must change the default printer OUTSIDE mozilla to print preview on
a different printer than the last one used INSIDE mozilla.

The change I suggest is to use the value of print.print_printer,
i.e., the last used printer inside mozilla.

If you agree on this interpretation, you might ask whether the change has
been made in the correct place?

Comment 7

[Kenneth Herron]

Comment on attachment 209263 [details] [diff] [review]
Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing

>--- gfx/src/nsPrintOptionsImpl.cpp	2 Sep 2005 09:28:49 -0000	1.78
>+++ gfx/src/nsPrintOptionsImpl.cpp	22 Jan 2006 09:44:12 -0000
>@@ -1095,29 +1095,31 @@ nsPrintOptions::InitPrintSettingsFromPre
>   aPS->GetIsInitializedFromPrefs(&isInitialized);
> 
>   if (isInitialized)
>     return NS_OK;
> 
>   nsAutoString prtName;
>   // read any non printer specific prefs
>   // with empty printer name
>-  nsresult rv = ReadPrefs(aPS, prtName, aFlags);
>+  nsresult rv = ReadPrefs(aPS, prtName, 
>+                          aFlags & ~nsIPrintSettings::kInitSavePrinterName);

We have other bug reports about the issue you're trying to fix here, and as it happens I'm working one of them. See bug 324072. Even if that weren't the case, this change is unrelated to the bug at hand, it's to a file belonging to a different module (so it's under someone else's control), and it has implications for other platforms (windows & mac). I'm afraid it's not appropriate to include it in this patch.

>--- gfx/src/gtk/nsDeviceContextSpecG.cpp	19 Oct 2005 16:37:28 -0000	1.68
>+++ gfx/src/gtk/nsDeviceContextSpecG.cpp	22 Jan 2006 09:44:21 -0000
>@@ -800,19 +855,23 @@ NS_IMETHODIMP nsPrinterEnumeratorGTK::En
>   return NS_OK;
> }
> 
> /* readonly attribute wstring defaultPrinterName; */
> NS_IMETHODIMP nsPrinterEnumeratorGTK::GetDefaultPrinterName(PRUnichar **aDefaultPrinterName)
> {
>   DO_PR_DEBUG_LOG(("nsPrinterEnumeratorGTK::GetDefaultPrinterName()\n"));
>   NS_ENSURE_ARG_POINTER(aDefaultPrinterName);
>-
>-  GlobalPrinters::GetInstance()->GetDefaultPrinterName(aDefaultPrinterName);
>-
>+ 
>+  nsresult rv;
>+  nsCOMPtr<nsIPref> pPrefs = do_GetService(NS_PREF_CONTRACTID, &rv);
>+  if (!NS_FAILED(rv))
>+    rv = pPrefs->CopyUnicharPref ("print.print_printer", aDefaultPrinterName);
>+  if (NS_FAILED (rv))
>+    GlobalPrinters::GetInstance()->GetDefaultPrinterName(aDefaultPrinterName);

This change isn't appropriate to this bug either. Aside from the the fact it's out of scope, you're not verifying that the printer from print.print_printer is still valid (see bug 323781).

>@@ -1148,28 +1207,29 @@ NS_IMETHODIMP nsPrinterEnumeratorGTK::In
>     printerFeatures.SetSupportsResolutionNameChange(PR_FALSE);
>     printerFeatures.SetSupportsColorspaceChange(PR_FALSE);
> #endif /* SET_PRINTER_FEATURES_VIA_PREFS */ 
>       
> #ifdef SET_PRINTER_FEATURES_VIA_PREFS
>     printerFeatures.SetCanChangeOrientation(PR_TRUE);
> #endif /* SET_PRINTER_FEATURES_VIA_PREFS */
> 
>-    nsXPIDLCString orientation;
>-    if (NS_SUCCEEDED(CopyPrinterCharPref(pPrefs, "postscript", printerName, "orientation", getter_Copies(orientation)))) {
>-      if (!PL_strcasecmp(orientation, "portrait")) {
>+    int orientationIdx;
>+    if (NS_SUCCEEDED(GetPrinterIntPref(pPrefs, "postscript", fullPrinterName, "print_orientation", &orientationIdx))) {
>+      switch (orientationIdx) {
>+      case 0:

Once again, this is out of scope for the bug at hand. Considering the new |GetPrinterIntPref| this change adds quite a bit of code to this patch. I see that other parts of the printing code refer to a "print_orientation" pref so there may be a real issue here, but I'd rather see it handled in a bug specific to that subject.

>+    PRBool hasSpoolerCmd;
>+    switch (nsPSPrinterList::GetPrinterType(fullPrinterName)) {
>+    case nsPSPrinterList::kTypePS: 
>+    case nsPSPrinterList::kTypeCUPS: 
>+      hasSpoolerCmd = true;
>+      break;
>+    default: hasSpoolerCmd = false;
>     }
>-
>-    PRBool hasSpoolerCmd = (nsPSPrinterList::kTypePS ==
>-        nsPSPrinterList::GetPrinterType(fullPrinterName));

Aside from being out of scope this is just wrong. CUPS printers don't have a spooler command.

>--- gfx/src/ps/nsPrintJobPS.cpp	21 May 2005 15:33:09 -0000	1.7
>+++ gfx/src/ps/nsPrintJobPS.cpp	22 Jan 2006 09:44:22 -0000
>@@ -359,16 +359,20 @@ nsPrintJobCUPS::Init(nsIDeviceContextSpe
>     /* Printer name */
>     const char *printerName = nsnull;
>     aSpec->GetPrinterName(&printerName);
>     NS_ENSURE_TRUE(printerName, NS_ERROR_GFX_PRINTER_NAME_NOT_FOUND);
> 
>     const char *slash = strchr(printerName, '/');
>     mPrinterName = slash ? slash + 1 : printerName;
>     mJobTitle.SetIsVoid(PR_TRUE);
>+    /* Paper name */
>+    const char* paperName = nsnull;
>+    aSpec->GetPaperName (&paperName);

Could you lose the space after the function name here? It's not the style followed in this file.

>--- gfx/src/psshared/nsCUPSShim.cpp	8 May 2005 15:01:20 -0000	1.3
>+++ gfx/src/psshared/nsCUPSShim.cpp	22 Jan 2006 09:44:22 -0000
>@@ -40,23 +40,26 @@
> #include "nsString.h"
> #include "nsCUPSShim.h"
> #include "prlink.h"
> 
> 
> // List of symbols to find in libcups. Must match symAddr[] defined in Init().
> // Making this an array of arrays instead of pointers allows storing the
> // whole thing in read-only memory.
>-static const char gSymName[][sizeof("cupsPrintFile")] = {
>+static const char gSymName[][sizeof("cupsPpdOpenFile")] = {

This is a mistake? "cupsPpdOpenFile" isn't in the array and "cupsPrintFile" is longer than "ppdOpenFile".

>     { "cupsAddOption" },
>     { "cupsFreeDests" },
>     { "cupsGetDest" },
>     { "cupsGetDests" },
>     { "cupsPrintFile" },
>     { "cupsTempFd" },
>+    { "cupsGetPPD" },
>+    { "ppdOpenFile" },
>+    { "ppdClose" },

>--- gfx/src/psshared/nsPaperPS.cpp	7 Sep 2004 17:51:50 -0000	1.1
>+++ gfx/src/psshared/nsPaperPS.cpp	22 Jan 2006 09:44:22 -0000
> [...]
>+#if 0
>+nsPaperSizePS::nsPaperSizePS() {
>+    DO_PR_DEBUG_LOG(("nsPaperSizePS::nsPaperSizePS()\n"));
>+    mList = mDefaultList;
>+    mCount = COUNTOF(mDefaultList);
>+    mDynamicList = nsnull;
>+    mDynamicCount = 0;
>+    mCurrent = 0; 
>+}
>+#endif /* 0 */

This doesn't need to be in here at all, does it?

>+nsPaperSizePS::nsPaperSizePS (const char* fullPrinterName,
>+                              const char* printerName) {

You're adding a lot of complexity here by trying to make one class that can handle everything. Let me suggest you structure this as follows: a base nsPaperSizePS class for non-CUPS printers, a subclass of that for CUPS printers, and a factory function. The factory function would take a printer name as an argument and return an instance of the correct paper size class. If you'd like to see an example of this kind of thing, see gfx/src/ps/nsPrintJobFactoryPS.* and gfx/src/ps/nsPrintJobPS.*.

>+    mPrefSvc = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+    if (!NS_SUCCEEDED(rv)) return;
>+    rv = mPrefSvc->GetBranch("print.", getter_AddRefs(mPref));
>+    if (!NS_SUCCEEDED(rv)) return;
>+    // Should we try cups?
>+    PRBool useCups = PR_TRUE;
>+    mPref->GetBoolPref("postscript.cups.enabled", &useCups);

You don't need to check this. You can assume that if you were passed a CUPS name then CUPS is available. Even if you did have to check this, it looks like mPrefSvc and mPref are only used in this function so they don't need to be member variables, do they?

>+    mCups.Init();

Initializing a CUPS shim is expensive. We need to try very hard to avoid creating & destroying one for every CUPS printer. The device context spec could pass one to the factory function, for example.

>+    mDynamicCount = ppd->num_sizes;
>+    mDynamicList =
>+        (nsPaperSizePS_*) calloc (sizeof (nsPaperSizePS_), mDynamicCount);

If this were a CUPS-specific subclass, you might consider holding on to the original ppd object for the life of the paper size object and serving size requests directly from that, instead of building an array of nsPaperSizePS_ structs. If not, you should allocate the nsPaperSizePS_ array using new[] instead of calloc().

>+            mDynamicList [j].width_mm = rintf (size.width * 0.35277);

You need to define this magic number 0.35277 in some symbolic way. And rintf() is apparently new to C99 so it's not portable enough for mozilla. See <http://docs.hp.com/en/B9106-90010/rint.3M.html> for example, where it's only supported on their 64-bit platform.

Comment 8

[Arne John Glenstrup]

Attachment: Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing

(In reply to comment #3)

Thanks again, Kenneth, for useful and precise comments.  I entirely agree.

This attachment attempts to implement what you suggest, getting
rid of the intermediate data structure for holding paper sizes,
serving requests directly from the PPD structure.

Comment 9

[Arne John Glenstrup]

Attachment: Improved patch to always display a paper size, even if no preference value exists

Comment 10

[Arne John Glenstrup]

Attachment: Bug fix of patch 210887, exporting nsPaperFactoryPS

Comment 11

[Wolfgang Rosenauer [:wolfiR]]

out of curiosity I've tested the latest patch but it doesn't seem to make a difference. The paper size is always letter although my cups printers are A4 printers.
With NSPR_LOG_MODULES=PaperSizePS:5 I only get output like this:
1446704384[895beb0]: nsPaperSizeCUPS::nsPaperSizeCUPS('CUPS/gb_2_1_19', 'gb_2_1_19')
when switching printers but no hint about the paper size to be available.

Comment 12

[Arne John Glenstrup]

(In reply to comment #11)

A little more debug info can hopefully be emitted by letting
NSPR_LOG_MODULES=PaperSizePS:5,DeviceContextSpecGTK:5

It would also be interesting to know which paper sizes are listed when you
click File->Print...->Properties...->Paper Size.

Compare the list of paper sizes with your system's CUPS setup (use a printer
configurator, or some CUPS aware program like OpenOffice or GIMP);  if you
don't get the CUPS paper sizes, but only the six mozilla default paper
sizes, something's not as expected.

Comment 13

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #12)
> It would also be interesting to know which paper sizes are listed when you
> click File->Print...->Properties...->Paper Size.

OK, my list is longer than 6 entries but even if I use a CUPS printer which
has no "Letter" format and I see in the log that A4 is chosen correctly the UI still shows "Letter" as paper-format as set value. Could it be that the default "Letter" is always avaiable in the UI because of some reason?
 
> Compare the list of paper sizes with your system's CUPS setup (use a printer
> configurator, or some CUPS aware program like OpenOffice or GIMP);  if you
> don't get the CUPS paper sizes, but only the six mozilla default paper
> sizes, something's not as expected.

I see more values so the list seems to be gathered correctly from CUPS.
However Letter seems to be always available.

In addition you search for the paper defined as pref if available and use this which is correct. But if you don't have a paper defined already you just take the first paper you get from CUPS.
But CUPS can define a default paper size which should be used in that case.
(sorry, I never programmed with cups libs so I don't know how to get it)
At least this is what is displayed as in this example:

Hygiea:~ # lpoptions -p testy -l
Duplex/Double-Sided Printing: *None DuplexNoTumble DuplexTumble
PageSize/Media Size: Legal Executive A4 A5 B5 *EnvISOB5 Env10 EnvC5 EnvDL EnvMonarch
InputSlot/Media Source: *Default Tray1 Tray2 Tray3 Manual
PageRegion/PageRegion: Legal Executive A4 A5 B5 EnvISOB5 Env10 EnvC5 EnvDL EnvMonarch
Resolution/Output Resolution: 150dpi *300dpi 600dpi 1200dpi 2400dpi

I've tested your patch with this printer. The * marked size is the default
set in the PPD. I've removed Letter from the available list but it's still displayed in the UI size-list.

Comment 14

[Arne John Glenstrup]

Attachment: Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing

Updated the patch:
- file has been moved into widget/src/gtk2
- added minor feature, looking for CUPS system default paper
  size when none is present in the stored preferences

Kenneth, could you please suggest what needs to be done for this patch
to be usable (i.e., eventually committable)?

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

Arne,

(In reply to comment #14)
> Updated the patch:
> - file has been moved into widget/src/gtk2
> - added minor feature, looking for CUPS system default paper
>   size when none is present in the stored preferences

I've also prepared a modified patch to use the default paper given by the PPD configuration. I had a look at your patch but it seems that you never use your SytemDefault method?

And as addition: After some more testing it seems that my first negative test-results were caused by old preferences. If I use a fresh profile I always got the correct results.

mPPD should be initialized with nsnull.

Comment 16

[Arne John Glenstrup]

Attachment: Patch adding support for importing paper sizes from CUPS into GTK and PostScript printing

> I've also prepared a modified patch to use the default paper given by the PPD
> configuration. I had a look at your patch but it seems that you never use your
> SytemDefault method?

I forgot.  This updated patch includes the call.

> mPPD should be initialized with nsnull.

What do you mean here?  mPPD is not a static member, and it
does get initialised in the constructor.

Comment 17

[Wolfgang Rosenauer [:wolfiR]]

Kenneth: any objections to that patch? At least I would love to see it in Firefox2

Comment 18

[Arne John Glenstrup]

Attachment: Patch importing CUPS paper sizes for mozilla 1.8 branch

Updated the patch for MOZILLA_1_8_BRANCH, and extended it to display
human-readable paper sizes, if available.

Kenneth: Where do you suggest we go from here with this bug?

Comment 19

[Arne John Glenstrup]

Attachment: Patch importing CUPS paper sizes for seamonkey trunk

FWIW, this is a slightly older patch applicable to the trunk.
However, as printing directly to printers using the trunk version
is not possible, cf. bug 335754, this patch is currently less
interesting.

Comment 20

[Arne John Glenstrup]

Attachment: Patch importing CUPS paper sizes for seamonkey trunk

Now that bug 335754 has been fixed, here is a patch for the trunk.

Comment 21

[timeless]

Attachment: comments on attachment 247064: Patch importing CUPS paper sizes for seamonkey trunk

Comment 22

[Arne John Glenstrup]

Attachment: Patch importing CUPS paper sizes for seamonkey trunk

Minor changes in response to comment #21.

Comment 23

[Arne John Glenstrup]

Attachment: Responses to comments in attachment #247841

In response to attachment #247841 [details] of comment #21:

Thanks for your comments; I have followed your advice in many
cases, and this attachment discusses the remaining ones.

Comment 24

[timeless]

Comment on attachment 247955 [details]
Responses to comments in attachment #247841 [details]

>>scary.

by scary i was complaining about the lack of a temporary between an in and its replacement. it's no big deal.

>>>Index: gfx/src/psshared/nsPSPrinters.h
>>>+#include "nsCOMPtr.h"

in general if the header itself isn't using something, you could probably stick the includes into the files that include the header. if it absolutely had to be in the header, that is really scary.

it doesn't really matter at all. i just noted it because it was the *only* change in the file. it's very much stylistic (except for build time penalties involved in opening a file that isn't needed for any c++ files that include this chain and don't need this file, of which there are probably very few anyway).

the other changes can certainly be handled in a later bug.

Comment 25

[timeless]

Comment on attachment 247950 [details] [diff] [review]
Patch importing CUPS paper sizes for seamonkey trunk

+    nsCString paperName = nsnull;

most of our magical classes have good defaults (ns*String, ns*Ptr<*>), don't assign nsnull here.

i'd probably write PaperSizeManagerBlah instead of getting the Blah stuck in the middle.

Comment 26

[Arne John Glenstrup]

Attachment: Patch importing CUPS paper sizes for seamonkey trunk

Response to comment #24: nsCOMPtr.h is used by nsPSPrinters.h
itself.  Seems that nsPSPrinters.h previously has only been
included at places where nsCOMPtr.h had already been included,
thus causing no error messages.

Response to comment #25: Fixed the two issues.

Is this patch ready for superreview?

Comment 27

[Arne John Glenstrup]

This problem still persists; it is impossible to print in any other
format than one of seven standard formats, rendering the use of
some printers (e.g., photo printers, label printers) impossible.

I would very much like to see Mozilla improved to be able to use CUPS
information on paper sizes.  Further, I would be willing to make the
present patch work with the latest Mozilla, if someone could tell me
what steps would be necessary and sufficient to ensure it get included
in future Mozilla releases?

Comment 28

[Marcelo Ricardo Leitner]

Attachment #245644 [details] [diff]:

>+nsPaperSizeCUPS::~nsPaperSizeCUPS() {
>+    if (mPPD) mCups.mPpdClose (mPPD);
>+    if (mPageSizeText) delete (mPageSizeText);
>+}

Without proper initializing these variables at nsPaperSizeCUPS::nsPaperSizeCUPS(), firefox will crash if you open the print dialog, select a printer with a valid PPD and then select a printer without a PPD, because those pointers will contain invalid values.

Comment 29

[Kenneth Herron]

Comment on attachment 245644 [details] [diff] [review]
Patch importing CUPS paper sizes for mozilla 1.8 branch

Review of attachment 245644 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I haven't worked on Mozilla in years.

Comment 30

[Kenneth Herron]

Comment on attachment 249890 [details] [diff] [review]
Patch importing CUPS paper sizes for seamonkey trunk

Review of attachment 249890 [details] [diff] [review]:
-----------------------------------------------------------------

.