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

RESOLVED FIXED in Firefox 60

Status

()

P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

(Blocks: 1 bug)

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox60 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1347507
Priority: -- → P3
status-firefox57: --- → wontfix
Any plans to do this now, Bob?
Flags: needinfo?(bobowencode)
(Assignee)

Updated

a year ago
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(bobowencode)
Priority: P3 → P1
(Assignee)

Comment 3

a year ago
Created attachment 8949118 [details] [diff] [review]
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.
Attachment #8949118 - Flags: review?(jwatt)
(Assignee)

Comment 4

a year ago
Created attachment 8949119 [details] [diff] [review]
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.
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+
(Assignee)

Comment 6

a year ago
(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

a year ago
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
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.