Closed
Bug 1379117
Opened 7 years ago
Closed 7 years ago
PrintPreview should only be allowed on nsIWebBrowserPrint when retrieved through the docshell.
Categories
(Core :: Print Preview, enhancement, P1)
Core
Print Preview
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: bobowen, Assigned: bobowen)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
9.24 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
6.11 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
Blocks: post-57-api-changes
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
Flags: needinfo?(bobowencode)
Priority: P3 → P1
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
||
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 5•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8949119 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 6•7 years 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
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
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/806f9f8166be
https://hg.mozilla.org/mozilla-central/rev/58996b7409fc
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•