1650886 - Add available paper size information to nsIPrinter interface

Status: RESOLVED FIXED

(Core :: Printing: Setup, enhancement, P1)

Description

[Erik Nordin [:nordzilla]]

The new print preview UI will require page-size information. This patch will add functionality to get the available page size names and dimensions from an nsIPrinter.

Implementation Plan

• Part 1 -- Add nsIPaper Interface and base implementation
• Part 2 -- Implement retrieval for macOS
• Part 3 -- Implement retrieval for GTK
• Part 4 -- Implement retrieval for Windows

Comment 1

[Erik Nordin [:nordzilla]]

Attachment: Bug 1650886 - Part 1 nsIPaper Base Implementation r=jwatt,alaskanemily

• Add nsIPaper interface.
• Add nsIPaper implementaiton.

Comment 2

[Erik Nordin [:nordzilla]]

Attachment: Bug 1650886 - Part 2 Expose Paper Sizes for macOS r=jwatt,alaskanemily

• Add macOS-specific function to retrieve the paper list for a given printer.
• Add JS test to ensure papers are initialized with valid values.

Comment 3

[Erik Nordin [:nordzilla]]

Attachment: Bug 1650886 - Part 3 Expose Paper Sizes for Linux (GTK) r=jwatt,alaskanemily

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed8db2a54c61
Part 1 nsIPaper Base Implementation r=jwatt
https://hg.mozilla.org/integration/autoland/rev/e2485d1bb142
Part 2 Expose Paper Sizes for macOS r=jwatt

Comment 5

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=e2485d1bb1429c90880968b886565a3beb3ba9fe&failure_classification_id=2&selectedTaskRun=PQ2q5WchT-mGVE2FJREUwg.0

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309456978&repo=autoland&lineNumber=117770

Backout: https://hg.mozilla.org/integration/autoland/rev/6c9609b0b2516c964a65b42796fd58917f7f66b6

Comment 6

[Bob Owen (:bobowen)]

At least some of this is down to a previous change that hasn't been hit until this test:
https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/widget/windows/nsDeviceContextSpecWin.cpp#598

When the argument was changed this wasn't removed and never gets deleted.

I still seem to be getting another nsTArray_base leaked however ... still looking.

Comment 7

[Bob Owen (:bobowen)]

Found the last one, it is from here [1], which is called as part of [2].

I think we need to add a custom destructor to nsPrinterListWin and add the following into it:
GlobalPrinters::GetInstance()->FreeGlobalPrinters();

[1] https://searchfox.org/mozilla-central/rev/89814940895946b48b4c04c702efd2c676ec8e7e/widget/windows/nsDeviceContextSpecWin.cpp#624
[2] https://searchfox.org/mozilla-central/rev/89814940895946b48b4c04c702efd2c676ec8e7e/widget/windows/nsDeviceContextSpecWin.cpp#589

Comment 8

[Erik Nordin [:nordzilla]]

Thanks for hunting those down. I found the GlobalPrinters::GetInstance()->FreeGlobalPrinters(); one over the weekend but I saw that it didn't fix the entire issue:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8448f368dcb90a23584e5639a029f32af4112668&selectedTaskRun=Vp8Vopv_Szam689iOOyHnw.0

I will patch this ASAP.

Comment 9

[Erik Nordin [:nordzilla]]

I've launched a try with the updated changes.

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

Will land as soon as I see it's green. This is my fault for not running a try on Windows initially. I only tested the behavior on macOS because that is the implementation that is landing in part 2 where the test is introduced. I didn't consider that there may be a previously untested memory leak in the Windows code.

Edit:

Try came up green: re-queuing to land.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f65d2491cbb4
Part 1 nsIPaper Base Implementation r=jwatt
https://hg.mozilla.org/integration/autoland/rev/de485a27b337
Part 2 Expose Paper Sizes for macOS r=jwatt

Comment 11

[Atila Butkovits]

Backed outfor causing bustage at Unified_cpp_widget_windows1.obj

Backout link: https://hg.mozilla.org/integration/autoland/rev/70319a11bacb0a38df08ad586bbb3a5ce5c13764

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=de485a27b33787fbdea473b1647920068e3c491f&selectedTaskRun=Yt_PSb-LSYqh4PJIilQ0Ug.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309623367&repo=autoland&lineNumber=37137

Comment 12

[Erik Nordin [:nordzilla]]

Oh gosh, that's embarrassing. I re-queued for landing after the try came up green, but I forgot to push the latest update to phabricator.

My apologies.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7614ba2d1d3
Part 1 nsIPaper Base Implementation r=jwatt
https://hg.mozilla.org/integration/autoland/rev/82d3cbd0c3a8
Part 2 Expose Paper Sizes for macOS r=jwatt

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a7614ba2d1d3
https://hg.mozilla.org/mozilla-central/rev/82d3cbd0c3a8

Comment 15

[Bob Owen (:bobowen)]

Attachment: Bug 1650886 Part 4: Expose Paper Sizes for Windows. r=jwatt!

Comment 16

[Bob Owen (:bobowen)]

Part 4 try push:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedTaskRun=Cpvqs5H3S9iRzBPOMSw2lA.0&revision=a77f7420339add812b32911a40b48cc8516f4f67

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20d873dac781
Part 4: Expose Paper Sizes for Windows. r=jwatt

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/20d873dac781

Comment 19

[Erik Nordin [:nordzilla]]

Marking as resolved, because all relevant patches have landed.