Tested using this build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008021104 Minefield/3.0b4pre Basically, we're not taking margins into account when using the default paper size. However, when the user manually chooses a paper size, we *do* apply the margins from there on out, even if the user-selected size is the same as the default. Steps to reproduce: 1. Start a Linux trunk build using a fresh profile, viewing Google.com 2. Open Print-Preview. (alt-f, v) ** OBSERVE: No margins present.** (header/footer are right against page bounds) 3. Close Print-Preview 4. Open Page-Setup dialog. (alt-f, u) 5. Click the "Paper size" dropdown list, and select "US Letter" at the top of the list. (Note: US Letter is already set as the default value) 6. Click "Apply", and then start another print-preview ** OBSERVE: Now there are margins.** (.25in on top/sides, and .5in on bottom, I think) After step 6, we seem to correctly print margins, even after closing & reopening Firefox. The only way I've found to revert to the buggy no-margins state is to use a fresh profile.

NOTE: So far when I've said "margins" in this bug, I'm talking about what's also called "Edge" -- the paper edge margins, NOT the content margins. i.e. I'm talking about the distance from the edge of the paper to the header/footer. (In some areas of code, "margin" is actually distance between header/footer boundaries and the page content area, whereas "edge" is what I'm actually talking about here in this bug -- the distance from paper edge to imageable area. One such area is nsSimplePageSequenceFrame::Reflow -- the calls to mPageData->mPrintSettings->GetMarginInTwips and mPageData->mPrintSettings->GetEdgeInTwips have these meanings.) Anyway, having said that -- when it appears there are no paper-margins, it's because we're falling back to a default edge value of 1/100 of an inch. This is set as '4' in about:config, which translates 1/100 of an inch, or about 58 twips, I believe. Reference: http://mxr.mozilla.org/seamonkey/source/modules/libpref/src/init/all.js#2097 2041 #ifdef XP_UNIX ... 2094 // Enables you to specify the gap from the edge of the paper to the margin 2095 // this is used by both Printing and Print Preview 2096 pref("print.print_edge_top", 4); // 1/100 of an inch 2097 pref("print.print_edge_left", 4); // 1/100 of an inch 2098 pref("print.print_edge_right", 4); // 1/100 of an inch 2099 pref("print.print_edge_bottom", 4); // 1/100 of an inch You can also view these settings in about:config.

> (In reply to comment #1) > NOTE: So far when I've said "margins" in this bug, I'm talking about what's > also called "Edge" -- the paper edge margins, NOT the content margins. I take that back. AFAICT, "Edge" is NOT equivalent to Paper Margin. Moreover, the crux of this bug is that nsPrintSettingsGTK thinks they *are* equivalent, when they're really not. This assumption of equivalence seems to be the basis for our implementations of nsPrintSettingsGTK::SetEdgeInTwips and GetEdgeInTwips. [0] JUSTIFICATION FOR THEM NOT BEING EQUIVALENT: AFAICT, the print_edge_XXX prefs only let us tweak the header & footer positioning, but they don't move the actual page content. Case in point: If I set a very large value of print.print_edge_top (e.g. 6000) in about:config, and then I run a print-preview, I see that the **header** jumps halfway down the page, but the **page content** does not move. I verified this on both Linux and Mac OS X. Therefore, they're not behaving the same as paper margins should. HOW THE ASSUMPTION OF EQUIVALENCE IS CAUSING THE BUGGY BEHAVIOR: (as described in comment 0) a) User initiates a Print operation. b) We create a new GTKPageSetup, which starts out with Gnome's default page margins (0.25 on left/top/right, 0.56 on bottom) [1] c) In nsPrintOptions::ReadPrefs, we overwrite the GTKPageSetup's margins with values read from the "print.print_edge_XXX" prefs, via a call to SetEdgeInTwips. [2] ==> Result: Printed document has virtually no margins margins between paper borders and header/footer. d) If we manually select a paper-size in the File|Page Setup dialog, then the new paper size's *correct* margins are read via GetEdgeInTwips[3] and are written into the "print.print_edge_XXX" prefs, in nsPrintOptions::ritePrefs. ==> Result: Now when we print or print-preview, the margins appear correct, because the values that we end up reading from "print.print_edge_XXX" (in part (b) above) are the manually-selected paper size's margins. Code References: [0] http://mxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsPrintSettingsGTK.cpp#691 [1] http://mxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsPrintSettingsGTK.cpp#84 [2] http://mxr.mozilla.org/seamonkey/source/widget/src/xpwidgets/nsPrintOptionsImpl.cpp#284 [3] http://mxr.mozilla.org/seamonkey/source/widget/src/xpwidgets/nsPrintOptionsImpl.cpp#559

(In reply to comment #2) > d) If we manually select a paper-size in the File|Page Setup dialog, then the > new paper size's *correct* margins are read via GetEdgeInTwips[3] and are > written into the "print.print_edge_XXX" prefs, in nsPrintOptions::ritePrefs. er, I meant nsPrintOptions::WritePrefs. :)

How did we make a new default margin size before the new printing code?

(In reply to comment #4) > How did we make a new default margin size before the new printing code? I'm not sure which default margin size you're referring to. If you mean the default values of "print.print_edge_XXX", then according to CVS Blame on all.js, those have been in there since 2004-01-16. (though I don't know if they've always been honored) Link: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/modules/libpref/src/init/all.js&rev=3.731 Or if you're referring to the initial default margins in the newly-created GTKPageSetup, from part (b) of Comment 2, I think those come from the Gnome's default "US Letter" page layout. (At least, that's the default layout in the USA; not sure if it's different elsewhere :))

I meant the Gecko settings before we used GTK print, so yes that means all.js. For some reason OS X's margin settings are set to 0 in that file yet Unix's are set to 4. Can you modify all.js and change the Unix print edge prefs from 4 to 0? I want to see the effect of that and can't myself because I don't have a real printer right now.

(In reply to comment #6) > I meant the Gecko settings before we used GTK print, so yes that means all.js. > For some reason OS X's margin settings are set to 0 in that file yet Unix's are > set to 4. Yeah, I have no idea why it's 4 on Unix/Linux but 0 everywhere else. It should probably be 0, because I don't know any reason why it'd need to be different from other platforms, but I want to ask whoever it was that initially set it to 4 (I think bsmedberg?) to make sure. I'm guessing it probably had a purpose years ago but no longer does. > Can you modify all.js and change the Unix print edge prefs from 4 to > 0? I'll try that tomorrow morning, though I doubt it will have much effect -- it should just reduce the header's paper-edge-offset to zero, instead of being just barely larger than zero. I tried setting it to an absurdly large number earlier, and it just shifted the print header down. (see Comment 2, "case in point") > I want to see the effect of that and can't myself because I don't have a > real printer right now. You actually should be able to try it out without a printer, via: 1) tweaking the setting in about:config (easier than changing all.js) 2) firing up Print-Preview. (that pref also affects header placement in print preview -- it's not just for real printing.)

In FF2, we just allowed people to specify "Gap from edge of paper to margin" if they click "Properties" while printing. (see screenshot) And the values in those boxes map directly to print_edge_XXX values. See also http://forums.mozillazine.org/viewtopic.php?t=375630&sid=a9643402fcb36d67da56165b8019ea47 (a forum post about using them)

And here's the FF2 page-setup dialog, where users specify margins/etc.

Based on FF2 behavior, it looks like placement is supposed to be as follows on Linux (examining only top margin/edge, and with scale grossly exaggerated) ___________________________ ^ | | ^ | | | | (print_edge_top) | | | | | | | v | | | | HEADER GOES HERE | (top margin from page setup) | | | | | | | | | | | v |PAGE CONTENT BEGINS HERE | | ... | The point being: both "print_edge_top" and "top margin from page setup" are measured from the *top of the paper*. They're distinct and non-interacting measurements. AFAICT, the "Edge" settings are simply a user-customizable setting for positioning the header/footer, to prevent them from getting chopped off by being too close to the page edge. They don't affect the positioning of the content or the margins. So, in the new FF3 printing code, I don't think the gtk_page_setup_get_XXX_margin functions should be used inside Get/SetEdgeInTwips -- instead, they should be in Get/SetMarginInTwips.

(In reply to comment #12) > So, in the new FF3 printing code, I don't think the > gtk_page_setup_get_XXX_margin functions should be used inside > Get/SetEdgeInTwips -- instead, they should be in Get/SetMarginInTwips. ... because the GTK page setup margins map much more directly to our FF2 "page setup" margins (the measurement on the right in the dialog above), which is what Get/SetMarginInTwips control.

(In reply to comment #10) > In FF2, we just allowed people to specify "Gap from edge of paper to margin" if > they click "Properties" while printing. (see screenshot) And the values in > those boxes map directly to print_edge_XXX values. We should probably make the print_edge_XXX values configurable via the custom "Options" tab in FF3's print dialog, as they were in FF2 -- would you agree, Michael? (They might logically fit better on the "Page Setup" tab in that dialog, but my understanding from http://ventnorsblog.blogspot.com/2008/01/print-me-print-me-print-me-man-after.html is that the Options tab is the only Mozilla-specific / configurable one, and the rest of the tabs are running GTK code.)

Here's a preliminary patch. This "fixes" the issue as described in comment 0, by making us stick to the initial "OBSERVE" behavior, after Step 2 -- (header/footer are right against page bounds). This behavior remains after we change the paper size / margins -- the content is shifted to accommodate the new format, but the header/footer remain in the same spot. (matching FF2 behavior) I tried tweaking print_edge_XXX values, also, and that still works as it did in FF2. If I'm not missing anything, I think the only additional thing we'd want to add here is a GUI to let the user configure their print_edge_XXX values, as I suggested in comment 14.

(In reply to comment #14) > (In reply to comment #10) > > In FF2, we just allowed people to specify "Gap from edge of paper to margin" if > > they click "Properties" while printing. (see screenshot) And the values in > > those boxes map directly to print_edge_XXX values. > > We should probably make the print_edge_XXX values configurable via the custom > "Options" tab in FF3's print dialog, as they were in FF2 -- would you agree, > Michael? > > (They might logically fit better on the "Page Setup" tab in that dialog, but my > understanding from > http://ventnorsblog.blogspot.com/2008/01/print-me-print-me-print-me-man-after.html > is that the Options tab is the only Mozilla-specific / configurable one, and > the rest of the tabs are running GTK code.) > Correct. Margin options are available from the page setup dialog if you make a custom page size which is how it is done in GTK2. GTK2 provides default dmensions and margins, you just need to select a paper size or make your own.

Just talked with Michael on IRC a bunch -- bottom line, I'm gonna see how we handle this stuff on Mac and see how easy it'd be to tweak stuff to emulate that. Specifically, I'm looking at these things: - on Mac, the page-setup margin setting specifies the placement of the *header* (and footer), not the page content. - on Mac, there seems to be a fixed ~0.25-inch offset between the header and the page content, regardless of the margin setting.

Ok, here's what we do on Mac: - The Mac OS "Page Setup Dialog" margin information is used to set the size & positioning of the **printable surface** of the page. (This essentially determines the distance from paper edge to header/footer, as long as print_edge_XXX is zero.) This happens in nsDeviceContextSpecX::GetSurfaceForPrinter.[1] - The offset between the printable-surface boundary and the page content comes directly from the "print_margin_XXX" settings. These are 0.5 by default.[2] - We can also optionally tweak the specific positioning of the headers/footer (without moving anything else) using the "print_edge_XXX" settings. [3] AFAIK, there's no GUI to modify print_margin_XXX or print_edge_XXX, and that's fine, given that the "Page Setup Dialog" margins let us tweak our printed region to match the printer's actual printable region. I think this behavior is pretty intuitive - definitely moreso than our behavior on linux, at least. I think we can & should emulate it on Linux. Code sources: [1] Page Setup margin info is read in via nsDeviceContextSpecX::GetPageMargins, which is called at http://mxr.mozilla.org/mozilla/source/widget/src/cocoa/nsDeviceContextSpecX.mm#224 when we're setting up the printing surface [2] print_margin_XXX is read out of prefs and into the nsIPrintSettings at http://mxr.mozilla.org/seamonkey/source/widget/src/xpwidgets/nsPrintOptionsImpl.cpp#266 It's then retrieved & used at http://mxr.mozilla.org/seamonkey/source/layout/generic/nsSimplePageSequence.cpp#202 [3] print_edge_XXX is read out of prefs and into the nsIPrintSettings at http://mxr.mozilla.org/seamonkey/source/widget/src/xpwidgets/nsPrintOptionsImpl.cpp#284 It's then retrieved & used at http://mxr.mozilla.org/seamonkey/source/layout/generic/nsSimplePageSequence.cpp#212

Comment on attachment 305839 [details] [diff] [review] patch v1 (just applying s/edge/margin/ to nsPrintSettingsGTK.*) So, the main change that needs to happen for this to work automagically is to make nsDeviceContextSpecG::GetSurfaceForPrinter obtain the GTK Printing Configuration's printer margins, and use those in setting up the surface. If that works, we should be able to get rid of all the nsPrintSettingsGTK::Get/SetEdgeXXX functions, and just fall back on the inherited versions.

(In reply to comment #19) > So, the main change that needs to happen for this to work automagically is to > make nsDeviceContextSpecG::GetSurfaceForPrinter obtain the GTK Printing > Configuration's printer margins, and use those in setting up the surface. ... though I'm not 100% sure that's possible. That behavior on Mac depended on us being able to do this sort of magic in nsDeviceContextSpecX.mm using OS X's Quartz API: 237 CGContextTranslateCTM(context, leftMargin, bottomMargin + height); PR_TRUE); ... thereby positioning the printing surface correctly on the paper. I'm not sure if there's an equivalent function to CGContextTranslateCTM on Linux (or more specifically, an equivalent function for gfxPDFSurface / gfxPSSurface) that would give us the same sort of result.

You should be able to use gfxASurface::SetDeviceOffset to get a similar effect.

Yeah, that seems to do it -- thanks Roc! Also -- whatever changes happen to printing here, I need to make sure they take effect in print preview as well. (That happens for free on Mac; there, print preview effectively prints to temporary PDF which is then opened in a different app. But not so on Windows/Linux.)

(In reply to comment #22) > Yeah, that seems to do it -- thanks Roc! > > Also -- whatever changes happen to printing here, I need to make sure they take > effect in print preview as well. (That happens for free on Mac; there, print > preview effectively prints to temporary PDF which is then opened in a different > app. But not so on Windows/Linux.) > I'm pretty sure Print Preview also uses GetSurfaceForPrinter, we detect for the presence of Print Preview there.

I take it your next patch will have a GTK2-specific version of Get/SetPaperMarginInTwips? Because as it stands there is no way for the margins to be read from the page setup dialog.

Comment on attachment 306641 [details] [diff] [review] patch wip 1 Yup, I've got an in-progress patch that addresses that.

FWIW, on Windows, we get around all of this by directly querying the printer driver to get the imageable size of the page. I'm pretty sure we can't do this on Linux, which is why print_edge_XXX was created in the first place. But anyway, here's where we do this on Windows, in nsThebesDeviceContext.cpp: 705 #ifdef XP_WIN ... 713 size.width = NSFloatPixelsToAppUnits(::GetDeviceCaps(dc, HORZRES)/mPrintingScale, AppUnitsPerDevPixel()); 714 size.height = NSFloatPixelsToAppUnits(::GetDeviceCaps(dc, VERTRES)/mPrintingScale, AppUnitsPerDevPixel()); Code URL: http://mxr.mozilla.org/seamonkey/source/gfx/src/thebes/nsThebesDeviceContext.cpp When I print to BullZip PDF printer, we set the size = 8.5 x 11 here, and (as expected) the headers end up flush with the page edges. When I print to the Mozilla Bldg S printer, we set the size = 8.0 x 10.666 here, and (as expected) the headers end up offset a bit from the page edges.

Here's a better (mostly done?) WIP patch. This patch basically just creates an "UnwriteableMargin" pref, which (if set) is added to both "Edge" (the header offset) and "Margin" (the content offset) during nsSimplePageSequence reflow. As it turns out, the strategy we use on Mac from comment 18 -- reducing the size of the printable surface -- doesn't work on Linux. That stragegy breaks print-to-file, because the resulting PostScript document is confused about how big it should actually be, since it was created as an oddly-sized surface. The strategy in my new patch should have the same effect as what we do on mac, without breaking print-to-file.

I probably could've taken more time to read the patch, but heres something I'd like to know: Does changing the paper size in the Page Setup dialog also change the margin to reflect the new setting, or will it use the locale default every time, even if the user selects an obscure page format e.g. ROC 16k, or even making a custom page size?

(In reply to comment #29) > I probably could've taken more time to read the patch, but heres something I'd > like to know: > > Does changing the paper size in the Page Setup dialog also change the margin to > reflect the new setting Yes -- I've tested this with setting custom page layouts in the Page Setup dialog, and this does change the UnwriteableMargin setting. ** More detailed response: ** Basically, after we run the Page Setup dialog, we hit nsPrintOptions::WritePrefs, in which we extract the gtk_page_setup_get_XXX_margin and store them in the about:config prefs as print_unwriteable_margin_XXX. (This is important because the GTKPageSetup object gets destroyed at this point, and we want to preserve its information) Then, when we print, those values are read via nsPrintOptions::ReadPrefs, and we pass them into a new GTKPageSetup object. (via gtk_page_setup_get_XXX_margin) Finally, the UnwriteableMargin values are read back out of this new GTKPageSetup object in nsSimplePageSequence::Reflow, where they're added to "marginTwips" and "edgeTwips" to shift the header and content away from the edge of the page. > or will it use the locale default every time It only uses the local default if you've never run the Page Setup dialog. (Until you run the Page Setup dialog, your print_unwriteable_margin prefs will all be -1, which means "use default", because negative margin values are simply ignored in nsPrintSettingsGTK::SetUnwriteableMargin.) Once you've run the page setup dialog, the last margins that you selected will be stored in your print_unwriteable_margin pref, and those will be used when you print.

(In reply to comment #30) > Yes -- I've tested this with setting custom page layouts in the Page Setup > dialog, and this does change the UnwriteableMargin setting. ... and this is visible on printed / print-previewed documents.

(In reply to comment #27) > FWIW, on Windows, we get around all of this by directly querying the printer > driver to get the imageable size of the page. I'm pretty sure we can't do this > on Linux, which is why print_edge_XXX was created in the first place. I'm no printer guru but at least for CUPS printers you can get the ImageableArea from the PPD. In the past I used to help creating a patch to read the default paper size from the PPD for CUPS printers but I didn't follow all the printing changes on trunk and the patch was never landed. (bug 324060) I would expect that the same approach can be used to get the ImageableArea.

(In reply to comment #32) > I'm no printer guru but at least for CUPS printers you can get the > ImageableArea from the PPD. Since I'm not a printer guru either, I'm not going to worry about trying that for now. AFAIK, it's simpler to just define the printable region based on the margins that we're already getting from the OS's Page-Setup Dialog, like we do on Mac. I think the approach you're suggesting would require more significant changes to the code, and I'm not sure if it'd work for all types of printers. But if anyone has an idea for how to incorporate such a strategy, though, feel free to file a follow-up bug. :)

Here's current WIP patch, now fixed to make OS X share the same code that I've added for Linux.

Comment on attachment 309011 [details] [diff] [review] patch v4 Requesting review. I'm posting a summary of the patch as my next comment, to help w/ review.

A) First two chunks are typo fixes; s/Ponits/Points/. B) Create "nsPrintSettings::GetUnwriteableMarginInTwips", which on Mac & Linux should return the native print settings' paper-margins. On other platforms, it will return (0,0,0,0) due to the default implementation in nsPrintSettingsImpl.cpp. C) Use nsPrintSettings::GetUnwriteableMarginInTwips in nsSimplePageSequence::Reflow, to increase both "marginTwips" (distance from edge of paper to content -- default 0.5in) and "edgeTwips" (distance from edge of paper to headers -- defaults to 0.0in) *Note that on non-Mac/Linux platforms, this will have no effect, because the default implementation of GetUnwriteableMarginInTwips returns 0's D) Cache "UnwriteableMargin" property in about:config preferences, so that we can recover its value when printing. (This is important because the nsPrintSettings object is destroyed when we close the Print Setup dialog, and we need to store its values for use when we're actually printing.) * Note that this pref defaults to (-1,-1,-1,-1), which (when passed in to SetUnwriteableMarginInTwips) will be ignored, so that we keep the system-default unwriteable margins. E) Remove old Unix/Linux-only hack of defaulting print_edge_XXX to 4 (0.04in) (This was originally intended to make the headers more likely to appear on printable area, rather than right against the paper boundary. But now hearders will be pushed inside the UnwriteableMargin, so we don't have to worry about that anymore.) F) Changes to Cocoa printing code: i) Make printing surface the size of the full paper (to match all other platforms), rather than just the size of the printable area. Making this change because the Unwriteable Margins will now be subtracted out in nsSimplePageSequence::Reflow, as described above in part (C), so we don't need to subtract them when setting up the surface. ii) Effectively just move nsDeviceContextSpecX::GetPageMargins to nsPrintSettingsX::GetUnwriteableMarginInTwips. G) Magic-number fixes: s/20/TWIPS_PER_POINT_FLOAT/. (using FLOAT, not INT, because the "width" and "height" being divided are doubles, so we're doing float math.)

The only outstanding things I want to check up on / address are: 1) Will this give us the right margins in Landscape orientation? I'm going to take a look at that. If that requires a change, it'll be minor. 2) I should probably implement a custom version of SetUnwriteableMarginInTwips for nsPrintSettingsX, rather than falling back on the default version, which does nothing.* *This method's implementation hasn't been necessary thus far, because (unlike GTK) our Cocoa printing code preserves the PMPageFormat object between when we show the Print Dialog and when we actually print, and so we already remember the unwriteable margins without having to grab them from the prefs. But I think I should implement the Setter for symmetry and also so that changes made manually to about:config will take effect.

(In reply to comment #38) > 2) I should probably implement a custom version of SetUnwriteableMarginInTwips > for nsPrintSettingsX, rather than falling back on the default version, which > does nothing.* Note to self: this will probably require using PMPaperCreateCustom to make a replacement for the old PMPaper object with new margins, because there is no "PMPaperSetMargins" function.

Comment on attachment 309011 [details] [diff] [review] patch v4 >+ >+ /** >+ * Sets/Gets the "unwriteable margin" for the page format. This defines >+ * the boundary from which we'll measure the EdgeInTwips and MarginInTwips >+ * attributes, to place the headers and content, respectively. >+ */ >+ [noscript] void SetUnwriteableMarginInTwips(in nsNativeMarginRef aEdge); >+ [noscript] void GetUnwriteableMarginInTwips(in nsNativeMarginRef aEdge); > }; Interesting how you document the effects of negative margins everywhere but here, where documentation is most important :) Document in detail here about the effects of -1, something like: "On platforms which implement this function, a negative value on a margin will be ignored and the previous value for that margin will be kept. If the margin has not been set before, this means the system default value of that margin is kept." Other than that, looks good.

(In reply to comment #38) > 2) I should probably implement a custom version of SetUnwriteableMarginInTwips > for nsPrintSettingsX, rather than falling back on the default version, which > does nothing.* > ... > But I > think I should implement the Setter for symmetry and also so that changes made > manually to about:config will take effect. Actually, I think I'm taking this bak -- I don't think it's a good idea to implement a setter on Mac currently, because its output gets overwritten anyway, which would make it useless and untestable. I've found that our mac code implements a **custom** version of nsPrintOptions::ReadPrefs (called nsPrintOptionsX::ReadPrefs) which does the following: * First, calls the superclass function (including, with my patch, calling mPrintSettings->SetUnwriteableMargin with values read from prefs) * Then, it calls printSettingsX->ReadPageFormatFromPrefs, which overwrites the "mPageFormat" variable with a mac-specific binary-blob that's read from the prefs. mPageFormat (or its PMPaper component, to be specific) is where the UnwriteableMargin is stored right now on mac. So, an implementation of SetUnwriteableMargins would end up tweaking mPageFormat, and these tweaks would be obliterated when we overwrite mPageFormat during the call to printSettingsX->ReadPageFormatFromPrefs. Ultimately, I think the best way to handle this would be to not store mPageFormat as a binary blob, and instead store the bits of it that we need as individual prefs. (If they're mac-specific things, we can read/write those separate prefs in the mac-specific nsPrintOptions function) That's beyond the scope of this bug, though. (also, fwiw, I'm not sure such a change would make it in time for FF3, nor is it crucial that it would.)

+ mMargin = nsMargin(aPresContext->TwipsToAppUnits(marginTwips.left + + unwriteableTwips.left), + aPresContext->TwipsToAppUnits(marginTwips.top + + unwriteableTwips.top), + aPresContext->TwipsToAppUnits(marginTwips.right + + unwriteableTwips.right), + aPresContext->TwipsToAppUnits(marginTwips.bottom + + unwriteableTwips.bottom)); Make a helper function and use nsMargin::operator+. Other than that, looks fine. Are you going to submit a new version after addressing comment #38?

(In reply to comment #38) > 1) Will this give us the right margins in Landscape orientation? I'm going to > take a look at that. If that requires a change, it'll be minor. Cool -- my patch does handle margins correctly in landscape mode. i.e. if I print-preview a landscape-oriented page, with a large "top margin", then I get a large margin on the top (11-inch) side of the print-previewed page. (rather than on the 8.5-inch side of the page, which is where the top margin would be in portrait orientation.) bug 389949 is still an issue, though -- landscape mode gets printed on top of portrait-oriented paper. I'm going to take care of that after this patch lands.

(In reply to comment #42) > Make a helper function and use nsMargin::operator+. Will do. > Are you going to submit a new version after addressing comment #38? Nope, no need -- see comment 41 and comment 43, which address the potential issues I brought up in comment 38. I will post an interdiff that addresses your & Michael's review suggestions, though.

Comment on attachment 309281 [details] [diff] [review] interdiff between patches v4 and v5 Summary of interdiff: - added documentation of negative-number behavior to nsIPrintSettings.idl (requested by Michael) - added helper function, and used operator+, for margin-adding. (requested by roc) - Made a whitespace-only change to fix indentation in nested "if" statements, in widget/src/cocoa/nsPrintSettingsX.mm

Comment on attachment 309281 [details] [diff] [review] interdiff between patches v4 and v5 > MarginTwipsToAppUnits Just overload TwipsToAppUnits

> Just overload TwipsToAppUnits Ah, good idea. That's what this version does.

Patch v5a landed. Thanks for the reviews, Michael & roc! Checking in gfx/thebes/public/gfxPDFSurface.h; /cvsroot/mozilla/gfx/thebes/public/gfxPDFSurface.h,v <-- gfxPDFSurface.h new revision: 1.11; previous revision: 1.10 done Checking in gfx/thebes/public/gfxPSSurface.h; /cvsroot/mozilla/gfx/thebes/public/gfxPSSurface.h,v <-- gfxPSSurface.h new revision: 1.11; previous revision: 1.10 done Checking in layout/base/nsPresContext.h; /cvsroot/mozilla/layout/base/nsPresContext.h,v <-- nsPresContext.h new revision: 3.201; previous revision: 3.200 done Checking in layout/generic/nsSimplePageSequence.cpp; /cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v <-- nsSimplePageSeq uence.cpp new revision: 3.165; previous revision: 3.164 done Checking in layout/printing/nsPrintEngine.cpp; /cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v <-- nsPrintEngine.cpp new revision: 1.168; previous revision: 1.167 done Checking in modules/libpref/src/init/all.js; /cvsroot/mozilla/modules/libpref/src/init/all.js,v <-- all.js new revision: 3.736; previous revision: 3.735 done Checking in widget/public/nsIPrintSettings.idl; /cvsroot/mozilla/widget/public/nsIPrintSettings.idl,v <-- nsIPrintSettings.idl new revision: 1.45; previous revision: 1.44 done Checking in widget/src/cocoa/nsDeviceContextSpecX.h; /cvsroot/mozilla/widget/src/cocoa/nsDeviceContextSpecX.h,v <-- nsDeviceContextSpecX.h new revision: 1.7; previous revision: 1.6 done Checking in widget/src/cocoa/nsDeviceContextSpecX.mm; /cvsroot/mozilla/widget/src/cocoa/nsDeviceContextSpecX.mm,v <-- nsDeviceContextSpecX.mm new revision: 1.13; previous revision: 1.12 done Checking in widget/src/cocoa/nsPrintSettingsX.h; /cvsroot/mozilla/widget/src/cocoa/nsPrintSettingsX.h,v <-- nsPrintSettingsX.h new revision: 1.3; previous revision: 1.2 done Checking in widget/src/cocoa/nsPrintSettingsX.mm; /cvsroot/mozilla/widget/src/cocoa/nsPrintSettingsX.mm,v <-- nsPrintSettingsX.mm new revision: 1.5; previous revision: 1.4 done Checking in widget/src/gtk2/nsDeviceContextSpecG.cpp; /cvsroot/mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp,v <-- nsDeviceContextSpecG.cpp new revision: 1.104; previous revision: 1.103 done Checking in widget/src/gtk2/nsPrintSettingsGTK.cpp; /cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.cpp,v <-- nsPrintSettingsGTK.cpp new revision: 1.6; previous revision: 1.5 done Checking in widget/src/gtk2/nsPrintSettingsGTK.h; /cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.h,v <-- nsPrintSettingsGTK.h new revision: 1.4; previous revision: 1.3 done Checking in widget/src/os2/nsDeviceContextSpecOS2.cpp; /cvsroot/mozilla/widget/src/os2/nsDeviceContextSpecOS2.cpp,v <-- nsDeviceContextSpecOS2.cpp new revision: 1.55; previous revision: 1.54 done Checking in widget/src/windows/nsDeviceContextSpecWin.cpp; /cvsroot/mozilla/widget/src/windows/nsDeviceContextSpecWin.cpp,v <-- nsDeviceContextSpecWin.cpp new revision: 3.74; previous revision: 3.73 done Checking in widget/src/xpwidgets/nsPrintOptionsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintOptionsImpl.cpp,v <-- nsPrintOptionsImpl.cpp new revision: 1.98; previous revision: 1.97 done Checking in widget/src/xpwidgets/nsPrintSettingsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.cpp,v <-- nsPrintSettingsImpl.cpp new revision: 1.44; previous revision: 1.43 done

This patch broke a few pagination reftests. I'm pretty sure the issue is that the tests weren't written to expect "UnwriteableMargins" in their printing surfaces, but now they're getting 'em, so there's less space on the page than was originally envisioned when the tests were written. Backing out, & will re-land when I can either a) fix the tests, or b) fix the patch so it doesn't give unwriteablemargins to our pagination tests.

Link to broken tinderbox log: Mac OS X: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1205481420.1205483013.9414.gz#err2

(In reply to comment #53) > Link to broken tinderbox log: Linux: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1205483492.1205489047.21589.gz

This patch fixes the reftest failures on Linux by manually setting the UnwriteableMargin to 0 for paginated reftests. (see changes to reftest.js) For it to work on Mac, I'm going to have to change my mind about Comment 41, though, and implement SetUnwriteableMarginXXX on mac. Working on getting that going.

(patch v7 had an unnecessary whitespace change and a comment that was less clear than it could be -- both fixed in this version.)

Comment on attachment 310170 [details] [diff] [review] interdiff between patch v5a and v7a Changes in this patch: > nsIPrintSettings.idl > reftest.js - Created formal attributes for unwriteableMarginTop/Left/etc, so that we can easily use JS to set them to 0 during reftests (in reftest.js). This also now better matches our interface for the "margin" & "edge" properties -- those both have attributes for top/left/etc in the IDL file.) > nsPrintSettingsImpl.h / cpp - Add Getters/Setters for the unwriteableMarginTop/Left/etc attributes > nsPrintSettingsX.h / mm > nsPrintSettingsGTK.h / cpp > nsPrintSettingsImpl.h / cpp - Added an explicit mUnwriteableMargin variable in the nsPrintSettings class, to back all of the Get/SetUnwriteableMargin functions. This variable gets its values initially from the native print-settings objects. (and if the native print-settings objects are overwritten, we re-initialize mUnwriteableMargin as well, using InitUnwriteableMargin()) This is better than the behavior from patch v5a because: a) the new behavior mirrors how we handle most other print parameters b) the new behavior lets me implement SetUnwriteableMargin on Mac, despite the fact that the native OS X print-settings types (PMPaper in particular) are immutable There's just one remaining thing I'm not 100% sure about, which is this comment: // XXXdholbert -- do I need to PMRelease the PMPaper or PMPaperMargins? I'm not totally sure how the OS X Core Printing memory management works, so I'm not sure if I need to call ::PMRelease on the PMPaper object there. But I'll find that out & resolve it before checking in.

Comment on attachment 310170 [details] [diff] [review] interdiff between patch v5a and v7a Michael, your feedback would be much appreciated as well, if you get a chance to look at this. (I'm not sure what your schedule's like during the week these days)

(Also, the patch passes reftests now. :))

Would very much like this to make it into Beta 5 at some point during the freeze -- marking target milestone as mozilla1.9beta5.

(In reply to comment #61) > There's just one remaining thing I'm not 100% sure about, which is this > comment: > // XXXdholbert -- do I need to PMRelease the PMPaper or PMPaperMargins? > I'm not totally sure how the OS X Core Printing memory management works, so I'm > not sure if I need to call ::PMRelease on the PMPaper object there. But I'll > find that out & resolve it before checking in. To follow up on this question -- according to Apple's core printing guide, I would only need to PMRelease these objects if I had called PMRetain on them. (i.e. if I wanted to keep them around for a while longer) So, the code is good as-is. Removing that XXX comment. And Vlad, thanks for the r+sr! Source: http://developer.apple.com/documentation/GraphicsImaging/Reference/CorePrintRef/Reference/reference.html#//apple_ref/c/func/PMRelease

Final patch -- I just removed the XXX comment mentioned above, and I added a clarifying comment to nsPrintSettingsImpl.h: + // mMargin, mEdge, and mUnwriteableMargin are stored in twips

patch v7b landed. Checking in gfx/thebes/public/gfxPDFSurface.h; /cvsroot/mozilla/gfx/thebes/public/gfxPDFSurface.h,v <-- gfxPDFSurface.h new revision: 1.13; previous revision: 1.12 done Checking in gfx/thebes/public/gfxPSSurface.h; /cvsroot/mozilla/gfx/thebes/public/gfxPSSurface.h,v <-- gfxPSSurface.h new revision: 1.13; previous revision: 1.12 done Checking in layout/base/nsPresContext.h; /cvsroot/mozilla/layout/base/nsPresContext.h,v <-- nsPresContext.h new revision: 3.203; previous revision: 3.202 done Checking in layout/generic/nsSimplePageSequence.cpp; /cvsroot/mozilla/layout/generic/nsSimplePageSequence.cpp,v <-- nsSimplePageSequence.cpp new revision: 3.167; previous revision: 3.166 done Checking in layout/printing/nsPrintEngine.cpp; /cvsroot/mozilla/layout/printing/nsPrintEngine.cpp,v <-- nsPrintEngine.cpp new revision: 1.170; previous revision: 1.169 done Checking in layout/tools/reftest/reftest.js; /cvsroot/mozilla/layout/tools/reftest/reftest.js,v <-- reftest.js new revision: 1.31; previous revision: 1.30 done Checking in modules/libpref/src/init/all.js; /cvsroot/mozilla/modules/libpref/src/init/all.js,v <-- all.js new revision: 3.741; previous revision: 3.740 done Checking in widget/public/nsIPrintSettings.idl; /cvsroot/mozilla/widget/public/nsIPrintSettings.idl,v <-- nsIPrintSettings.idl new revision: 1.47; previous revision: 1.46 done Checking in widget/src/cocoa/nsDeviceContextSpecX.h; /cvsroot/mozilla/widget/src/cocoa/nsDeviceContextSpecX.h,v <-- nsDeviceContextSpecX.h new revision: 1.9; previous revision: 1.8 done Checking in widget/src/cocoa/nsDeviceContextSpecX.mm; /cvsroot/mozilla/widget/src/cocoa/nsDeviceContextSpecX.mm,v <-- nsDeviceContextSpecX.mm new revision: 1.15; previous revision: 1.14 done Checking in widget/src/cocoa/nsPrintSettingsX.h; /cvsroot/mozilla/widget/src/cocoa/nsPrintSettingsX.h,v <-- nsPrintSettingsX.h new revision: 1.5; previous revision: 1.4 done Checking in widget/src/cocoa/nsPrintSettingsX.mm; /cvsroot/mozilla/widget/src/cocoa/nsPrintSettingsX.mm,v <-- nsPrintSettingsX.mm new revision: 1.7; previous revision: 1.6 done Checking in widget/src/gtk2/nsDeviceContextSpecG.cpp; /cvsroot/mozilla/widget/src/gtk2/nsDeviceContextSpecG.cpp,v <-- nsDeviceContextSpecG.cpp new revision: 1.106; previous revision: 1.105 done Checking in widget/src/gtk2/nsPrintSettingsGTK.cpp; /cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.cpp,v <-- nsPrintSettingsGTK.cpp new revision: 1.8; previous revision: 1.7 done Checking in widget/src/gtk2/nsPrintSettingsGTK.h; /cvsroot/mozilla/widget/src/gtk2/nsPrintSettingsGTK.h,v <-- nsPrintSettingsGTK.h new revision: 1.6; previous revision: 1.5 done Checking in widget/src/os2/nsDeviceContextSpecOS2.cpp; /cvsroot/mozilla/widget/src/os2/nsDeviceContextSpecOS2.cpp,v <-- nsDeviceContextSpecOS2.cpp new revision: 1.57; previous revision: 1.56 done Checking in widget/src/windows/nsDeviceContextSpecWin.cpp; /cvsroot/mozilla/widget/src/windows/nsDeviceContextSpecWin.cpp,v <-- nsDeviceContextSpecWin.cpp new revision: 3.76; previous revision: 3.75 done Checking in widget/src/xpwidgets/nsPrintOptionsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintOptionsImpl.cpp,v <-- nsPrintOptionsImpl.cpp new revision: 1.100; previous revision: 1.99 done Checking in widget/src/xpwidgets/nsPrintSettingsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.cpp,v <-- nsPrintSettingsImpl.cpp new revision: 1.46; previous revision: 1.45 done Checking in widget/src/xpwidgets/nsPrintSettingsImpl.h; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.h,v <-- nsPrintSettingsImpl.h new revision: 1.33; previous revision: 1.32 done

Maybe I'm missing something, but the Get/SetUnwriteableMargin* functions in nsPrintSettingsImpl.cpp seem to be unbalanced. The getter returns a double containing inches, and the setter takes a twips value as a double... See line 512. Are these missing a NS_INCHES_TO_TWIPS() conversion? They don't seem to be called.

(In reply to comment #68) > Maybe I'm missing something, but the Get/SetUnwriteableMargin* functions in > nsPrintSettingsImpl.cpp seem to be unbalanced. The getter returns a double > containing inches, and the setter takes a twips value as a double... See line > 512. > > Are these missing a NS_INCHES_TO_TWIPS() conversion? > > They don't seem to be called. > Oops -- good catch! I'll post a follow-up fix shortly. (FWIW, this typo shouldn't cause any problems yet, because the only place these setters are used is in reftest.js, where we just pass in 0.)

Comment on attachment 310626 [details] [diff] [review] follow-up patch [checked in] Heh, I thought something looked odd here, I just didn't trace things all the way through correctly.

Comment on attachment 310626 [details] [diff] [review] follow-up patch [checked in] Follow-up patch checked in. Checking in nsPrintSettingsImpl.cpp; /cvsroot/mozilla/widget/src/xpwidgets/nsPrintSettingsImpl.cpp,v <-- nsPrintSettingsImpl.cpp new revision: 1.47; previous revision: 1.46 done