Closed Bug 303091 Opened 19 years ago Closed 19 years ago

JS Exception when changing print scale

Categories

(Core :: Printing: Output, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mcsmurf, Assigned: bastiaan)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
Assignee: printing → b.jacques
Flags: blocking1.8b4+
This is related to bug 302495
Attached patch fix (obsolete) — Splinter Review
Attached patch the right patch (obsolete) — Splinter Review
Attachment #191431 - Attachment is obsolete: true
Attachment #191433 - Flags: review?(timeless)
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?  ;)
Attached patch fix 2 (obsolete) — Splinter Review
(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! :-)
Attachment #191433 - Attachment is obsolete: true
Attachment #191602 - Flags: review?(timeless)
Attachment #191433 - Flags: review?(timeless)
Attached patch fix 2.1Splinter Review
timeless Suggested on IRC that I use a different NS_ENSURE macro.
Attachment #191602 - Attachment is obsolete: true
Attachment #191605 - Flags: superreview?(dmose)
Attachment #191605 - Flags: review?(timeless)
Attachment #191602 - Flags: review?(timeless)
Attachment #191605 - Flags: review?(timeless) → review+
Blocks: branching1.8
Whiteboard: patch; r+; awaiting supperreview
Whiteboard: patch; r+; awaiting supperreview → [1.8 Branch ETA=8/9] patch; r+; awaiting supperreview
Comment on attachment 191605 [details] [diff] [review]
fix 2.1

sr=shaver
Attachment #191605 - Flags: superreview?(dmose)
Attachment #191605 - Flags: superreview+
Attachment #191605 - Flags: approval1.8b4+
Checked in by timeless (2005-08-08 18:35).
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [1.8 Branch ETA=8/9] patch; r+; awaiting supperreview
You need to log in before you can comment on or make changes to this bug.