1429883 - merge msgPrintEngine.xul for Seamonkey and Thunderbird

Status: RESOLVED FIXED

(MailNews Core :: Printing, enhancement)

Description

[:aceman]

It seems the files msgPrintEngine.xul for Seamonkey and Thunderbird are mostly identical. Let's merge them to avoid them drifting away and allow sharing future improvements.

Comment 1

[:aceman]

Attachment: 1429883.patch

The only difference between the files is this:
< <?xml-stylesheet href="chrome://messenger/skin/dialogs.css" type="text/css"?>
---
> <?xml-stylesheet href="chrome://messenger/skin/" type="text/css"?>

I'm not sure what the difference in the css does, Paenglab, can you please check?

11,12d9
< <!DOCTYPE window SYSTEM "chrome://messenger/locale/msgPrintEngine.dtd">
< 
28,29c25,26
<   <key id="printKb" key="&printCmd.key;" command="cmd_print" modifiers="accel"/>
<   <key id="key_close" key="&closeCmd.key;" command="cmd_close" modifiers="accel"/>
---
>   <key id="printKb"/>
>   <key id="key_close"/>

Merging the files, Seamonkey gets bug 581470 for free ;)

There should be no functional changes for TB here.
Main review is whether this still works for Seamonkey.

Comment 2

[Richard Marti (:Paenglab)]

Comment on attachment 8941959 [details] [diff] [review]
1429883.patch

ui-r+ in the sense that I see no difference to before the patch.

Comment 3

[:aceman]

There should be no change visible on TB as I used the TB version of the file.

But what does chrome://messenger/skin/ do differently from chrome://messenger/skin/dialogs.css ?

Comment 4

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8941959 [details] [diff] [review]
1429883.patch

SeaMonkey MailNews is rather broken right now after the XUL template removal but was able to do a print preview composing a new message. I don't see a difference and think it is the right thing to do.

https://developer.mozilla.org/en-US/docs/Web/CSS/%40import

The first line in SeaMonkeys chrome://messenger/skin/dialogs.css
is @import url("chrome://messenger/skin/");

so the changed css should make no difference.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8941959 [details] [diff] [review]
1429883.patch

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

::: mailnews/base/content/msgPrintEngine.js
@@ +66,5 @@
>  function setPPTitle(aTitle)
>  {
> +  let title = aTitle;
> +  let gBrandBundle = document.getElementById("bundle_brand");
> +  let msgBundle = document.getElementById("bundle_messenger");

Can you please explain why this is OK when it previously was:
let msgBundle = Services.strings.createBundle("chrome://messenger/locale/messenger.properties");
which is code we still have elsewhere:
https://dxr.mozilla.org/comm-central/search?q=Services.strings.createBundle(%22chrome%3A%2F%2Fmessenger%2Flocale%2Fmessenger.properties%22)%3B&redirect=false

Comment 6

[:aceman]

(In reply to Jorg K (GMT+1) from comment #5)
> > +  let msgBundle = document.getElementById("bundle_messenger");
> 
> Can you please explain why this is OK when it previously was:
> let msgBundle =
> Services.strings.createBundle("chrome://messenger/locale/messenger.
> properties");

I found it strange to load a bundle that we already have imported via the <stringbundle> element. So I just used the element and the function to load a string from it has a different name.

> which is code we still have elsewhere:
> https://dxr.mozilla.org/comm-central/search?q=Services.strings.
> createBundle(%22chrome%3A%2F%2Fmessenger%2Flocale%2Fmessenger.
> properties%22)%3B&redirect=false

You can only use the element if you have a containing xul file with the element for your JS. So e.g. mailnews/base/test/unit/test_bug434810.js has no XUL dialog so Services.strings.createBundle() is appropriate there. From the occurrences dxr shows, maybe mail/base/content/aboutDialog.js and mail/components/preferences/advanced.js could be converted to <stringbundle> (but I looked and it would be of no benefit), the others are probably OK as they are (used from various places where we are not sure if there is a <stringbundle> around).

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8941959 [details] [diff] [review]
1429883.patch

(In reply to :aceman from comment #6)
> I found it strange to load a bundle that we already have imported via the
> <stringbundle> element. So I just used the element and the function to load
> a string from it has a different name.
You're referring to
mail/base/content/msgPrintEngine.xul
24 <stringbundle id="bundle_messenger" src="chrome://messenger/locale/messenger.properties"/>
Right?

I didn't try it, but the changes are just a bunch of clean-up plus this change which you've explained. Richard doesn't see a difference, so r+.

Comment 8

[:aceman]

(In reply to Jorg K (GMT+1) from comment #7)
> You're referring to
> mail/base/content/msgPrintEngine.xul
> 24 <stringbundle id="bundle_messenger"
> src="chrome://messenger/locale/messenger.properties"/>
> Right?

Yes, thanks.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8bebc74d29d4
clean up syntax in msgPrintEngine.js and switch SM to using mailnews/. r=jorgk,frg