When print preview settings are changed, the frontend calls through to nsPrintJob::DoCommonPrint with a new nsIPrintSettings object where we clone the current print preview document to create a new print preview document.

To support printing for Fission, we very much want to make it a requirement that each clone requires a new window, in which this cloning behavior for print preview changes becomes a major pain for the frontend. It would then need to add a bunch of complicated code to create (potentially a series of) hidden windows/tabs.

To meet the requirements for Fission and avoid complicating the frontend code, it would be much better if settings changes in print preview could just make the backend re-layout the print preview clone doc with the new settings.

The "simplified" print preview mode is worth remarking on here. It is created by the frontend creating a new whole-window tab, serialize the source document, using ReaderMode.jsm to parse that serialization, then inserting it into the DOM of the about:blank page that the new tab is initiated with.

Hiro, would this bug be interesting to you to work on? Feel free to ping me on Matrix and we can discuss what's involved.

Yep, I will take a look this.

:jwatt, the Document object has the latest cloned document, so using the latest cloned document would be sufficient? I don't yet understand how recloning the document affects in fission so I am not sure for the case, but quick testing on print preview using the latest cloned document works properly.

To expand on the first paragraph in comment 0 (the other two paragraphs were just background):

Whenever the user uses the print preview UI to change settings, that triggers code in printPreviewToolbar.js that saves the settings to prefs, then calls PrintUtils.printPreview() which ends up calling nsPrintJob::PrintPreview. That currently reinitializes print preview with the new settings by throwing away the existing print preview document and creating another one. That happens in nsPrintObject::InitAsRootObject. When a repeat nsPrintJob::PrintPreview call is made (i.e. the nsPrintJob has already been set up for a print preview) we know that we're just experiencing a settings change, and we want to avoid throwing away the document.

Basically this bug is mainly about trying to figure out how to avoid the CreateStaticClone call in nsPrintObject::InitAsRootObject. But in fact we probably want to avoid recreating a new nsPrintData and tree of nsPrintObject objects etc. too. We "just" want to apply the new nsIPrintSettings by "only" recreating the presentation in the print preview.

This is definitely easier said than done. The setup part of nsPrintJob is less of a sewer than it was, but it's still quite a mess and disentangling things may be a headache I'm afraid. :( Several further refactorings to make this code more sane may be required before we can fix this bug. For example, I'm working on bug 1638162.

Hmm I think I understand part of them.

(In reply to Jonathan Watt [:jwatt] from comment #5)

Basically this bug is mainly about trying to figure out how to avoid the CreateStaticClone call in nsPrintObject::InitAsRootObject.

To me, we should do the CreateStaticClone in InitAsRootObject, the InitAsRootObject, as it stands for, it's for the initial setup for print/print-preview, so it should call CreateStaticClone, as far as I can tell.

This is definitely easier said than done. The setup part of nsPrintJob is less of a sewer than it was, but it's still quite a mess and disentangling things may be a headache I'm afraid. :( Several further refactorings to make this code more sane may be required before we can fix this bug. For example, I'm working on bug 1638162.

Yeah, bug 1638162 is quite understandable to me. I assume you are trying to remove a bunch of prerequisite setup from DoCommonPrint in bug 1638162 right? I am also presuming that we'd want to stop calling DoCommonPrint when user changes the print setting or some such, right? We could expose setPrintSetting() APIor some such in JS, I assume.

Anyway, a big messy thing in printing stuff is that DoCommonPrint does a bunch of stuff something like a gigantic main function. :/

Setting to v81 to reflect that it's not absolutely necessary for 80 as part of the print UX support.

It may not be absolutely necessary for the new printing UI, but for larger documents like the HTML5 spec, generating the print preview document takes a very long time. As long as we have to regenerate the document for every settings change the user makes in the print preview UI, then changing settings are intollerably slow for some set of prints.

While this is already the case for our current print preview implementation, the issue is more severe for the new UI:

1. currently only 7.5% of prints are done via print preview, but if we change that to 100% by exposing the new UI, then this issue will be encounterd during many more prints
2. the new print UI exposes a lot more settings than the current print preview UI, presumably making it more likely that users will change settings before printing, and more likely to encounter this issue

This centralizes our print and preview setup in nsGlobalWindowOuter so
that we never re-clone a clone, and so that we reuse the window.open()
codepath to create the browsing context to clone into.

For window.print, for both old print dialog / silent printing and new
print preview UI, we now create a hidden browser (as in with visibility:
collapse, which takes no space but still gets a layout box).

• In the modern UI case, this browser is swapped with the actual print
  preview clone, and the UI takes care of removing the browser.

• In the print dialog / silent printing case, the printing code calls
  window.close() from nsDocumentViewer::OnDonePrinting().

• We don't need to care about the old print preview UI for this case
  because it can't be open from window.print().

We need to fall back to an actual window when there's no
nsIBrowserDOMWindow around for WPT print tests and the like, which don't
have one. That seems fine, we could special-case this code path more if
needed but it doesn't seem worth it.

Other engines also do this, but with my previous patch breaks it
(because we only hit print() on the print-content-viewer after doing
the clone).

So move it before triggering all the machinery, and only for
window.print(). Given we didn't check this for print preview etc, I
think it's fine to carry on for user-triggered loads.

Trivial test-case (which I'm not quite sure how to turn into an
automated test...)

<!doctype html>
<h1>I do get printed but...</h1>

<script>
  window.print();
</script>

<h2>Do I?</h2>

Note that this is broken with the new print preview UI already, this
fixes it.

Depends on D87063

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b1f9290da1b8f5844cb1a955f35dc06ba304934 is a try run, and two test-fixes for the non-intermittent orange there incoming.

This crashtest opens a gazillion popups via window.open() which then it
never closes.

This becomes a problem because other better-behaved crashtests in the
same subdirectory get their popups blocked by the popup blocker and then
time out when not removing the reftest-wait class, like this:

https://searchfox.org/mozilla-central/source/dom/base/crashtests/1529203-2.html#17

This didn't happen before the first patch of this bug, because we were
aborting when creating these windows here, and after my patches we fall
back to opening in a new window:

https://searchfox.org/mozilla-central/rev/3c98465c9d329625c4e1b22a7b832aabeafc4cc3/dom/ipc/ContentParent.cpp#5027-5030

I could restore the abort if preferred, but I don't think that's great.

The first patch in this bug makes this test a bit more heavy-weight.

Instead of rendering the print-preview document in an iframe, and
snapshotting the 400x400 pixels in the top left corner, we open a window
and snapshot the whole print preview document.

I could keep it doing the same if needed by passing the docshell
argument to window.printPreview. But this test wasn't great to begin
with, and the changes in part 1 are a net improvement imo.

The bad part is that they cause the test to become slower such as it can
time out in some debug builds. Request a longer timeout because of this.

Depends on D87965

https://hg.mozilla.org/mozilla-central/rev/eb4cab0cf634
https://hg.mozilla.org/mozilla-central/rev/0ed1a41f4549

https://hg.mozilla.org/mozilla-central/rev/93e3344dd1f7
https://hg.mozilla.org/mozilla-central/rev/0c1847dee730
https://hg.mozilla.org/mozilla-central/rev/15e57226a195
https://hg.mozilla.org/mozilla-central/rev/9b7c7000bc9b

Can you request Beta uplift approval for the patches other than the main patch? (Those that can be uplifted without it.) I guess we want to leave the main patch for another half day or so to bake on Nightly before uplifting it?

I don't think there's a lot of value on uplifting only the test fixes. The other patches yeah, I want to fix regressions as needed before deciding to uplift.

Comment on attachment 9171456 [details]
Bug 1636728 - Centralize printing entry points in nsGlobalWindowOuter, and move cloning out of nsPrintJob. r=nika!,jwatt!

Beta/Release Uplift Approval Request

• User impact if declined: Changing settings in the new printing UI will be much slower.
• 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: High
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment on attachment 9171456 [details]
Bug 1636728 - Centralize printing entry points in nsGlobalWindowOuter, and move cloning out of nsPrintJob. r=nika!,jwatt!

Approved for 81.0b6.

https://hg.mozilla.org/releases/mozilla-beta/rev/586fdb9a133a
https://hg.mozilla.org/releases/mozilla-beta/rev/7fd9e8573f5b
https://hg.mozilla.org/releases/mozilla-beta/rev/0a778fdadc7c
https://hg.mozilla.org/releases/mozilla-beta/rev/39a640589d59
https://hg.mozilla.org/releases/mozilla-beta/rev/60c51c0882e1
https://hg.mozilla.org/releases/mozilla-beta/rev/9665388ac7c6