When you change the scaling in Print Preview (at least in FF, SeaMonkey seems to work fine) you get this exception: Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPrintSettingsService.savePrintSettingsToPrefs]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://global/content/printUtils.js :: anonymous :: line 86" data: no] Bug 297277 added a NS_ENSURE_FALSE(prtName.IsEmpty(), NS_ERROR_FAILURE); check. This check always fails if aUsePrinterNamePrefix is set to false because then GetAdjustedPrinterName always returns null.

As I said on irc, it seems to me that the function can legally return empty even when aUsePrinterNamePrefix is null. If people are trying to catch OOM, then they should make GetAdjustedPrinterName return nsresult.

This is related to bug 302495

So why is this changing the behavior? With this patch, if aUsePrinterNamePrefix is false nothing will be saved. Which seems odd to me.

The behaviour didn't change, except that we're not writing prefs with an empty prtName when we're writing printer specific prefs. This sounds logical to me..

With your patch, SavePrintSettingsToPrefs does nothing if aUsePrinterNamePrefix is false. Are you saying this is the right behavior? In that case, why does the method even exist and have such an argument? That makes no sense. Unless you've done extensive testing (and it's clear that you have not, given the print preview regression the patch to bug 297277 caused), please restore the logic flow to what it was before the bug 297277 patch landed and feel free to try to rewrite all this code in 1.9. ccing reviewers from bug 297277 -- please look over that patch again for unintended logic changes and make sure they all come out of the tree for 1.8, ok?

(In reply to comment #7) > With your patch, SavePrintSettingsToPrefs does nothing if aUsePrinterNamePrefix > is false. Are you saying this is the right behavior? To be honest, I'm not sure if it is. But I do think it was the original author's intention. > In that case, why does > the method even exist and have such an argument? That makes no sense. You'd have to ask the original author :) > Unless you've done extensive testing (and it's clear that you have not, given > the print preview regression the patch to bug 297277 caused), please restore the > logic flow to what it was before the bug 297277 patch landed and feel free to > try to rewrite all this code in 1.9. I can do that, but that would make us write the prefs with an empty prtName. Are you sure that we want to do that? To reiterate, the logic flow with the attached patch is the same as it was before the patch to bug 297277, apart from the check for an empty prtName.

> But I do think it was the original author's intention. Couldn't have been. If it were, the original author would have simply not called the method in cases when saving settings was not desired instead of having a method call with a special argument that means "oh, but don't actually do anything". > I can do that, but that would make us write the prefs with an empty prtName. If that's what we were doing before your patches, then that's what we should continue to do. Again, unless we have firm evidence that it's the wrong thing to do and have carefully tested the change to make sure nothing breaks. > apart from the check for an empty prtName. So the method does the same as it used to except in all these cases when it no longer does anything instead of what it used to do, right? ;)

(In reply to comment #9) > Couldn't have been. If it were, the original author would have simply not > called the method in cases when saving settings was not desired instead of > having a method call with a special argument that means "oh, but don't actually > do anything". At the same time, you would think that the author wouldn't have added a comment about printer specific prefs, if he intended to write the prefs even with an empty printer name. But I guess we'll never know what exactly the author meant, and I suppose continuing to guess what he did mean is rather unproductive. So, I'll restore the original behaviour, and delay the changes until 1.9, as you requested. > So the method does the same as it used to except in all these cases when it no > longer does anything instead of what it used to do, right? ;) Exactly! :-)

timeless Suggested on IRC that I use a different NS_ENSURE macro.

Comment on attachment 191605 [details] [diff] [review] fix 2.1 sr=shaver

Checked in by timeless (2005-08-08 18:35).