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?

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

Investigating

What problem will be caused if mPaperName is null?

Hmmm, this is a question for Roland

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) ...).

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") ... ;-((

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

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@@@!!!... ;-(

Taking myself...

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...

> Can you verify that this problem does not occur for normal printing ? ya, normal printing is fine

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 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 rods@netscape.com 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.

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

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

rods wrote: > I am going to apply this patch and see what happens on Windows. Any results from that test ?

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

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

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

Comment on attachment 103716 [details] [diff] [review] patch to printing.js bring the r=/sr= forward

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.

taking bug....

ok, really taking

Comment on attachment 103720 [details] [diff] [review] patch v4 r=dcone

Comment on attachment 103720 [details] [diff] [review] patch v4 backing off r.. it was the wrong patch.

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

Comment on attachment 103721 [details] [diff] [review] patch v5 r=dcone

Comment on attachment 103721 [details] [diff] [review] patch v5 sr=kin@netscape.com, 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)

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 on attachment 104407 [details] [diff] [review] patch v6 a=roc+moz for trunk

fixed