1669149 - Platform code changes to prepare to validate prefs against printer to improve performance

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

This is a sketch of an idea about how the PrintPreview API could be changed to improve performance and check settings against the printer.
It’s written as a change to the nsFrameLoader API, but could be done in frontend code, although some of the steps might need new platform APIs.
I don't know how the "Apply settings to printer" step works for CUPS printers or even if it is possible.

As it would now return print settings as well, it would now need to return a Promise that resolves to an object that contains the print settings and the current promise for the PrintPreviewSuccessInfo.
Steps involving printer interaction would need to be run async on background threads.

PrintPreview(nsIPrintSettings settings)->Promise->{nsIPrintSettings, Promise}:
1. settings passed in
2. call content API to create/update preview with settings
3. apply settings to printer and retrieve updated settings (like at [1])
4. compare settings from printer to settings from prefs
5. should normally be no difference so return Promise with settings and Promise
Alternatives
1a. no settings passed (this would be the first call)
   1a1. Get settings from prefs
   1a2. return to 2.
1b. no settings passed in or saved in prefs
   1b1. get default settings from printer
   1b2. call content API to create preview with settings
   1b3. return Promise with settings and Promise
5a. settings from printer differ to settings from prefs or passed in
   5a1. call content API to update preview with settings
   5a2. return Promise with settings and Promise

[1] https://searchfox.org/mozilla-central/rev/7ef5cefd0468b8f509efe38e0212de2398f4c8b3/widget/windows/nsDeviceContextSpecWin.cpp#439-453

Comment 1

[Jonathan Watt [:jwatt]]

(In reply to Bob Owen (:bobowen) from comment #0)

I don't know how the "Apply settings to printer" step works for CUPS printers or even if it is possible.

I don't believe it is possible. I expect we need to collect the capabilities of the printer and validate each setting against those manually. But whether the DEVMODE is used to validate/fix settings on Windows or settings are fixed up by manual comparison, that seems like an implementation detail we should abstract over.

Comment 2

[Jonathan Watt [:jwatt]]

One situation we need to cater for that wouldn't be provided for with this API is when the user has the "Save to PDF" printer selected, but then clicks on "Print using system dialog...". In that case we want to copy as many settings as possible (validating them) onto a new settings object that has its printer name set to the name of a system recognized printer (the system dialog doesn't know what to do with the "Save to PDF" printer). (We do not want to copy the printer name, obiously.) For that, it seem like we probably want to add something like the following to the nsIPrintSettings interface:

// Copies all settings that are valid for this settings object's printer.
// Does not copy the printer name.
Promise copyFromWithValidation(in nsIPrintSettings);

Since we need that API (I think), it seems like it could also be used in the more general case that comment 0 aims to address.

So in the case of the user having never printed to the printer before (no settings saved to prefs) the frontend code would get an nsIPrinter and just pass the settings returned by createDefaultSettings to FrameLoader.print/.printPreview().

In the case the user has used the printer before, the frontend code would get a new settings object, initialize it with initPrintSettingsFromPrefs, immediately dispatch it to FrameLoader.print/.printPreview(), but for now keeping the UI disabled. It would also create a second print settings object and call copyFromWithValidation() on it, passing the the initialized-from-prefs settings object. Once the returend Promise resolved, it would compare the two settings objects for differences (we should add API for that too). If there are no differences, it would simply remove the "creating preview" spinner and enable the UI. In the unlikely event there were differences, it would dispatch a second FrameLoader.print/.printPreview() call with the validated preferences and wait for that Promise to resolve before enabling the UI.

In the rare case where we have to dispatch fixed up settings, essentially we'd behave in the same way we do now, and have the same performance characteristics. In the common case where there are no differences and we don't need to regenerate the print doc with new settings then the content process would have been laying out the doc in parallel to the parent process validating settings. That would make things much faster for users for whom getting printer capabilities is very slow.

This approach would also have the advantage on not complicating the printPreview() API.

Thoughts?

Comment 3

[Jonathan Watt [:jwatt]]

As Bob just pointed out to me, the API wouldn't be on nsIPrintSettings. It would be on nsIPrinter, and be more like:

Promise copySettingsWithValidaton(fromSettings);

Comment 4

[Bob Owen (:bobowen)]

Attachment: Bug 1669149 p1: Introduce nsIPrintSettings::EquivalentTo and nsIPrinter::CopyFromWithValidation. r=jwatt

This is to allow settings saved in prefs to be validated against a printer and
for print preview to be updated if any changes affecting layout occur.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Bug 1669149 p3: Never go back to the parent for print settings with tab modal print UI. r=jwatt!

Depends on D98458

Comment 6

[Bob Owen (:bobowen)]

Attachment: Bug 1669149 p4: Change unwriteable margin pref to be stored in twips. r=jwatt!

This means that more precise values can be stored, so that they match what we
actually retrieve from the printer and use in layout.

Depends on D99806

Comment 7

[Bob Owen (:bobowen)]

Attachment: Bug 1669149 p5: Make nsPrintSettingsService::ReadPrefs return NS_ERROR_NOT_AVAILABLE, not NS_OK, if no prefs are read. r=jwatt!

This allows us to detect if any prefs were read in JavaScript.

Depends on D99807

Comment 8

[Bob Owen (:bobowen)]

Attachment: Bug 1669149 p6: Change print settings stored as floats to be stored as doubles. r=jwatt!

This more precisely matches what we get from the printer and use in layout.

Depends on D99808

Comment 9

[Bob Owen (:bobowen)]

Attachment: Bug 1669149 p7: Always use size from device context for print paper size on Windows. r=jwatt!

This is always available and makes it more consistent for comparing print
settings.
Also change to round when converting back to DEVMODE for better accuracy.

Depends on D99810

Comment 10

[Bob Owen (:bobowen)]

Attachment: Bug 1669149 p7: Use settings stored in prefs for first preview if available. r=mstriemer!,jwatt

The settings are then validated against the printer and if changes affecting layout occur a new preview is generated.

Depends on D99811

Comment 11

[Bob Owen (:bobowen)]

Attachment: Bug 1669149 p2: Update the printer's DefaultSettings with the global settings. r=jwatt!

Depends on D98458

Comment 12

[Bob Owen (:bobowen)]

Finally got a green try push, one with just browser chrome:
https://treeherder.mozilla.org/jobs?repo=try&duplicate_jobs=visible&revision=42720467ea7ee0c5ae3a4bd673f7919991d8b5d8

Previous auto one that had browser chrome print test failures:
https://treeherder.mozilla.org/jobs?repo=try&duplicate_jobs=visible&revision=d243bd3033adfdc8e325ab0d3988d017dfebe00b

Comment 13

[Mark Striemer [:mstriemer]]

Attachment: Bug 1669149 p7: Start print preview early and validate settings

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f7c6dff65fc
p1: Introduce nsIPrintSettings::EquivalentTo and nsIPrinter::CopyFromWithValidation. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/6e70617cbb79
p2: Update the printer's DefaultSettings with the global settings. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/1fb0f7dc84a9
p3: Never go back to the parent for print settings with tab modal print UI. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/a969b3be9e10
p4: Change unwriteable margin pref to be stored in twips. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/7cc7109100a6
p5: Make nsPrintSettingsService::ReadPrefs return NS_ERROR_NOT_AVAILABLE, not NS_OK, if no prefs are read. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/9cd220f0da60
p6: Change print settings stored as floats to be stored as doubles. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/2f40a908044e
p7: Always use size from device context for print paper size on Windows. r=jwatt

Comment 15

[Jonathan Watt [:jwatt]]

I've landed all the platform work as parts 1-7 (previously 1-6, but I renumbered part 1.5 before landing) and will split out the final frontend patch into a separate bug, and wait at least a Nightly release before landing it. That should prevent the platform patches from bitrotting again in the event that there are issues getting the frontend patch reviewed, and help with any potential regression tracking.

Comment 16

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/2f7c6dff65fc
https://hg.mozilla.org/mozilla-central/rev/6e70617cbb79
https://hg.mozilla.org/mozilla-central/rev/1fb0f7dc84a9
https://hg.mozilla.org/mozilla-central/rev/a969b3be9e10
https://hg.mozilla.org/mozilla-central/rev/7cc7109100a6
https://hg.mozilla.org/mozilla-central/rev/9cd220f0da60
https://hg.mozilla.org/mozilla-central/rev/2f40a908044e