1667053 - Convert nsPrintSettingsX to do something like nsPrintSettingsWin::CopyFromNative/CopyToNative

Status: VERIFIED FIXED

(Core :: Printing: Setup, task, P2)

Description

[Jonathan Watt [:jwatt]]

nsPrintSettingsX containing an NSPrintInfo is a continued source of bugs and pain, just the latest example being bug 1666473. (Two sources of truth is never good.) We should get rid of nsPrintSettingsX::mPrintInfo and do something like nsPrintSettingsWin::CopyFromNative/CopyToNative instead.

Comment 1

[Jonathan Kew [:jfkthame]]

Proposing to steal this from Erik as I understand he's busy with other things at the moment.

Comment 2

[Erik Nordin [:nordzilla]]

Attachment: Bug 1667053 - Refactor macOS print settings implementation for clarity.

Uploading the WIP that I had been working on for this.
Feel free to cherrypick anything from it, or start for scratch.
I just want it to be available if it's useful at all.

I was currently trying to figure out why PMPageFormat
was getting messed up and causing tiny, cropped pages.

Comment 3

[Erik Nordin [:nordzilla]]

jfkthame I uploaded the WIP that I had been working on in case it's useful to you.

Comment 4

[Jonathan Kew [:jfkthame]]

Thanks! I'll take a look.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1667053 patch 2 - If the user runs the system print UI, hold on to and use the printInfo it returns.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fba6c89ef4cf
Refactor macOS print settings implementation for clarity. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/164586483e97
patch 2 - If the user runs the system print UI, hold on to and use the printInfo it returns. r=jwatt

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/fba6c89ef4cf
https://hg.mozilla.org/mozilla-central/rev/164586483e97

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 9180466 [details]
Bug 1667053 - Refactor macOS print settings implementation for clarity.

Beta/Release Uplift Approval Request

• User impact if declined: Handling of printer settings on macOS is sometimes unreliable due to complex/flaky code (e.g. bug 1672304); this patch rearchitects the settings management for simpler, clearer, more reliable code. Uplifting this will also simplify uplift of other printing-related patches (e.g. bug 1670943) which will otherwise need significant work to rebase onto the older architecture.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: No specific STR but extensive testing of various print dialog settings such as paper size & orientation, print job destinations, scaling etc (through both the tab-modal and legacy UIs) would be valuable.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This is a major rewrite of how the print job settings are managed, so does come with some risk of its own new bugs; but overall we believe the new code to be a much better foundation, and simpler to debug and maintain.
  The alternative is to continue applying ad hoc fixes to the old code whenever flaky behavior is noted.
• String changes made/needed:

Comment 10

[Vlad Lucaci, Desktop QA (:vlucaci)]

We have performed an unscheduled test run(https://testrail.stage.mozaws.net/index.php?/reports/view/3612) in order to verify that no regressions were introduced by the macOS print settings refactoring in Firefox 84. We have executed our smoke & sanity test cases as well as an extensive exploratory session targeting both the new and old print UI with an emphasis on different print settings.

During this test run, we have encountered 1 issue (1673586) but it doesn't seem to be caused by this particular code refactor. Marking this as verified fixed for 84.
Leaving a ni? on myself to track this once it gets approved for beta 83. Depending when it lands, this will be covered in our preliminary 83 preliminary feature testing (26-30 Oct) or in our pre-release 83 feature testing (2-6 Nov)

Comment 11

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9180466 [details]
Bug 1667053 - Refactor macOS print settings implementation for clarity.

Let's take it in 83 beta 5 now as I'd like to have feedback from QA as early as possible in case we have to fix or back it out before we ship 83.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/996f437b3138
https://hg.mozilla.org/releases/mozilla-beta/rev/5c33c8b37ea5

Comment 13

[Vlad Lucaci, Desktop QA (:vlucaci)]

As per comment 10, we have covered this in our preliminary 83 feature testing(https://testrail.stage.mozaws.net/index.php?/plans/view/36545) and only managed to find one specific macOS issue : 1674155 that does not seem to be related to the refactoring, but instead to a very specific combination of hardware.