Closed Bug 1429883 Opened 4 years ago Closed 4 years ago

merge msgPrintEngine.xul for Seamonkey and Thunderbird

Categories

(MailNews Core :: Printing, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file)

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.
Attached patch 1429883.patchSplinter Review
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.
Attachment #8941959 - Flags: ui-review?(richard.marti)
Attachment #8941959 - Flags: review?(jorgk)
Attachment #8941959 - Flags: review?(frgrahl)
Depends on: 581470
Comment on attachment 8941959 [details] [diff] [review]
1429883.patch

ui-r+ in the sense that I see no difference to before the patch.
Attachment #8941959 - Flags: ui-review?(richard.marti) → ui-review+
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 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.
Attachment #8941959 - Flags: review?(frgrahl) → review+
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
(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 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+.
Attachment #8941959 - Flags: review?(jorgk) → review+
(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.
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8bebc74d29d4
clean up syntax in msgPrintEngine.js and switch SM to using mailnews/. r=jorgk,frg
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
You need to log in before you can comment on or make changes to this bug.