1660527 - Convert the frontend code to nsIPrinter.createDefaultSettings

Status: RESOLVED FIXED

(Toolkit :: Printing, task, P1)

Description

[Jonathan Watt [:jwatt]]

Bug 1658300 and bug 1658299 have now landed.

The new UI is currently relying on code that calls nsIPrintSettingsService.initPrintSettingsFromPrinter to get its print settings object. There are a couple of issues with this API:

1. On macOS and Linux this function does basically nothing much and so the nsIPrintSettings object does not get lots of its properties set with sensible, printer specific defaults (hence the preview being displayed with zero margins for printers that don't support that, etc.).

2. The call is a blocking call, and would be better done as a background task.

The new nsIPrinterList.getNamedOrDefaultPrinter and nsIPrinter.createDefaultSettings APIs allow the frontend code to now be written along the lines of the following (plus caching, etc.):

  let lastUsedPrinterName = printSettingsService.lastUsedPrinterName;

  let printer = await printerList.getNamedOrDefaultPrinter(lastUsedPrinterName);
  if (!printer) {
    printer = getPseudoMozillaSaveToPDFPrinter();
  }

  // In the future we should just use the saved settings, if present, and avoid
  // waiting for the defaults to be fetched asynchronously.  Unfortunately for
  // now we need to always get the defaults and overwrite them with any
  // saved settings.
  let settings = await printer.createDefaultSettings();
  printSettingsService.initPrintSettingsFromPrefs(settings, /* aUsePrinterNamePrefix */ true, settings.kInitSaveAll);

  // Now that we have our initial settings, send them to the content process
  // ASAP to start the print preview creation since this can take a long time.
  // All other work should happen after this point so that the parent and
  // content process can be doing their work in parallel.
  browser.print(settings);

  this.populateUIFormFieldsWithCurrentSettings(settings);
  this.updateUIWithAvailablePrinterSettings(printer);

  // Get other available printers:
  let availablePrinters = await printerList.printers;
  for (let printer of availablePrinters) {
    // Add name to printer dropdown
  }

Comment 1

[Jonathan Watt [:jwatt]]

Please also be aware of nsIPrintSettings.clone() (and nsIPrintSettings.assign()). That should(!) allow you to cache a copy of the default settings you fetch with createDefaultSettings() that you leave untouched, and then whenever you need a new settings object initialized with the printer's defaults you'd .clone() that vanilla settings object and then add settings saved to prefs, settings changed by the user in the UI, etc. to the clone.

Comment 2

[Emma Malysz]

From what I can tell, using .clone() on Mac causes the app to crash

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Emma Malysz from comment #2)

From what I can tell, using .clone() on Mac causes the app to crash

That's annoying. jfkthame, any chance you might be able to take a look at that (specifically for the default settings returned by createDefaultSettings called on an nsIPrinter for a real printer returned by nsIPrinterList.getNamedPrinter)?

Comment 4

[Jonathan Kew [:jfkthame]]

Ouch. I can try to look tomorrow; won't have time to dig into it tonight I'm afraid.

Comment 5

[Jonathan Watt [:jwatt]]

No worries.

The following (with the printer name replaced with a valid printer for the system) will repro the crash when pasted into the Browser Console:

async function crash() {
  const printerList = Cc["@mozilla.org/gfx/printerlist;1"].createInstance(Ci.nsIPrinterList);
  let printer = await printerList.getNamedPrinter("Brother MFC-L8690CDW series");
  printer.QueryInterface(Ci.nsIPrinter);
  let settings = await printer.createDefaultSettings();
  settings.QueryInterface(Ci.nsIPrintSettings);
  return settings.clone();
}
let clone;
crash().then(c => clone = c);

Comment 6

[Jonathan Watt [:jwatt]]

Basically this is broken:

nsPrintSettingsX::nsPrintSettingsX(const nsPrintSettingsX& src) { *this = src; }

since nsPrintSettingsX::operator=(const nsPrintSettingsX& rhs) can't be called on an uninitialized object since it calls [mPrintInfo release].

Comment 7

[Jonathan Watt [:jwatt]]

The mac fix is building on autoland. I'm not sure if you can make use of that, but mentioning it just in case:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4f7e328f6ad54dc623eed7c49b36c6f8aed4d1f7

Comment 8

[Jonathan Watt [:jwatt]]

I audited the .clone() code and found more bugs. I've landed the fixes for those in bug 1660918.

Comment 9

[Jonathan Watt [:jwatt]]

Both fixes are now on m-c.

Comment 10

[Jonathan Watt [:jwatt]]

As per the conversation on the call we just had, let's forget about the getNamedOrDefaultPrinter part of this for now. (I'll think about it some more and file another bug if necessary.) That optimization isn't essential, but getting the code converted to use nsIPrinter.createDefaultSettings intead of initPrintSettingsFromPrinter is. So let's use this bug to just focus on that.

Comment 11

[Bob Owen (:bobowen)]

Just for some extra context here.
We've found when adding telemetry in bug 1660686, that the use of initPrintSettingsFromPrinter is negating some of the benefits of pushing other system API usage to background threads.
i.e. initPrintSettingsFromPrinter is blocking them from running their promise resolve/reject part back on the main thread.

Comment 12

[Sam Foster [:sfoster] (he/him)]

I plan on leaving the initPrintSettingsFromPrinter stuff in PrintUtils.js as-is and use createDefaultSettings from within print.js for the new UI only.

Comment 13

[Jonathan Watt [:jwatt]]

Perfect. Trying to change the old UI would just introduce unnecessary risk.

Comment 14

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1660527 - Use printer.createDefaultSettings to get default and user settings for each printer. r?jwatt,emalysz!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12b9a9eaa010
Use printer.createDefaultSettings to get default and user settings for each printer. r=jwatt,emalysz

Comment 16

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/12b9a9eaa010

Comment 17

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9173514 [details]
Bug 1660527 - Use printer.createDefaultSettings to get default and user settings for each printer. r?jwatt,emalysz!

Beta/Release Uplift Approval Request

• User impact if declined: A necessary part of the new print UI implementation
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1662769
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The old print experience in printUtils.js is not affected by this patch. The new is held behind a pref.
• String changes made/needed: NOne

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9173514 [details]
Bug 1660527 - Use printer.createDefaultSettings to get default and user settings for each printer. r?jwatt,emalysz!

Approved for 81.0b6.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9173514 [details]
Bug 1660527 - Use printer.createDefaultSettings to get default and user settings for each printer. r?jwatt,emalysz!

Actually, this has conflicts with bug 1653340. Please post a rebased patch.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #17)

• List of other uplifts needed: Bug 1662769

This needs an approval request in the other bug too.

Comment 21

[Sam Foster [:sfoster] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)

Comment on attachment 9173514 [details]
Bug 1660527 - Use printer.createDefaultSettings to get default and user settings for each printer. r?jwatt,emalysz!

Actually, this has conflicts with bug 1653340. Please post a rebased patch.

The conflicts are because we didn't uplift Bug 1653340 yet. I've requested uplift on that one, and blocked this on it.

Comment 22

[Sam Foster [:sfoster] (he/him)]

Attachment: bug-1660527-beta.patch

If we need to uplift without Bug 1653340, we can use this patch. (Note the print.js changes in Bug 1653340 will then a corresponding similar change if that lands on beta later). Specifically, 1653340 adds a couple of lines to print.js#refreshSettings: printerInfo.settings.isInitializedFromPrinter = true. I change those same lines in this patch, moving the isInitializedFromPrinter stuff into resolvePropertiesForPrinter and updating how we get this.settings and this.defaultSettings in refreshSettings to use the return value from resolvePropertiesForPrinter.

Comment 23

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9173810 [details] [diff] [review]
bug-1660527-beta.patch

Beta/Release Uplift Approval Request

• User impact if declined: A necessary part of the new print UI implementation
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1662769
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The old print experience in printUtils.js is not affected by this patch. The new is held behind a pref.
• String changes made/needed: None

Comment 24

[Sam Foster [:sfoster] (he/him)]

I've gone ahead and requested uplift for the beta-specific patch. The version that landed on central has the patches on bug 1653340 as ancestors. As those have not been uplifted, I've adjusted it in attachment 9173810 [details] [diff] [review] to merge without. I've built beta locally and done a smoke test to check the print UI works. If it turns out the patches on bug 1653340 are going to uplift into the same beta build, it would make more sense to land in the same sequence as moz-central and use attachment 9173514 [details] instead.

Comment 25

[Jonathan Watt [:jwatt]]

Comment on attachment 9173514 [details]
Bug 1660527 - Use printer.createDefaultSettings to get default and user settings for each printer. r?jwatt,emalysz!

Beta/Release Uplift Approval Request

• User impact if declined: Missing/slow getting settings for printing.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9173514 [details]
Bug 1660527 - Use printer.createDefaultSettings to get default and user settings for each printer. r?jwatt,emalysz!

Approved for 81.0b6.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f3bd1cca7420