167894 - InitPrintSettingsFromPrinter() not called for Print Preview

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Andrew Schultz]

current (2002091016) CVS trunk, linux
Valgrind says complains about uninitialized mPaperName:
Conditional jump or move depends on uninitialised value(s)
 at PL_strcasecmp (strccmp.c:82)
 by paper_name_to_PSPaperSizeRec(char const *) (nsPostScriptObj.cpp:294)
 by nsPostScriptObj::Init(nsIDeviceContextSpecPS *) (nsPostScriptObj.cpp:339)
 by nsDeviceContextPS::SetSpec(nsIDeviceContextSpec *) (nsDeviceContextPS.cpp:131)

The real problem appears to be that 
nsPrinterEnumeratorGTK::InitPrintSettingsFromPrinter() is never called (using
NSPR_LOGging).  a build from 20020905 called this function and got the paper
name fine.

regression from bug 166217?

Comment 1

[Andrew Schultz]

steps to reproduce:
1. setenv NSPR_LOG_MODULES DeviceContextSpecGTK:5
2. setenv NSPR_LOG_FILE something.log
3. print preview in Mozilla

InitPrintSettingsFromPrinter() is not called and papername is "<NULL>" or maybe
garbage.  this also worked in a binary .mozilla.org build from 2002090905

Comment 2

[Pete Zha]

Investigating

Comment 3

[Pete Zha]

What problem will be caused if mPaperName is null?

Comment 4

[rods (gone)]

Hmmm, this is a question for Roland

Comment 5

[Roland Mainz]

Pete Zha wrote:
> What problem will be caused if mPaperName is null?

The Xprint module won't be able to find a matching paper name and will return an
error (the PostScript module still looks at the paper size values if there is no
matching paper with that name (BTW: this code can be removed, I only added it
for compatibility reasons long ago to avoid an additional bug dependicy) ...).

Comment 6

[Roland Mainz]

Andrew Schultz wrote:
> steps to reproduce:
> 1. setenv NSPR_LOG_MODULES DeviceContextSpecGTK:5
> 2. setenv NSPR_LOG_FILE something.log
> 3. print preview in Mozilla

Oh, we're talking about Print Preview...
... I guess we forgot to fix that part in bug 166217 ("Print settings on Linux
are saved at shutdown but not read at next start") ... ;-((

Comment 7

[Roland Mainz]

Andrew Schultz:
Can you verify that this problem does not occur for normal printing ?

Comment 8

[Roland Mainz]

OK, I can verify that Print Preview doesn't work correctly with Xprint printers
anymore - the "fun" is that we still switch into PP mode even if the
nsIDeviceContext for thr print module returns an error.
x@@@!!!... ;-(

Comment 9

[Roland Mainz]

Taking myself...

Comment 10

[Roland Mainz]

Attachment: Patch for 2002-08-30-08-trunk with bug_166217_attachment_98312 applied

Comment 11

[Roland Mainz]

Per comment #5:
Filed bug 168043 ("Remove some obsolete code from the PostScript and Xprint
print modules") to get the obsolete "find paper by size"-code in
mozilla/gfx/src/ps/nsPostScriptObj.cpp removed...

Comment 12

[Andrew Schultz]

> Can you verify that this problem does not occur for normal printing ?

ya, normal printing is fine

Comment 13

[Pete Zha]

Comment on attachment 98758 [details] [diff] [review]
Patch for 2002-08-30-08-trunk with bug_166217_attachment_98312 applied

I have tested this patch with attachment 98760 [details] [diff] [review] of bug 168043. It works well and
can fix this bug. 
r=pete
bz, can you sr it? Thanks!

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 98758 [details] [diff] [review]
Patch for 2002-08-30-08-trunk with bug_166217_attachment_98312 applied

+var gPrintService = null;

I see no need for a global variable that holds on to the print service,
especially not with the way the rest of this code is written. Or do we for some
unknown reason need to hold on to the print service longer now than we used to?

-      var psService =
Components.classes["@mozilla.org/gfx/printsettings-service;1"]
+      gPrintService =
Components.classes["@mozilla.org/gfx/printsettings-service;1"]
					
.getService(Components.interfaces.nsIPrintSettingsService);
       if (gPrintSettingsAreGlobal) {
-	 gPrintSettings = psService.globalPrintSettings;	
-	 if (gSavePrintSettings) {
-	   psService.initPrintSettingsFromPrefs(gPrintSettings, false,
gPrintSettings.kInitSaveNativeData);
-	 }
+	 gPrintSettings = gPrintService.globalPrintSettings;
+	 
+	 setPrinterDefaultsForSelectedPrinter();
       } else {
-	 gPrintSettings = psService.newPrintSettings;
+	 gPrintSettings = gPrintService.newPrintSettings;

Change the above back to using the local psService variable and pass that as an
argument to setPrinterDefaultsForSelectedPrinter() in stead of using the
global.

I'm not all that excited about landing this code anywhere, I'd feel much better
about this if [e-mail] would have a look at this before it's checked
in.

If this *must* land tonight to get enough testing before some release date I'm
unaware of, then sure, sr=jst, but I'd still recommend that rods has a look at
this, post-checkin if not before.

Comment 15

[Pete Zha]

OK, let rods look at it. I will upload a patch with jst's comments later. Thanks
jst!

Comment 16

[Pete Zha]

Attachment: patch with jst's comments

Comment 17

[rods (gone)]

I am going to apply this patch and see what happens on Windows.

Comment 18

[Roland Mainz]

rods wrote:
> I am going to apply this patch and see what happens on Windows.

Any results from that test ?

Comment 19

[Andrew Schultz]

*** Bug 175825 has been marked as a duplicate of this bug. ***

Comment 20

[rods (gone)]

Comment on attachment 98875 [details] [diff] [review]
patch with jst's comments

Now, that I understand all the ramifications of this and the other patch. PLus,
Bug 175828 will fix the PP issues on Windows.
r=rods

Comment 21

[rods (gone)]

Attachment: patch to printing.js

The js code has been moved to printing.js this is a hand merge of that patch.

Comment 22

[rods (gone)]

Comment on attachment 103716 [details] [diff] [review]
patch to printing.js

bring the r=/sr= forward

Comment 23

[rods (gone)]

Attachment: patch v4

I have incorporated the the fixes from bug 175828, because this 175828 was
caused by a previous checkin directly related to this patch.

The printing.js portion of this patch has already been reviewed and sr'ed.

So this new patch also:
1) Adds two bools to the PrintSettings and then when the PS gets
initialized from Prefs or from the Printer they get set. They get unset when
the printer name changes.

This is all necessary because the Page Setup Dialog calls
initPrintSettingsFromPrinter each time it is shown. So if you have set values
they will get "written" over the next time the dialog is shown.

2) Removes a lot of platform specific initialization from nsPrintOptionsImpl
and it now uses just the the "generic" XP routines and is now consistent with
the other platforms for initializing the PS.

Comment 24

[rods (gone)]

taking bug....

Comment 25

[rods (gone)]

ok, really taking

Comment 26

[dcone (gone)]

Comment on attachment 103720 [details] [diff] [review]
patch v4

r=dcone

Comment 27

[dcone (gone)]

Comment on attachment 103720 [details] [diff] [review]
patch v4

backing off r.. it was the wrong patch.

Comment 28

[rods (gone)]

Attachment: patch v5

patch v4 is the wrong patch, this is the correct patch

Comment 29

[dcone (gone)]

Comment on attachment 103721 [details] [diff] [review]
patch v5

r=dcone

Comment 30

[kinmoz]

Comment on attachment 103721 [details] [diff] [review]
patch v5

sr=[e-mail], just address the following:


==== Remove the blank line before the brace.


+	     if (NS_SUCCEEDED(rv)) {
+		 aPrintSettings->SetIsInitializedFromPrinter(PR_TRUE);

+	     }


==== Put a return in between these 2 lines:


+    }
+    return NS_OK;


==== You can check |aPrinter| once if you reorder your expression to look like
this |if (!aPrinter || !mPrinter.Equals(aPrinter))|:


+  if ((aPrinter && !mPrinter.Equals(aPrinter)) || !aPrinter) {


==== Should these PRBools be PRPackedBools?


   PRBool	 mPrintToFile;
   nsString	 mToFileName;
+  PRBool	 mIsInitedFromPrinter;
+  PRBool	 mIsInitedFromPrefs;



==== Why the rename with a leading underscore? I seem to remember other
reviewers discouraging the use of leading underscores.


-NS_IMETHODIMP nsPrintOptionsWin::CreatePrintSettings(nsIPrintSettings
**_retval)
+nsresult nsPrintOptionsWin::_CreatePrintSettings(nsIPrintSettings **_retval)

Comment 31

[rods (gone)]

Attachment: patch v6

new patch with Kin's suggestions, as far as the underscaores go, those were put
in a while back and now would be a bad time to change them.

Comment 32

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

Comment on attachment 104407 [details] [diff] [review]
patch v6

a=roc+moz for trunk

Comment 33

[rods (gone)]

fixed