Closed Bug 1089815 Opened 10 years ago Closed 10 years ago

Printing and print preview from view source will be broken when bug 1082575 lands

Categories

(Toolkit :: Printing, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m5+ ---
firefox36 + fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1082575 is critical to land for printing in e10s to start to work. Unfortunately, this seems to break printing from view source.

For print preview, I seem to be unable to get the messageManager for the print preview browser - strangely, the message manager on the print preview browser is null.

When printing, the exception "Windows running with remote tabs must explicitly pass a content window to PrintUtils.print" is being thrown. Unsure why the view source dialog things its running with remote tabs.
[Tracking Requested - why for this release]:

Bug 1082575 is going to land soon, which will cause this regression on trunk (36).
Assignee: nobody → mconley
<xul:browser>'s do not get messageManager's constructed for them unless they
have their type attribute set to "content", "content-targetable", or
"content-primary". This patch updates the view source print preview browser
with that type attribute, and also updates the PrintUtils documentation to
mention this requirement.
Comment on attachment 8516790 [details] [diff] [review]
View source print preview browser does not have a message manager. r=?

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

<xul:browser>'s without type="content[-targetable|-primary]" don't get message managers, it seems. The old view source print preview browser never got that attribute assigned, so that's why it didn't work.

This patch adds type="content" to the view source print preview browser, and updates the PrintUtils documentation.
Attachment #8516790 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8516790 [details] [diff] [review]
View source print preview browser does not have a message manager. r=?

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

Wonder why that is.
Attachment #8516790 - Flags: review?(dtownsend+bugmail) → review+
It's because mIsTopLevelContent in nsFrameLoader is set to the return value of nsFrameLoader::AddTreeItemToTreeOwner, which only returns true if the type attribute starts with "content"[1].

mIsTopLevelContent is then checked in nsFrameLoader::EnsureMessageManager, and we return early if !mIsTopLevelContent and not a b2g app/browser, and not a remote browser[2].

Not entirely sure why that restriction is there... smaug, do you know?

Thanks for the review!

[1]: http://hg.mozilla.org/mozilla-central/file/54d05732f29b/dom/base/nsFrameLoader.cpp#l668
[2]: http://hg.mozilla.org/mozilla-central/file/54d05732f29b/dom/base/nsFrameLoader.cpp#l2366
Flags: needinfo?(bugs)
We certainly don't want message manager for all the <browser>s. IIRC that would break sidebar for example.
Flags: needinfo?(bugs)
I'm confused - how would it break the sidebar browser? It'd just get a messageManger... I don't see what else would be different. What am I missing?
Flags: needinfo?(bugs)
You'd get the frame scripts loaded which are meant for content tabs (assuming window level mm was used for loading the scripts). That was at least the original issue, years ago, IIRC.
Flags: needinfo?(bugs)
Ah. This might no longer be an issue with the "messagemanagergroup" which seems to allow for greater targeting.

But thanks for clearing it up, smaug. :)
https://hg.mozilla.org/mozilla-central/rev/2e64d4b84555
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: