Closed Bug 1315121 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

52 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: haik, Assigned: haik)

References

Details

(Whiteboard: sbmc1)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1314056
Depends on: 1303051
Assignee: nobody → haftandilian
Whiteboard: sbmc1
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.
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 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
Attachment #8810023 - Flags: review?(mconley) → review+
Blocks: 1317801
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.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/50f830bdb2c8
OS X Remote printing (print_via_parent) scaled prints don't fill page; r=mconley
Keywords: checkin-needed
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
Flags: needinfo?(haftandilian)
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.
Flags: needinfo?(haftandilian)
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.
Attachment #8807431 - Attachment is obsolete: true
Got an updated r+ from mconley on IRC.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b3099750e870
OS X Remote printing (print_via_parent) scaled prints don't fill page; r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b3099750e870
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8810023 - Flags: approval-mozilla-aurora?
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
Attachment #8810023 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.