When printing, there are two different margins used. One is the normal margin, settable by nsIPrintSettings.margin* that defines the margins around the page content. The 2nd margin is the margins used to place the headers/footers on the page. That margin is not settable via nsIPrintSettings; instead nsSimplePageSequenceFrame takes it directly from the print.printer_<name>.print_edge_* prefs. That's not very friendly to use for embedders. So I propose to allow setting this via nsIPrintSettings too.

Comment on attachment 281005 [details] [diff] [review] proposed patch Trying a different reviewer... hope that's alright. And also adding additional review req from roc for the layout/generic/ changes.

attribute double marginTop; /* these are in inches */ attribute double marginLeft; attribute double marginBottom; attribute double marginRight; + attribute double edgeTop; /* these are in inches */ + attribute double edgeLeft; + attribute double edgeBottom; + attribute double edgeRight; Document the meanings/differences? Why do we need *InchesIntToTwipsPref? Can't we use *InchesToTwipsPref?

Comment on attachment 281005 [details] [diff] [review] proposed patch Hrm, I'm not the right person to review this -- I know nothing about printing.

I can do all the reviews here (once my question is answered).

(In reply to comment #3) > attribute double marginTop; /* these are in inches */ > attribute double marginLeft; > attribute double marginBottom; > attribute double marginRight; > > + attribute double edgeTop; /* these are in inches */ > + attribute double edgeLeft; > + attribute double edgeBottom; > + attribute double edgeRight; > > Document the meanings/differences? How about this? /** * The edge defines the area at the border of the paper * outside of which the printer cannot print. The headers and footers * will be placed inside. */ /** * The margins define the area of the paper to print the content to. * The headers and footers are placed outside, but still inside the border * defined by the edge attributes above. */ > Why do we need *InchesIntToTwipsPref? Can't we use *InchesToTwipsPref? The print_edge_* prefs are int prefs (in 1/100th's of inches), while ReadInchesToTwipsPref reads a string (getCharPref) and converts the string to float. And it's impossible to use getCharPref on a int pref (it throws). So either I need a variant that uses getIntPref, or I'd have to add new prefs and migration code for the old prefs; the former seemed easier to me.

I forgot to add the changes in toolkit/components/printing/printjoboptions.js and printdialog.js in the patches aboves. (I also patched their copies in xpfe/global/resources/content/; not sure if that is necessary.) Whom should I ask for r? on this part?

Comment on attachment 281005 [details] [diff] [review] proposed patch Minusing cause I'm not sure if this is the right patch for checkin. Please re-nominate whatever it is you want to check in.

Let's get gavin's review and then roll the two patches together for approval.

Comment on attachment 284842 [details] [diff] [review] additional patch Sorry for the delay, this fell off my radar.

Christian, can you roll this up and submit for approval? Thanks

This is the combined patch of the 2 patches above, for checkin. It has r+sr=roc on the core bits, and r=gavin on the toolkit/xpfe bits. The patch implements a useful feature for embedders, so they can control all aspects of printing with nsIPrintSettings instead of also needing to set some prefs. The risk is low; and I had debug bits in my tree that verified that all values passed to the print engine (nsSimplePageSequence) were the same as before.

Comment on attachment 288135 [details] [diff] [review] combined patch for checkin a+

Checking in widget/public/nsIPrintSettings.idl; /cvsroot/mozilla/widget/public/nsIPrintSettings.idl,v <-- nsIPrintSettings.idl new revision: 1.41; previous revision: 1.40 done Checking in widget/src/xpwidgets/nsPrintOptionsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintOptionsImpl.cpp,v <-- nsPrintOptionsImpl.cpp new revision: 1.94; previous revision: 1.93 done Checking in widget/src/xpwidgets/nsPrintOptionsImpl.h; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintOptionsImpl.h,v <-- nsPrintOptionsImpl.h new revision: 1.42; previous revision: 1.41 done Checking in widget/src/xpwidgets/nsPrintSettingsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.cpp,v <-- nsPrintSettingsImpl.cpp new revision: 1.38; previous revision: 1.37 done Checking in widget/src/xpwidgets/nsPrintSettingsImpl.h; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.h,v <-- nsPrintSettingsImpl.h new revision: 1.30; previous revision: 1.29 done Checking in layout/generic/nsSimplePageSequence.cpp; /cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v <-- nsSimplePageSequence.cpp new revision: 3.158; previous revision: 3.157 done Checking in layout/generic/nsSimplePageSequence.h; /cvsroot/mozilla/layout/generic/nsSimplePageSequence.h,v <-- nsSimplePageSequence.h new revision: 3.57; previous revision: 3.56 done Checking in toolkit/components/printing/content/printdialog.js; /cvsroot/mozilla/toolkit/components/printing/content/printdialog.js,v <-- printdialog.js new revision: 1.16; previous revision: 1.15 done Checking in toolkit/components/printing/content/printjoboptions.js; /cvsroot/mozilla/toolkit/components/printing/content/printjoboptions.js,v <-- printjoboptions.js new revision: 1.15; previous revision: 1.14 done Checking in xpfe/global/resources/content/printdialog.js; /cvsroot/mozilla/xpfe/global/resources/content/printdialog.js,v <-- printdialog.js new revision: 1.38; previous revision: 1.37 done Checking in xpfe/global/resources/content/unix/printjoboptions.js; /cvsroot/mozilla/xpfe/global/resources/content/unix/printjoboptions.js,v <-- printjoboptions.js new revision: 1.24; previous revision: 1.23 done

Did this cause reftest failures? REFTEST UNEXPECTED FAIL (LOADING): file:///C:/slave/trunk/mozilla/layout/reftests/printing/blank.html REFTEST UNEXPECTED FAIL (LOADING): file:///C:/slave/trunk/mozilla/layout/reftests/printing/272830-1.html REFTEST UNEXPECTED FAIL (LOADING): file:///C:/slave/trunk/mozilla/layout/reftests/pagination/blank.html REFTEST UNEXPECTED FAIL (LOADING): file:///C:/slave/trunk/mozilla/layout/reftests/pagination/abspos-overflow-01.xhtml

(In reply to comment #16) > Did this cause reftest failures? This might be bug 388169. I had a heck of a time checking in an earlier patch that modified nsIPrintSettings.

(In reply to comment #17) > (In reply to comment #16) > > Did this cause reftest failures? > > This might be bug 388169. I had a heck of a time checking in an earlier patch > that modified nsIPrintSettings. > The patch was backed out just before this comment. Windows tboxes are green again.

You should be able to check in the patch if you clobber win32. But note the boxes that are finicky about nsIPrintSettings are also finicky about clobbers. See bug 393808 and bug 393852.

Relanded. Checking in widget/public/nsIPrintSettings.idl; /cvsroot/mozilla/widget/public/nsIPrintSettings.idl,v <-- nsIPrintSettings.idl new revision: 1.43; previous revision: 1.42 done Checking in widget/src/xpwidgets/nsPrintOptionsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintOptionsImpl.cpp,v <-- nsPrintOptionsImpl.cpp new revision: 1.96; previous revision: 1.95 done Checking in widget/src/xpwidgets/nsPrintOptionsImpl.h; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintOptionsImpl.h,v <-- nsPrintOptionsImpl.h new revision: 1.44; previous revision: 1.43 done Checking in widget/src/xpwidgets/nsPrintSettingsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.cpp,v <-- nsPrintSettingsImpl.cpp new revision: 1.40; previous revision: 1.39 done Checking in widget/src/xpwidgets/nsPrintSettingsImpl.h; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.h,v <-- nsPrintSettingsImpl.h new revision: 1.32; previous revision: 1.31 done Checking in layout/generic/nsSimplePageSequence.cpp; /cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v <-- nsSimplePageSequence.cpp new revision: 3.160; previous revision: 3.159 done Checking in layout/generic/nsSimplePageSequence.h; /cvsroot/mozilla/layout/generic/nsSimplePageSequence.h,v <-- nsSimplePageSequence.h new revision: 3.59; previous revision: 3.58 done Checking in toolkit/components/printing/content/printdialog.js; /cvsroot/mozilla/toolkit/components/printing/content/printdialog.js,v <-- printdialog.js new revision: 1.18; previous revision: 1.17 done Checking in toolkit/components/printing/content/printjoboptions.js; /cvsroot/mozilla/toolkit/components/printing/content/printjoboptions.js,v <-- printjoboptions.js new revision: 1.17; previous revision: 1.16 done Checking in xpfe/global/resources/content/printdialog.js; /cvsroot/mozilla/xpfe/global/resources/content/printdialog.js,v <-- printdialog.js new revision: 1.40; previous revision: 1.39 done Checking in xpfe/global/resources/content/unix/printjoboptions.js; /cvsroot/mozilla/xpfe/global/resources/content/unix/printjoboptions.js,v <-- printjoboptions.js new revision: 1.26; previous revision: 1.25 done

I think there is a problem with this patch, and it looks like with some of the old code too. There are missing twips to app unit conversions. I've attached a patch, but it has some cruft from my tree - I'm sorry but I don't have time to clean it up right now. I think this is all the places this is true, but you might want to make some more checks. Particularly, the Getter/Setter functions for the twips appear to be taking nscoord's not PRInt32's. Regards, -Jeremy

Ah yes. Christian, would you mind cleaning up this patch so I can review it?

Sure. Since it's not a problem introduced by the original patch (which just took the values from settings instead of prefs), I'll file a new bug for it though.