Closed
Bug 544018
Opened 15 years ago
Closed 15 years ago
print preview doesn't work in view source
Categories
(Core :: Print Preview, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(2 files, 6 obsolete files)
|
(deleted),
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
Details | Diff | Splinter Review |
I made a silly mistake to mix toolkit and browser code in bug 487667.
Fixing that here.
printutils requires now two more methods to be implemented.
That way browser.js can open a new tab for print preview, and
viewsource can lazily create a browser for the print preview.
This patch simplifies the code and makes it more re-usable.
The changes to printUtils.js are need also because of TB and SM.
I am sorry about not really thinking about this before.
Clone-doc-for-printing patch was big and important enough as such,
so I focused on getting the most commonly used UI working.
There is also a bug that autohide tabs become visible after printpreview.
That is fixed by the maybeShowStrip code.
Trivial change to nsDocument.cpp to make more things cycle collected.
Attachment #425008 -
Flags: review?(dao)
|
||
Comment 1•15 years ago
|
||
Comment on attachment 425008 [details] [diff] [review]
patch
> gBrowser.setStripVisibilityTo(false);
>- if (this._chromeState.hadTabStrip)
>- gBrowser.setStripVisibilityTo(true);
>+ gBrowser.maybeShowStrip();
>+ <method name="maybeShowStrip">
>+ <body>
>+ <![CDATA[
>+ if (this.mStrip.collapsed &&
>+ ((this.mTabs.length - this._removingTabs.length > 1) ||
>+ !this.mPrefs.getBoolPref("browser.tabs.autoHide")))
>+ this.setStripVisibilityTo(true);
>+ ]]>
>+ </body>
>+ </method>
Instead of this, you can do gBrowser.mStrip.setAttribute("moz-collapsed", true/false) like FullScreen.mouseoverToggle does it... no new API needed.
>+ // calling PrintUtils.printPreview() requires that you have five functions
>+ // in the global scope: getBrowserToPP(), which returns the browser for which
>+ // the print preview should be created, getPPBrowser(), which returns the browser
>+ // element in the window print preview uses, doneWithPPBrowser() which is called
>+ // when print preview browser isn't needed anymore, getNavToolbox(),
>+ // which returns the element (usually the main toolbox element) before which the
>+ // print preview toolbar should be inserted, and getWebNavigation(), which returns
>+ // the document's nsIWebNavigation object
Rather than messing with the global scope, can these be callbacks on the PrintPreviewListener object that browser.js passes to PrintUtils.printPreview?
Since we're breaking consumers anyway, this seems like a good opportunity to do this.
|
Assignee | |
Comment 2•15 years ago
|
||
(In reply to comment #1)
> Instead of this, you can do gBrowser.mStrip.setAttribute("moz-collapsed",
> true/false) like FullScreen.mouseoverToggle does it... no new API needed.
Ah, will do.
> Rather than messing with the global scope, can these be callbacks on the
> PrintPreviewListener object that browser.js passes to PrintUtils.printPreview?
> Since we're breaking consumers anyway, this seems like a good opportunity to do
> this.
Yeah, sounds good.
|
|
||
Updated•15 years ago
|
Attachment #425008 -
Flags: review?(dao)
|
|
Assignee | |
Comment 3•15 years ago
|
||
Remove the requirement for global functions.
Attachment #425008 -
Attachment is obsolete: true
Attachment #425309 -
Flags: review?(dao)
|
|
||
Comment 4•15 years ago
|
||
Comment on attachment 425309 [details] [diff] [review]
patch
> var PrintPreviewListener = {
>+ _printPreviewTab: null,
>+ _tabBeforePrintPreview: null,
>+
>+ getPPBrowser: function () {
Since there's already a host object, please remove PP from the method names.
>+ if (!this._printPreviewTab) {
>+ this._tabBeforePrintPreview = gBrowser.mCurrentTab;
s/mCurrentTab/selectedTab/
>+ this._printPreviewTab = gBrowser.loadOneTab("about:blank", null, null,
>+ null, true, false);
>+ gBrowser.selectedTab = this._printPreviewTab;
this._printPreviewTab = gBrowser.loadOneTab("about:blank", {inBackground: false});
>+ doneWithPPBrowser: function () {
>+ gBrowser.removeTab(this._printPreviewTab);
>+ this._printPreviewTab = null;
>+ gBrowser.selectedTab = this._tabBeforePrintPreview;
>+ this._tabBeforePrintPreview = null;
>+ },
>+ getBrowserToPP: function () {
>+ return gBrowser;
>+ },
Apart from "PP", could you find better names for these methods? I really don't understand their current names offhand.
>+ printPreview: function (aListenerOrGetBrowserToPPCallback,
>+ aGetPPBrowserCallback,
>+ aDoneWithPPBrowserCallback,
>+ aGetWebNavigationCallback,
>+ aGetNavToolboxCallback,
>+ aListenerOrEnterCallback,
>+ aExitCallback)
Again, since this breaks existing consumers anyway, no need to maintain the old way of passing the callbacks separately. We should always expect the single object and viewSource.js should implement it just like browser.js, imho.
|
|
||
Comment 5•15 years ago
|
||
Comment on attachment 425309 [details] [diff] [review]
patch
>+ doneWithPPBrowser: function () {
>+ gBrowser.removeTab(this._printPreviewTab);
>+ this._printPreviewTab = null;
>+ gBrowser.selectedTab = this._tabBeforePrintPreview;
>+ this._tabBeforePrintPreview = null;
Setting selectedTab before calling removeTab would be better, as it would prevent removeTab from temporarily selecting some other tab.
|
|
Assignee | |
Comment 6•15 years ago
|
||
(In reply to comment #4)
> (From update of attachment 425309 [details] [diff] [review])
> Again, since this breaks existing consumers anyway, no need to maintain the old
> way of passing the callbacks separately. We should always expect the single
> object and viewSource.js should implement it just like browser.js, imho.
Yeah, I was thinking about that, but wanted to keep the changes as small as
possible.
Will post a new patch tomorrow, I hope.
|
|
Assignee | |
Comment 7•15 years ago
|
||
I expanded the method names. Hopefully they are easier to understand now.
The idea is that there is a browser which wants to be print previewed and
there is another browser which shows the print preview.
If we're already showing print preview, and settings are changed, we want to
re-print-preview the current print preview browser.
Attachment #425309 -
Attachment is obsolete: true
Attachment #425443 -
Flags: review?(dao)
Attachment #425309 -
Flags: review?(dao)
|
|
||
Comment 8•15 years ago
|
||
Comment on attachment 425443 [details] [diff] [review]
patch
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>+ this._printPreviewTab = gBrowser.loadOneTab("about:blank", { inBackground: false});
Either add a space before } or remove the one after { ;-)
>+ getBrowserToPrintPreview: function () {
>+ return gBrowser;
>+ },
May I suggest getSourceBrowser as the name for this?
>+ getWebNavigation: function () {
>+ return window.getWebNavigation();
>+ },
Is this actually needed? Could printUtils.js use getSourceBrowser().webNavigation instead?
>--- a/toolkit/components/viewsource/content/viewSource.js
>+++ b/toolkit/components/viewsource/content/viewSource.js
>+ getBrowserToPrintPreview: function () {
>+ var browser = document.getElementById("ppBrowser");
>+ return browser && !browser.collapsed ? browser : gBrowser;
>+ },
I don't understand this. Shouldn't this always return gBrowser?
|
|
Assignee | |
Comment 9•15 years ago
|
||
(In reply to comment #8)
> I don't understand this. Shouldn't this always return gBrowser?
We want to re-create the print preview presentation based on the already
print previewed presentation. In view-source that doesn't really matter, unless
someone somehow manages to load a new view-source document.
> Either add a space before } or remove the one after { ;-)
Sure, will do
> May I suggest getSourceBrowser as the name for this?
If that sounds better to you, will do.
> Is this actually needed? Could printUtils.js use
> getSourceBrowser().webNavigation instead?
Right, that should work, and would be even more correct.
I won't be able to update the patch before Monday morning.
(Traveling and I have only a mac with me :/ )
Do you want to still review the final patch, or do I get r+ for the current patch if comments fixed? :)
|
|
||
Comment 10•15 years ago
|
||
(In reply to comment #9)
> We want to re-create the print preview presentation based on the already
> print previewed presentation. In view-source that doesn't really matter, unless
> someone somehow manages to load a new view-source document.
Shouldn't printUtils.js call getPrintPreviewBrowser in that case?
|
|
Assignee | |
Comment 11•15 years ago
|
||
Could do it that way too, sure.
|
|
Assignee | |
Comment 12•15 years ago
|
||
With tabbrowser that just happens sort of automatically, since
we want to print preview always the currently visible tab (which can be
the print preview tab).
|
|
||
Comment 13•15 years ago
|
||
Comment on attachment 425443 [details] [diff] [review]
patch
>+ getNavToolbox: function () {
>+ return window.getNavToolbox();
return gNavToolbox;
(In reply to comment #12)
> With tabbrowser that just happens sort of automatically, since
> we want to print preview always the currently visible tab (which can be
> the print preview tab).
Yeah, I didn't realize this. I really think printUtils.js should explicitly take care of this. Would it do that based on aCallback being null?
|
|
Assignee | |
Comment 14•15 years ago
|
||
(In reply to comment #13)
> I really think printUtils.js should explicitly
> take care of this.
Ok, I'll change that. Whenever callback is used, I could take the
sourceBrowser and use that, in other case the print preview browser should be used.
|
|
||
Updated•15 years ago
|
Attachment #425443 -
Flags: review?(dao) → review-
|
|
||
Comment 15•15 years ago
|
||
Comment on attachment 425443 [details] [diff] [review]
patch
>+ // If aCallback is not null, it must be an object which has the following methods:
>+ // getPrintPreviewBrowser(), getBrowserToPrintPreview(),
>+ // getWebNavigation(), getNavToolbox(), onEnter() and onExit().
>+ printPreview: function (aCallback)
As I understand it, aCallback should never be null when printPreview is called from the outside (i.e. only toolkit/components/printing/ code should call it without a callback). I think the comment could be clearer about this.
I'd like to take another look at this on Monday, but should be a quick review.
|
|
Assignee | |
Comment 16•15 years ago
|
||
Attachment #425443 -
Attachment is obsolete: true
Attachment #425778 -
Flags: review?(dao)
|
|
||
Comment 17•15 years ago
|
||
Comment on attachment 425778 [details] [diff] [review]
v4
>+++ b/browser/base/content/browser.js
>+ getSourceBrowser: function () {
>+ return gBrowser;
Shouldn't this be this._tabBeforePrintPreview.linkedBrowser || gBrowser.selectedTab, so that getSourceBrowser won't point to the print preview browser depending on when it's called?
>+++ b/toolkit/components/printing/content/printUtils.js
>+ // If aCallback is not null, it must be an object which has the following methods:
>+ // getPrintPreviewBrowser(), getSourceBrowser(),
>+ // getNavToolbox(), onEnter() and onExit().
>+ // If aCallback is null, then printPreview must have been called with non-null aCallback and
>+ // that object will be reused.
s/must have been called/must previously have been called/?
> // collapse the browser here -- it will be shown in
> // onEnterPrintPreview; this forces a reflow which fixes display
> // issues in bug 267422.
Can you adjust this comment? onEnterPrintPreview doesn't exist, I think enterPrintPreview is meant.
>+ if (this._sourceBrowser)
>+ this._sourceBrowser.collapsed = true;
Why the null check?
>- var webNav = getWebNavigation();
>+ var webNav = this._sourceBrowser.webNavigation;
> ppParams.value.docTitle = webNav.document.title;
> ppParams.value.docURL = webNav.currentURI.spec;
You can just use this._sourceBrowser.contentDocument and this._sourceBrowser.currentURI here.
>+++ b/toolkit/components/viewsource/content/viewSource.js
>+var PrintPreviewListener = {
>+ getPrintPreviewBrowser: function () {
>+ var browser = document.getElementById("ppBrowser");
>+ if (!browser) {
>+ const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>+ browser = document.createElementNS(XUL_NS, "browser");
browser = document.createElement("browser");?
|
|
Assignee | |
Comment 18•15 years ago
|
||
Attachment #425778 -
Attachment is obsolete: true
Attachment #425794 -
Flags: review?(dao)
Attachment #425778 -
Flags: review?(dao)
|
|
||
Comment 19•15 years ago
|
||
Comment on attachment 425794 [details] [diff] [review]
v5
>+ getSourceBrowser: function () {
>+ return this._tabBeforePrintPreview ?
>+ this._tabBeforePrintPreview.linkedBrowser :
>+ gBrowser.selectedTab.linkedBrowser;
nit: selectedBrowser instead of selectedTab.linkedBrowser
>+ ppParams.value.docTitle = this._sourceBrowser.contentDocument.title;
>+ ppParams.value.docURL = this._sourceBrowser.currentURI.spec;
In case this._sourceBrowser is the print preview browser, currentURI.spec seems to be about:blank. Is this expected?
|
|
||
Comment 20•15 years ago
|
||
(In reply to comment #19)
> (From update of attachment 425794 [details] [diff] [review])
> >+ getSourceBrowser: function () {
> >+ return this._tabBeforePrintPreview ?
> >+ this._tabBeforePrintPreview.linkedBrowser :
> >+ gBrowser.selectedTab.linkedBrowser;
>
> nit: selectedBrowser instead of selectedTab.linkedBrowser
also, trailing space after '?'
|
|
Assignee | |
Comment 21•15 years ago
|
||
Attachment #425794 -
Attachment is obsolete: true
Attachment #425813 -
Flags: review?(dao)
Attachment #425794 -
Flags: review?(dao)
|
|
||
Updated•15 years ago
|
Attachment #425813 -
Flags: review?(dao) → review+
|
|
Assignee | |
Comment 22•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
|
|
Assignee | |
Comment 23•15 years ago
|
||
Arg, I'm never going to get this simple thing fixed.
There are some tests I need to change.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
|
|
Assignee | |
Comment 24•15 years ago
|
||
Backed out
http://hg.mozilla.org/mozilla-central/rev/4760079804e4
Will push the patch later with the fix for a testcase.
|
|
||
Comment 25•15 years ago
|
||
This looks like a real failure to me at first glance, i.e. it needs a fix in printUtils.js rather than browser_bug386835.js.
|
||
Comment 26•15 years ago
|
||
bummer Dão and Olli. I guess I'll have to wait a little longer for this print preview bug to get fixed. another similar print preview bug mentioned in 543205:
https://bugzilla.mozilla.org/show_bug.cgi?id=543205
|
|
Assignee | |
Comment 27•15 years ago
|
||
(In reply to comment #25)
> This looks like a real failure to me at first glance, i.e. it needs a fix in
> printUtils.js rather than browser_bug386835.js.
Indeed. It was working ok at some point, but then, when I removed the
doneWithPPBrowser I guess, I changed zoom handling.
|
|
Assignee | |
Comment 28•15 years ago
|
||
This removes the zoom handling hacks from printutils.js and makes
FullZoom__applyPrefToSetting to do nothing when browser is in
print preview mode.
Attachment #425813 -
Attachment is obsolete: true
Attachment #425958 -
Flags: review?(dao)
|
|
||
Comment 29•15 years ago
|
||
Comment on attachment 425958 [details] [diff] [review]
v7
> _applyPrefToSetting: function FullZoom__applyPrefToSetting(aValue, aBrowser) {
>- if (!this.siteSpecific && !this._inPrivateBrowsing)
>+ if ((!this.siteSpecific && !this._inPrivateBrowsing) || gInPrintPreviewMode)
> return;
please add a line break after ||
> // this tells us whether we should continue on with PP or
> // wait for the callback via the observer
> if (!notifyOnOpen.value.valueOf() || this._webProgressPP.value == null)
> this.enterPrintPreview(aWindow);
> } catch (e) {
> this.enterPrintPreview(aWindow);
aWindow seems bogus, it's not declared here and enterPrintPreview doesn't expect it. You added this in bug 487667.
Attachment #425958 -
Flags: review?(dao) → review+
|
|
Assignee | |
Comment 30•15 years ago
|
||
Huh, I wonder why I did that.
Will fix.
|
|
Assignee | |
Comment 31•15 years ago
|
||
(In reply to comment #29)
> please add a line break after ||
I don't understand why, but will do.
|
|
||
Comment 32•15 years ago
|
||
I find it harder to parse the way it is. With the line break you might also want to remove the parentheses around !this.siteSpecific && !this._inPrivateBrowsing.
|
|
Assignee | |
Comment 33•15 years ago
|
||
I certainly want to keep the parentheses.
|
|
Assignee | |
Comment 34•15 years ago
|
||
|
|
Assignee | |
Updated•15 years ago
|
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•