Closed Bug 1379117 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: Print Preview, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox57 --- wontfix
firefox60 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
Priority: -- → P3
Any plans to do this now, Bob?
Flags: needinfo?(bobowencode)
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(bobowencode)
Priority: P3 → P1
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.
Attachment #8949118 - Flags: review?(jwatt)
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.
Attachment #8949119 - Flags: review?(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.
Attachment #8949118 - Flags: review?(jwatt) → review+
Attachment #8949119 - Flags: review?(jwatt) → review+
(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
Pushed by bobowencode@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/806f9f8166be
https://hg.mozilla.org/mozilla-central/rev/58996b7409fc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.