1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Haik Aftandilian [:haik]]

With the fix for bug 1303051, e10s Mac printing will again respect the scaling % set in the print dialog, but there is a remaining issue with remote printing which affects print scaling. (Remote printing is limited to Nightly at this time.)

Since 1303051 may need to be uplifted to Release, I'm using this bug to address the remote printing scaling issue separately.

With 1303051, on Nightly, scaled prints don't take up the full page. The content is scaled appropriately, but the page boundaries also appear to be scaled. As a result, scaled prints don't take up the full page (for each page) and prints require the same number of pages as non-scaled. This needs to be root caused and fixed so that remote printing can ship.

Comment 1

[Haik Aftandilian [:haik]]

Attachment: WIP patch that adjusts the size of the print surface to workaround scaling issue

This patch works around the problem, but I don't think it's the right fix. The patch changes the off-screen surface dimensions in nsDeviceContextSpecProxy::MakePrintTarget() which is common to Mac and Windows. Given that scaling already works for Windows remote printing, I suspect one of the scaling or DPI values derived from the Mac-specific print settings are causing the bug.

Comment 2

[Haik Aftandilian [:haik]]

The scaling issue with remote printing happens because the fix for bug 1303051 derives the child context width and height from the paper size. That's wrong because the size used on the parent for the drawing differs depending on the scaling. When scaling is used, the drawing dimensions are larger on the parent. In the parent, the drawing dimensions come from nsDeviceContextSpecX::GetPaperRect() where OS X's PMGetAdjustedPaperRect() is called. PMGetAdjustedPaperRect() takes a PMPageFormat object which includes the scaling percentage set in the print dialog. I'll post a new fix that sends the adjusted paper rect sizes to the child to be used for printing dimensions.

Comment 3

[Haik Aftandilian [:haik]]

Attachment: Bug 1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page;

Send the size from PMGetAdjustedPaperRect to the child to use as the page size.
Add an Mac implementation of GetEffectivePageSize that returns the adjusted dimensions.

Review commit: https://reviewboard.mozilla.org/r/92440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/92440/

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8810023 [details]
Bug 1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page;

https://reviewboard.mozilla.org/r/92440/#review93198

Thanks!

::: widget/cocoa/nsPrintSettingsX.mm:241
(Diff revision 1)
> +  *aWidth  = NS_INCHES_TO_TWIPS(mAdjustedPaperWidth / mWidthScale);
> +  *aHeight = NS_INCHES_TO_TWIPS(mAdjustedPaperHeight / mHeightScale);

Should do divide by 0 checks here

Comment 5

[Haik Aftandilian [:haik]]

Comment on attachment 8810023 [details]
Bug 1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92440/diff/1-2/

Comment 6

[Haik Aftandilian [:haik]]

Comment on attachment 8810023 [details]
Bug 1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page;

https://reviewboard.mozilla.org/r/92440/#review93198

> Should do divide by 0 checks here

Thanks for the review. I've added checks so that mWidthScale and mHeightScale can never be set to 0.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50f830bdb2c8
OS X Remote printing (print_via_parent) scaled prints don't fill page; r=mconley

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing reftest-sanity/page-width-4.1in.html on OS X 10.10 debug:

https://hg.mozilla.org/integration/autoland/rev/c8d1de55c6bfb14f68001a45a7f33a460ca221ed

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=50f830bdb2c836b149bf36b8f9f8b6b6e634c130
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=6800599&repo=autoland

07:57:09     INFO - REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/reftest-sanity/page-width-4.1in.html == file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/reftest-sanity/page-width-4in.html
07:57:09     INFO - REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/reftest-sanity/page-width-4.1in.html | 61 / 14237 (0%)
07:57:09     INFO - ++DOMWINDOW == 146 (0x121e1c800) [pid = 1978] [serial = 147] [outer = 0x11a054000]
07:57:09     INFO - [Child 1978] WARNING: Caller should supply a printer name.: file /builds/slave/autoland-m64-d-000000000000000/build/src/widget/nsPrintOptionsImpl.cpp, line 1130
07:57:09     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/reftest-sanity/page-width-4.1in.html == file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/reftest-sanity/page-width-4in.html | image comparison, max difference: 255, number of differing pixels: 190

Comment 9

[Haik Aftandilian [:haik]]

This was backed out due to reftest-print tests failing.

The fix added a nsPrintSettingsX::GetEffectivePageSize override which returns adjusted width/height values derived from the OS native PMPageFormat, specifically PMGetAdjustedPaperRect(). These values get setup when an nsPrintSettingsX object is created and are later updated via calls to the InitAdjustedPaperSize() method.

With print-reftests an nsPrintSettings object is built up manually in reftest-content.js and then later GetEffectivePageSize is called, but the adjusted width/height values are never set to correspond to the manual sizes so GetEffectivePageSize doesn't return the expected size. Prior to the fix, the base GetEffectivePageSize in nsPrintSettings returned sizes based on paperWidth and paperHeight.

from reftest-content.js,

  function setupPrintMode() {
     var PSSVC =
         CC[PRINTSETTINGS_CONTRACTID].getService(CI.nsIPrintSettingsService);
     var ps = PSSVC.newPrintSettings;
     ps.paperWidth = 5;
     ps.paperHeight = 3;
     ...
     docShell.contentViewer.setPageMode(true, ps);
  }

The updated fix adds nsPrintSettingsX overrides for SetPaperWidth and SetPaperHeight that also set the adjusted width and height values. When a real print occurs, SetPaperWidth/Height get called and the native adjusted values will be overwritten afterwards by calls to InitAdjustedPaperSize or SetAdjustedPaperSize.

Comment 10

[Haik Aftandilian [:haik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35a4480b330edb143ccb0866a99f9aa15e9cf337

New fix passes the reftest-print tests. This wasn't caught before inbound due to an oversight on my part.

Comment 11

[Haik Aftandilian [:haik]]

Comment on attachment 8810023 [details]
Bug 1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92440/diff/2-3/

Comment 12

[Haik Aftandilian [:haik]]

Got an updated r+ from mconley on IRC.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3099750e870
OS X Remote printing (print_via_parent) scaled prints don't fill page; r=mconley

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3099750e870

Comment 15

[Haik Aftandilian [:haik]]

Comment on attachment 8810023 [details]
Bug 1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page;

Comment on attachment 8811533 [details]
Bug 1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page

Approval Request Comment
[Feature/regressing bug #]: bug 1317801

[User impact if declined]: when we enable remote printing on 52, scaled prints won't fill the page without this fix, remote printing is needed for the first version of the Mac content sandbox

[Describe test coverage new/current, TreeHerder]: enabled on central 

[Risks and why]: Low risk, changes limited to printing, been tested on central

[String/UUID change made/needed]: None

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8810023 [details]
Bug 1315121 - OS X Remote printing (print_via_parent) scaled prints don't fill page;

printing fix prerequisite for osx sandboxing, take in aurora52

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84a91fbd335a