117233 - Implement nsIPrinterEnumerator for nsDeviceContextSpecX.mm

Status: RESOLVED FIXED

(Core :: Printing: Output, task)

Description

[rods (gone)]

Mac needs to implement the Printer Enumerator so we can tell if any printers
have been installed on the system.

Then once implemented, you need to uncomment out the ShowPrintErrorDialog call
in CheckForPrinters.

Comment 1

[dcone (gone)]

I don't think the Mac has this kind of feature.  The printer is selected on the 
desktop.. and controlled thru mac print facilities.

Comment 2

[Vedran Miletic]

retargeting

Comment 3

[louis bennett]

isn't this an os-supplied feature?

Comment 4

[Tony Goold]

Attachment: An implementation of the Printer Enumerator

This will break the Mac OS X nsPrintingPromptService implementation without the
associated bug fix submitted for bug #290214 (see details there), but it
appears to work once that patch has been applied.

Comment 5

[Tony Goold]

Attachment: Amended patch: Fixes issue with PMSessionCreatePrinterList

I recently discovered PMSessionCreatePrinterList will null out the array
reference it is passed if there are no printers, causing the subsequent
CFArrayGetCount call to crash. This patch amends the previous one to handle
this situation.

Comment 6

[Mike Pinkerton (not reading bugmail)]

nit. when calling OS global routines, we use the global scope operator so they
don't conflict with class methods of the same name, eg:

+    CFMutableArrayRef printerList = CFArrayCreateMutable(kCFAllocatorDefault,
+        0, &kCFTypeArrayCallBacks);

should be ::CFArrayCreateMutable(...) instead. 

+    CFRetain(printerName);
...
+    CFRelease(printerName);

Why are you retaining printerName? can it actually go away while you're running
on the main thread between these statements?

+    status = ::PMSessionDefaultPageFormat(mSession, pageFormat);
+    NS_ENSURE_TRUE(status == kPMNoError, NS_ERROR_UNEXPECTED);

this will leak |pageFormat| if there's an error.

+    status = ::PMSessionDefaultPrintSettings(mSession, settings);
+    NS_ENSURE_TRUE(status == kPMNoError, NS_ERROR_UNEXPECTED);

this will leak |settings| if there's an error.

+    OSStatus status = PMSessionCreatePrinterList(mSession, (CFArrayRef*)
&printerList,
+        &currentIndex, &currentPrinter);
+    NS_ENSURE_TRUE(status == kPMNoError, NS_ERROR_FAILURE);

this will leak |printerList| if there's an error, as will most of the
NS_ENSURE_TRUE's in this routine. That's the main reason i really don't like
NS_ENSURE* as it will leak anything with manual refcounting.

+private:
+    PMPrintSession    mSession;

mention with a comment that |mSession| is refcounted and a strong (owned) reference.

+#if TARGET_CARBON

didn't we remove all the TARGET_CARBON's from the tree (or we're trying to)? We
don't build on os9 any more.

Comment 7

[Tony Goold]

Attachment: Response to review comments

(In reply to comment #6)
> +    CFRetain(printerName);
> ...
> +    CFRelease(printerName);
> 
> Why are you retaining printerName? can it actually go away while you're
running
> on the main thread between these statements?

Slight oversight on my part: When printerList is released, printerName is sent
a release message as a result. I've now deferred the release of printerList
until after I'm done with printerName instead of retaining/releasing
printerName.

> +#if TARGET_CARBON
> 
> didn't we remove all the TARGET_CARBON's from the tree (or we're trying to)?
We
> don't build on os9 any more.

I wasn't sure, since nsGfxFactoryMac.cpp still contained other #if
TARGET_CARBON statements, but I've removed my own if guards and taken the
liberty of making the other TARGET_CARBON blocks unconditional as well.

Regarding the other issues (NS_ENSURE_TRUE related leaks, use of global scope
operator, and including a comment for mSession), I believe they've been fixed
as per your comments.

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 182691 [details] [diff] [review]
Response to review comments

looks good, would like to see more comments though for sr

r=pink.

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 182691 [details] [diff] [review]
Response to review comments

looks good, would like to see more comments though for sr

r=pink.

Comment 10

[Tony Goold]

Attachment: Same patch with additional comments

Added comments to nsPrinterEnumeratorX methods and moved clean up code for
errors outside the for loop in nsPrinterEnumeratorX::EnumeratePrinters.

Comment 11

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 184855 [details] [diff] [review]
Same patch with additional comments

+NS_IMETHODIMP nsPrinterEnumeratorX::EnumeratePrinters(PRUint32* aCount,
PRUnichar*** aResult)

I'm assuming that callers already know (and it's documented in the interface)
that they are responsible for deleting the 2d array from |aResult|? 

Thanks for the comments, *much* easier to follow.

r=pink

Comment 12

[Tony Goold]

Interface documentation is lacking, but this is consistent with other platform implementations and the 
nsPrinterListEnumerator class defined in mozilla/gfx/src/nsPrintOptionsImpl.cpp, which invokes 
EnumeratePrinters in its designated initializer, frees the 2d array in its destructor.

(In reply to comment #11)
> I'm assuming that callers already know (and it's documented in the interface)
> that they are responsible for deleting the 2d array from |aResult|? 

Comment 13

[Simon Fraser [no longer active]]

Can you explain when this interface would get used (since our print dialogs are native)?

Comment 14

[Tony Goold]

I've only used it in one instance, where I wanted to display a page count for the web browser document and I used the nsIWebBrowserPrint interface to do a print preview and get the number of pages. As far as I know, there's no way to do a "stealth" print preview using the native printing functions. Anyway, I found print preview was failing for lack of an nsIPrinterEnumerator implementation so I implemented it. My use for it is not directly related to anything for Firefox, but I saw the Bugzilla entry so I figured I would submit my patch.

(In reply to comment #13)
> Can you explain when this interface would get used (since our print dialogs are
> native)?

Comment 15

[Simon Fraser [no longer active]]

Comment on attachment 184855 [details] [diff] [review]
Same patch with additional comments

Clearing old request

Comment 16

[Matthias Versen [:Matti]]

Tony Goold: is this patch still wanted ? In that case we would have to find a new reviewer...

Comment 17

[Tony Goold]

I'd like to see this patched, since it causes printUtils.js to throw exceptions needlessly whenever it queries the default printer name, resulting in noise on the console. There's not really any functionality lost though, so I might be battling uphill on this one.

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 184855 [details] [diff] [review]
Same patch with additional comments

There is no API change here, so per the new sr policy this doesn't need sr.

Comment 19

[Jonathan Watt [:jwatt]]

This makes some of the nsPrintSettingsService methods no-ops (nsPrintSettingsService::GetDefaultPrinterName and nsPrintSettingsService::InitPrintSettingsFromPrinter), and may also impact nsPrintDialogUtil::ShowNativePrintDialog.

nsPrintSettingsService::GetDefaultPrinterName throwing causes _getDefaultPrinterName to return null in printUtils.js

Comment 20

[Jonathan Watt [:jwatt]]

We're going to need this for the new printing UI since it needs to be able to populate the settings sidebar based on the available printers and their valid settings.

Comment 23

[Jonathan Watt [:jwatt]]

Seems relevant (specifically printerNames as an entry point?):
https://developer.apple.com/documentation/appkit/nsprinter?language=objc

Comment 24

[Erik Nordin [:nordzilla]]

Attachment: Bug 117233 - Part 1 Add GlobalPrinters Class For macOS r=jwatt,alaskanemily

GlobalPrinters is a helper class to help share data between
nsDeviceContextSpec and PrinterEnumerator, and to interface
with the operating system.

Comment 25

[Erik Nordin [:nordzilla]]

Attachment: Bug 117233 - Part 2 Implement nsIPrinterEnumeratorX r=jwatt,alaskanemily

• Implement the printer enumerator for macOS
• Enable the contract @mozilla.org/gfx/printerenumerator;1 for macOS

Depends on D76198

Comment 26

[Erik Nordin [:nordzilla]]

Attachment: Bug 117233 - Implement nsIPrinterEnumeratorX r=jwatt,alaskanemily

• Implement GlobalPrinters helper class for nsDeviceContextSpecX
• Implement the nsPrinterEnumeratorX
• Enable the contract @mozilla.org/gfx/printerenumerator;1 for macOS
• Add test for default printer name.
• Remove restrictions preventing some tests from running on macOS

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/faf8e8097d23
Implement nsIPrinterEnumeratorX r=jwatt

Comment 28

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/faf8e8097d23