Closed Bug 387247 Opened 13 years ago Closed 13 years ago

Use toolkit printUtils.js instead of communicator printing.js

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: kairo, Assigned: philor)

References

()

Details

Attachments

(2 files, 1 obsolete file)

Since toolkit's printUtils.js has started as a clone of communicator's printing.js and has the same functions defined, we should use that global file instead of the toolkit file.

This affects Thunderbird files as well as a SeaMonkey mail, browser and composer.
Unfortunately, printUtils.js isn't as much a drop-in replacement for printing.js as I hoped, I'll need to investigate the diff between the two files.
The two differences I know about are the easy need to switch all calls to NSPrint and NSPrintSetup to call PrintUtils.print and PrintUtils.showPageSetup, and the not so easy need to deal with bug 270235 (particularly since the #ifdef MOZ_THUNDERBIRD bit has rotted, since BrowserExitPrintPreview() now only exists in calendar/, and to maximize the fun, Thunderbird was trying to avoid calls to hide the tabstrip, and now Thunderbird has a tabstrip (though not exactly the same tabstrip), while I think SeaMonkey mail doesn't, yet).
Duplicate of this bug: 246995
And perhaps the bug 246995 problem of the var XUL_NS in printUtils.js conflicting with a const XUL_NS, which should probably be solved by putting it in the PrintUtils object instead of the global scope.
Oh, the tabstrip part is easy: that should have been in browser.js's toggleAffectedChrome(), the way it is in suite's browser.js, so the ifdef bits should collapse into just needing a generalized way to get "the thing before which the toolbar should be inserted" and having Tbird close the window in its aExitPPCallback.
Bowing to the inevitable.
Assignee: kairo → philringnalda
Depends on: 387516
See bug 321954 where I did submit a patch to do something similar
Heh, somehow I missed that, even though I've been porting back your fixes from bug 311028 where it's mentioned. 
Depends on: 321954
Depends on the patch in bug 321954.
Attached patch Switch over SeaMonkey (obsolete) — Splinter Review
Comment on attachment 272380 [details] [diff] [review]
Switch over SeaMonkey

I think there will be the function getEngineWebBrowserPrint() that can be removed from:
/editor/ui/composer/content/editor.js
/mailnews/base/resources/content/msgPrintEngine.js
/suite/browser/browser.js

Also the method nsMsgPrintEngine::GetWebBrowserPrint could be removed from /mailnews/base/src/nsMsgPrintEngine.cpp if getWebBrowserPrint: function in printUtils.js works for mailnews/TB

On a separate note should we also be comparing the two printPreviewBindings.xml files in suite and toolkit?
Ah, indeed. More things to delete are always good, though I only hope I've gone after nsMsgPrintEngine correctly.

I'm not sure about the printPreviewBindings - unless I missed something, the difference is just a little code cleanup, and some debug code. Is it worth fighting for scarce review resources to sync and consolidate the two right now?
Attachment #272380 - Attachment is obsolete: true
Attachment #272379 - Flags: review?(mkmelin+mozilla)
Attachment #272457 - Flags: superreview?(neil)
Attachment #272457 - Flags: review?(neil)
Comment on attachment 272457 [details] [diff] [review]
Switch over SeaMonkey, v2

On second thought, "please review this even though you have no idea what will wind up happening in bug 321954" isn't a particularly good idea.
Attachment #272457 - Flags: superreview?(neil)
Attachment #272457 - Flags: review?(neil)
Attachment #272379 - Flags: review?(mkmelin+mozilla)
Attachment #272457 - Flags: superreview+
Attachment #272457 - Flags: review?(iann_bugzilla)
Comment on attachment 272379 [details] [diff] [review]
Switch over Tbird

Poor Magnus is going to be heartbroken that he missed a chance to review printing code by being offline.
Attachment #272379 - Flags: review?(mscott)
Attachment #272379 - Flags: review?(mscott) → review+
Just in case I forget, but then look at this comment, the Tbird patch does depend on the SeaMonkey one landing first.
Comment on attachment 272457 [details] [diff] [review]
Switch over SeaMonkey, v2

Looks good to me, sorry for the delay, r=me
Attachment #272457 - Flags: review?(iann_bugzilla) → review+
Just a reminder that calPrintEngine.js still mentions bug 270235 and probably could do with some tidy up now that bug 321954 has landed either in this bug or a follow up one (unless there is an appropriate one already filed).
mail/base/content/msgPrintEngine.xul 1.6
mail/base/content/mailWindowOverlay.xul 1.221
mail/base/content/mail3PaneWindowCommands.js 1.40
mail/base/content/messageWindow.js 1.52
mail/components/addrbook/content/addressbook.xul 1.71
mail/components/compose/content/messengercompose.xul 1.105
mail/components/compose/content/MsgComposeCommands.js 1.117

editor/ui/composer/content/ComposerCommands.js 1.206
editor/ui/composer/content/editor.xul 1.160
editor/ui/composer/content/editor.js 1.244
mailnews/compose/resources/content/MsgComposeCommands.js 1.401
mailnews/compose/resources/content/messengercompose.xul 1.291
mailnews/addrbook/resources/content/addressbook.xul 1.180
mailnews/base/resources/content/msgPrintEngine.xul 1.19
mailnews/base/resources/content/msgPrintEngine.js 1.23
mailnews/base/src/nsMsgPrintEngine.cpp 1.92
mailnews/base/public/nsIMsgPrintEngine.idl 1.11
suite/browser/navigator.xul 1.452
suite/browser/navigatorOverlay.xul 1.329
suite/browser/viewSourceOverlay.xul 1.30
suite/browser/browser.js 1.41
suite/common/jar.mn 1.26
suite/common/printing.js delete
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M8
Blocks: 394143
Gah, and mailnews/compose/resources/content/MsgComposeCommands.js 1.402, since I somehow managed to commit just one of the two hunks in that file.
(In reply to comment #17)
> Just a reminder that calPrintEngine.js still mentions bug 270235 and probably
> could do with some tidy up now that bug 321954 has landed either in this bug or
> a follow up one (unless there is an appropriate one already filed).

Bug 350099 would be the appropriate one - see its antecedent, bug 275883, which made that deadcode.
You need to log in before you can comment on or make changes to this bug.