396278 - should take edge values from print settings instead of prefs

Status: RESOLVED FIXED

(Core :: Printing: Output, enhancement)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

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 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: proposed patch

Comment 2

[Christian Persch (GNOME) (away; not receiving bug mail)]

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.

Comment 3

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

   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 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 281005 [details] [diff] [review]
proposed patch

Hrm, I'm not the right person to review this -- I know nothing about printing.

Comment 5

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

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

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

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

Comment 7

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: patch for checkin (added comments, and bumped nsIPrintSettings' UUID)

Comment 8

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: additional patch

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 9

[Mike Schroepfer]

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.

Comment 10

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

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

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 284842 [details] [diff] [review]
additional patch

Sorry for the delay, this fell off my radar.

Comment 12

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

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

Comment 13

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: combined patch for checkin

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 14

[Mike Schroepfer]

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

a+

Comment 15

[Reed Loden [:reed]]

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

Comment 16

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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

Comment 17

[Kenneth Herron]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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

Comment 19

[Kenneth Herron]

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.

Comment 20

[Reed Loden [:reed]]

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

Comment 21

[Jeremy Lea]

Attachment: Fix up twips to app unit conversions

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

Comment 22

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

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

Comment 23

[Christian Persch (GNOME) (away; not receiving bug mail)]

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.