1379117 - PrintPreview should only be allowed on nsIWebBrowserPrint when retrieved through the docshell.

Status: RESOLVED FIXED

(Core :: Print Preview, enhancement, P1)

Description

[Bob Owen (:bobowen)]

Once we get rid of non-WebExtensions addons we should be able to turn the warning at [1] into a full assertion.

We generally only hit this when we have retrieved the nsIWebBrowserPrint interface through nsGlobalWindow and not nsDocShell::GetPrintPreview.
It's possible that this is currently used by addons, but not after Fx57.

There are at least two tests that also do this:
layout/base/tests/chrome/test_printpreview_bug396024.xul
layout/base/tests/chrome/test_printpreview_bug482976.xul

So, these should be changed to use the docshell or otherwise rewritten.

[1] https://hg.mozilla.org/mozilla-central/file/a0b5515b13eb/layout/base/nsDocumentViewer.cpp#l4050

Comment 1

[Jonathan Watt [:jwatt]]

Any plans to do this now, Bob?

Comment 2

[Bob Owen (:bobowen)]

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

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 1: MOZ_ASSERT when not retrieving nsIWebBrowserPrint through docshell for Print Preview

Changing the last retrieval of the nsIWebBrowserPrint in
printpreview_bug396024_helper.xul caused problems because doing it via the
docshell interrupts the load that triggers run5.
So, I moved the test to run5, which is more consistent with run2 and run3.
I then realised that the frames would have switched anyway, so I changed it to
retrieve frame[0] for the last test, which I think makes more sense.
It's not totally clear to me what this was testing originally or whether it
continued to do so after the addition of the second iframe a long time ago.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 2: Remove some deprecated PrintUtils functions

I noticed these while looking for other things that were gettting an
nsIWebBrowserPrint for print preview. I think these must have been left for
legacy addons and they don't seem to be used, so I think they can go.

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 8949118 [details] [diff] [review]
Part 1: MOZ_ASSERT when not retrieving nsIWebBrowserPrint through docshell for Print Preview

Review of attachment 8949118 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDocumentViewer.cpp
@@ +3980,1 @@
>      "Using docshell.printPreview is the preferred way for print previewing!");

Seems like "preferred" may not be the most appropriate term any more if we're now MOZ_ASSERT'ing.

Comment 6

[Bob Owen (:bobowen)]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #5)
...
> >      "Using docshell.printPreview is the preferred way for print previewing!");
> 
> Seems like "preferred" may not be the most appropriate term any more if
> we're now MOZ_ASSERT'ing.

Good point, thanks. Changed to:
  "For print preview nsIWebBrowserPrint must be from docshell.printPreview!"

Also, pushed again to try, because I picked up some other thing that seemed to be perma-failing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17cd0c71ae746bc6b8acf37be95717885046eaa6

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/806f9f8166be
Part 1: MOZ_ASSERT when not retrieving nsIWebBrowserPrint through docshell for Print Preview. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/58996b7409fc
Part 2: Remove some deprecated PrintUtils functions. r=jwatt

Comment 8

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/806f9f8166be
https://hg.mozilla.org/mozilla-central/rev/58996b7409fc