377096 - Factor out nsIPrintOptions::AvailablePrinters

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement)

Description

[Kenneth Herron]

Attachment: patch v1

This is an attempt to simplify the interface between the cross-platform print dialog and the system's printing support. The print dialog (among others) calls nsIPrintOptions::AvailablePrinters() to get a printer list, but the AvailablePrinters implementation is just a wrapper around nsIPrinterEnumerator::EumeratePrinters. nsIPrinterEumerator is also scriptable, so callers could just call it directly.

Aside from removing AvailablePrinters, I've also changed nsIPrinterEnumerator::EnumeratePrinters to make it simpler. The current design requires a lot of manual memory allocations and frees. I've changed it to construct and return a string enumerator, and renamed it GetPrinterNameList.

One of the callers for AvailablePrinters was nsPrintEngine::CheckForPrinters. This function makes sure a newly created nsPrintSettings object includes a printer setting. The current nsPrintSettings factory always sets a printer, so this function is unnecessary and I've removed it.

I've tested this on GTK and MacOSX (which doesn't actually use any of this code). The only platform-specific code is the nsIPrinterEnumerator implementations; there are about six of them in the tree for different platforms. Most of them are virtually identical to the GTK version. I haven't included a patch for the Photon version because it's significantly different.

Comment 1

[Stuart Parmenter]

Comment on attachment 261202 [details] [diff] [review]
patch v1

>   PRInt32 printerInx = 0;
>-  while( count < numItems ) {
>+  while( printerInx < numPrinters ) {
>     LPTSTR name = GlobalPrinters::GetInstance()->GetItemFromList(printerInx++);
>-    nsAutoString newName; 
>-    NS_CopyNativeToUnicode(nsDependentCString(name), newName);
>-    PRUnichar *str = ToNewUnicode(newName);
>-    if (!str) {
>-      CleanupArray(array, count);
>-      return NS_ERROR_OUT_OF_MEMORY;
>-    }
>-    array[count++] = str;
>+    printers->AppendString(NS_ConvertUTF8toUTF16(name));
>   }

This new code is almost certainly wrong.  Windows doesn't use UTF8 for anything.

Comment 2

[Kenneth Herron]

Attachment: patch v2

You're right. I based that on the beos version of NS_CopyNativeToUnicode() without realizing that each platform had its own implementation of that function. It looks like there's no substitute for calling NS_CopyNativeToUnicode() here.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 262237 [details] [diff] [review]
patch v2

> The current nsPrintSettings factory always sets a printer, so
>this function is unnecessary and I've removed it.

I think you're wrong. It seems you're referring to this code:
http://lxr.mozilla.org/seamonkey/source/widget/src/xpwidgets/nsPrintOptionsImpl.cpp#909

However, a few platforms have their own implementation of that function:
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsPrintOptionsWin.cpp#66
That one doesn't seem to set the printer name.

toolkit/components/printing/content/printdialog.js

In getPrinters:
I'd prefer a codestyle like this (I hope bugzilla won't wrap it; the dots are supposed to be aligned with each other)

+    printerEnumerator = Components.classes["@mozilla.org/gfx/printerenumerator;1"];
+                                  .getService(Components.interfaces.nsIPrinterEnumerator)
+                                  .printerNameList;

This also leads to slightly shorter lines.

(also in xpfe/global/resources/content/printdialog.js)

+++ widget/src/beos/nsDeviceContextSpecB.cpp	20 Apr 2007 11:15:46 -0000

+  NS_ENSURE_ARG_POINTER(aPrinterNameList);

I see no point in nullchecking out parameters... (but then, this code used to do that. oh well.)

In the Xlib code:
   *aCount = count;
   *aResult = array;

you need to remove those :)

Looks good otherwise.

Comment 4

[Kenneth Herron]

Attachment: Restore CheckForPrinters

I think this addresses your comments. I restored nsIPrintOptions::CheckForPrinters, but rewrote it in terms of the print settings service default printer.

The current version of CheckForPrinters would enumerate printers and fetch the first printer from the list before checking whether the print settings object already had a printer set. I've reversed the order of these actions. Also, the current version has special-case logic for MacOSX to return NS_OK if it can't enumerate printers. In fact, Mac just uses the system's print dialog and doesn't bother with printer names, so I just made the whole function a no-op there.

Comment 5

[Kenneth Herron]

Comment on attachment 262958 [details] [diff] [review]
Restore CheckForPrinters

nsDeviceContextSpecXlib.cpp was removed from the tree, so you can ignore that part of the patch.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 262958 [details] [diff] [review]
Restore CheckForPrinters

layout/printing/nsPrintEngine.cpp
+#if defined(XP_MAC) || defined(XP_MACOSX)
+    // Mac doesn't support retrieving a printer list.
+    return NS_OK;

this should be indented only two spaces

I'd use an nsXPIDLString and getter_Copies here, instead of manually freeing the printer name.

in the two .js files:
+        Components.classes["@mozilla.org/gfx/printerenumerator;1"]
+        .getService(Components.interfaces.nsIPrinterEnumerator)

I meant putting the . from .getService under the dot from .classes

Comment 7

[Kenneth Herron]

Attachment: Patch for checkin

* Fixed the formatting issues.
* Rewrote CheckForPrinters using nsXPIDLString.
* Removed the Xlib section of the patch.

Comment 8

[Kenneth Herron]

Checked in.