387247 - Use toolkit printUtils.js instead of communicator printing.js

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Robert Kaiser]

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.

Comment 1

[Robert Kaiser]

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.

Comment 2

[Phil Ringnalda (:philor)]

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).

Comment 4

[Phil Ringnalda (:philor)]

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.

Comment 5

[Phil Ringnalda (:philor)]

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.

Comment 6

[Phil Ringnalda (:philor)]

Bowing to the inevitable.

Comment 7

[Ian Neal]

See bug 321954 where I did submit a patch to do something similar

Comment 8

[Phil Ringnalda (:philor)]

Heh, somehow I missed that, even though I've been porting back your fixes from bug 311028 where it's mentioned. 

Comment 9

[Phil Ringnalda (:philor)]

Attachment: Switch over Tbird

Depends on the patch in bug 321954.

Comment 10

[Phil Ringnalda (:philor)]

Attachment: Switch over SeaMonkey

Comment 11

[Ian Neal]

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?

Comment 12

[Phil Ringnalda (:philor)]

Attachment: Switch over SeaMonkey, v2

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?

Comment 13

[Phil Ringnalda (:philor)]

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.

Comment 14

[Phil Ringnalda (:philor)]

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.

Comment 15

[Phil Ringnalda (:philor)]

Just in case I forget, but then look at this comment, the Tbird patch does depend on the SeaMonkey one landing first.

Comment 16

[Ian Neal]

Comment on attachment 272457 [details] [diff] [review]
Switch over SeaMonkey, v2

Looks good to me, sorry for the delay, r=me

Comment 17

[Ian Neal]

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).

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[Phil Ringnalda (:philor)]

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.

Comment 20

[Phil Ringnalda (:philor)]

(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.