633937 - Make use of the toolkit's notificationbox to display notifications in MailNews

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Edmund Wong (:ewong)]

Bug 562048 made use of the toolkit's notificationbox to display notification
for junk, phishing, remotecontent and MDN.

Porting this bug to suite would make suite look better.

Comment 1

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications in MailNews (v1)

Comment 2

[[e-mail]]

Comment on attachment 634924 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v1)

Just noting this down, I haven't looked hard at this patch yet:

>+    <notificationbox id="msgNotificationBar"/>
This isn't actually the right way to use a <notificationbox>, it's designed to wrap a <browser/> element i.e.
<notificationbox id="msgNotificationBar">
  <browser id="messagepane"
           .../>
</notificationbox>

Comment 3

[Philip Chee]

Our syncUI notification box also doesn't wrap a browser or anything else.

http://hg.mozilla.org/comm-central/annotate/484802bbbd98/suite/common/sync/syncUI.js#l69

Comment 4

[[e-mail]]

Comment on attachment 634924 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v1)

The Junk button should toggle between Junk and Not Junk when you click it, but that doesn't work with the patch applied.

Comment 5

[[e-mail]]

Comment on attachment 634924 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v1)

>+draftMessageMsg=This is a draft message
>+draftMessageButton=Edit...
>+draftMessageButtonKey=E

>\ No newline at end of file
Oops.

>                     8  // 1 << (kMsgNotificationMDN - 1)
>                   ],
Not enough context to show this, but you don't use these any more.

>+  get mMsgNotificationBar() {
>+    delete this.mMsgNotificationBar;
>+    delete this.stringBundle;
>+    delete this.brandBundle;
>+
>+    this.stringBundle = document.getElementById('bundle_messenger');
>+    this.brandBundle = document.getElementById('brand_bundle');
>+    return this.mMsgNotificationBar = document.getElementById('msgNotificationBar');
>+  },
stringBundle/brandBundler should be separate getters.

>+    var junkBarMsg = this.stringBundle.getString('junkBarMessage');
Don't get this unless you need it.

>-  
>-    if (aMsgHdr) 
>-    {
>+
>+    if (aMsgHdr) {
Useless change.

>-    goUpdateCommand('button_junk');
Don't remove this!

>+    if (isJunk) {
Need to remove the notification if the message is marked as not junk. (Obviously if you do it from the notification itself, then that will happen automatically, but there are other ways to mark the message as not junk.)

>+      var buttons = [{
>+         label: this.stringBundle.getString('junkBarInfoButton'),
Nit: wrong indentation

>+           this.mMsgNotificationBar.collapsed = false;
What's the point of this?

>+       },
>+       {
>+        label: this.stringBundle.getString('junkBarButton'),
Nit: }, {
Nit: wrong indentation

>+      var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("junkContent");
I presume the short variable name is to fit inside 80 characters?

>+      if (oldNotif == null) {
Check for the old notification before starting to create the new one.
Nit: !oldNotif

>+           this.mMsgNotificationBar.PRIORITY_WARNING_HIGH,
Just checking, the overall priority is Phishing, Junk, Remote, MDN right?

>+        bar.insertBefore(linkLabel, bar.lastChild);
I think we do something slightly different for crashed plugins / geolocation.

>+//    if (!checkMsgHdrPropertyIsNot("notAPhishMessage", kIsAPhishMessage))
Test code? ;-)

>+          IgnorePhishingWarning();
We seems to call this MsgIsNotAScam() for some reason.

>+                 aMimeHdr.extractHeader("Return-Receipt-To", false); // not
Not what?

>   // private method used to set our message notification deck to the correct value...
Again, no context, but this is stuff that isn't used any more.

>+  <stringbundle id="brand_bundle" src="chrome://branding/locale/brand.properties"/>
Already exists (although separately) in the two users, so you should remove it from there if you want to add it here.

>-      ClearEditMessageBox();
Don't delete this.

Comment 6

[[e-mail]]

>+draftMessageMsg=This is a draft message
>+draftMessageButton=Edit...
>+draftMessageButtonKey=E
Oops, I meant to say "Not used?"

Comment 7

[[e-mail]]

(In reply to Philip Chee from comment #3)
> Our syncUI notification box also doesn't wrap a browser or anything else.
Although that uses a custom binding anyway, not our default binding...

Comment 8

[Philip Chee]

>>+          IgnorePhishingWarning();
> We seems to call this MsgIsNotAScam() for some reason.
I was looking into Bug 652794] (Port |Bug 584339 - In standalone window I cannot get rid of the scam warning| to SeaMonkey) but the one line fix in Thunderbird isn't sufficient. 

function setMsgHdrPropertyAndReload(aProperty, aValue)
{
  // we want to get the msg hdr for the currently selected message
  // change the appropiate property on it then reload the message

  var msgHdr = msgHdrForCurrentMessage();
  if (msgHdr)
  {
    msgHdr.setUint32Property(aProperty, aValue);
    ReloadMessage();
  }
}
We don't have a dummy Message Header sink like Thunderbird. so msgHdr is null.

Comment 9

[[e-mail]]

Comment on attachment 634924 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v1)

>diff --git a/suite/mailnews/messenger.xul b/suite/mailnews/messenger.xul
>--- a/suite/mailnews/messenger.xul
>+++ b/suite/mailnews/messenger.xul
>@@ -238,7 +238,7 @@
>                     persist="height width"
>                     class="window-focusborder"
>                     focusring="false">
>-                <deck id="msgNotificationBar"/>
>+                <notificationbox id="msgNotificationBar"/>
>                 <hbox id="msgHeaderView"/>
>                 <!-- The messagepanewrapper hbox exists to allow extensions
>                      to add sidebars to the message pane. -->
As of bug 765466, message windows now have notification boxes.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications in MailNews (v2)

This patch includes most of the changes from comment 9, but it was applied on 
top of the changes from bug #765466.  Need some feedback as the notification
bar no longer appears.  Well, it appears, but at  a brief instant and then
it's gone.

Comment 11

[[e-mail]]

Comment on attachment 639658 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v2)

>diff --git a/suite/mailnews/mailWindowOverlay.js b/suite/mailnews/mailWindowOverlay.js
Note: there are some constants near the beginning of this file that are no longer used.

>-  var junkBarWasDisplayed = gMessageNotificationBar.isFlagSet(kMsgNotificationJunkBar);
>   gMessageNotificationBar.setJunkMsg(msgHdr);
> 
>   // Only reload message if junk bar display state has changed.
>-  if (msgHdr && junkBarWasDisplayed != gMessageNotificationBar.isFlagSet(kMsgNotificationJunkBar))
>+  if (msgHdr)
[Hmm, should find some way of fixing this, via getNotificationWithValue?]

>   setJunkMsg: function(aMsgHdr)
>   {
...
>+    if (isJunk) {
...
>+      var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("junkContent");
>+      if (!oldNotif) {
...
>+        this.mMsgNotificationBar.appendNotification(junkBarMsg, "junkContent",
>+          null, this.mMsgNotificationBar.PRIORITY_WARNING_HIGH, buttons);
>+      }
>+    } else {
>+      this.clearMsgNotifications();
>+    }
You don't want to clear all of the notifications, just the junk one. The easy way to achieve this is to always query for an old notification, then if you don't want it, remove it.

>+      } else {
>+        this.mMsgNotificationBar.collapsed = false;
>+      }
I don't know what this is trying to do, but it looks wrong.

>+    } else {
>+      this.clearMsgNotifications();
>+    }
As above.

>+    } else {
>+      this.mMsgNotificationBar.collapsed = false;
>+    }
As above, but not so far ;-)

Comment 12

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications in MailNews (v3)

Comment 13

[[e-mail]]

Comment on attachment 641869 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v3)

Looking hopeful, but too many nits and also bitrotted.

>+        this.mMsgNotificationBar.collapsed = false;
Unnecessary.

>     // update the allow remote content for sender string
Comment is no longer relevant.

>+    var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("remoteContent");
I don't think we have to worry about an existing notification in this case.

>+        bar.insertBefore(linkLabel, bar.lastChild);
(How many children does the bar have?)
[The link doesn't wrap like the old one used to, probably something to address in a followup bug.]

>-    if (!checkMsgHdrPropertyIsNot("notAPhishMessage", kIsAPhishMessage))
>-      phishingMsg = isMsgEmailScam(aUrl);
>+    phishingMsg = isMsgEmailScam(aUrl);
This change looks wrong. (Although IMHO OnMsgParsed should be doing this check.)

>+
>+
Nit: double blank line

>+    var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("phishingContent");
Again, I don't think we get two phishing notifications for the same message.

>+            IgnorePhishingWarning();
MsgIsNotAScam();

>+        this.mMsgNotificationBar.appendNotification(phishingMsgNote, "phishingContent",
>+           null,
>+           this.mMsgNotificationBar.PRIORITY_CRITICAL_MEDIUM,
>+           buttons);
(Just picking on this one at random) You seem to have a mix of styles when appending notifications...

>+    let mdnHdr = aMimeHdr.extractHeader("Disposition-Notification-To", false) ||
>+                 aMimeHdr.extractHeader("Return-Receipt-To", false); // not
>+    let fromHdr = aMimeHdr.extractHeader("From", false);
>+
>+    let headerParser = Components.classes["@mozilla.org/messenger/headerparser;1"]
>+                                 .getService(Components.interfaces.nsIMsgHeaderParser);
>+    let mdnAddr = headerParser.extractHeaderAddressMailboxes(mdnHdr);
>+    let fromAddr = headerParser.extractHeaderAddressMailboxes(fromHdr);
>+
>+    let authorName = headerParser.extractHeaderAddressName(
>+                       aMsgHeader.mime2DecodedAuthor) || aMsgHeader.author;
Do you actually use these anywhere?

>+        callback: function() {
>+            SendMDNResponse();
>+        }
As it takes no parameters, you can actually write this as
callback: SendMDNResponse

>   clearMsgNotifications: function()
>   {
>+    this.mMsgNotificationBar.removeAllNotifications(true);
SeaMonkey's notification bar should automatically clear itself whenever you load a new message.
[Followup bug to remove the clearMsgNotifications function though.]

>-      ClearEditMessageBox();
>+      ClearEditMessageBox();      
Oops!

> #ifdef XP_MACOSX
>   skin/classic/messenger/accountManage.css                              (mac/messenger/accountManage.css)
>   skin/classic/messenger/filterDialog.css                               (mac/messenger/filterDialog.css)
>   skin/classic/messenger/mailWindow1.css                                (mac/messenger/mailWindow1.css)
>   skin/classic/messenger/messageHeader.css                              (mac/messenger/messageHeader.css)
>   skin/classic/messenger/primaryToolbar.css                             (mac/messenger/primaryToolbar.css)
>   skin/classic/messenger/searchDialog.css                               (mac/messenger/searchDialog.css)
>+  skin/classic/messenger/icons/info.png                                 (messenger/icons/info.png)
>+  skin/classic/messenger/icons/junk.png                                 (messenger/icons/junk.png)
>+  skin/classic/messenger/icons/phishing.png                             (messenger/icons/phishing.png)
These belong in the section after the ifdef.

>diff --git a/suite/themes/classic/mac/messenger/icons/junk.png b/suite/themes/classic/mac/messenger/icons/junk.png
Out of interest, where do these icons come from? It looks as if they haven't been run though optipng or some other png compressor.

Comment 14

[[e-mail]]

>-    // If the return receipt doesn't go to the sender address, note that in the
>-    // notification.
>-    if (mdnAddr != fromAddr)
>-      barMsg = bundle.getFormattedString("mdnBarMessageAddressDiffers",
>-                                         [authorName, mdnAddr]);
>-    else
>-      barMsg = bundle.getFormattedString("mdnBarMessageNormal", [authorName]);
Looks like this got lost somewhere...

Comment 15

[[e-mail]]

Comment on attachment 641869 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v3)

>-<!ENTITY junkBarMessage.label "&brandShortName; thinks this message is junk.">
>+junkBarMessage=This message is Junk mail.
Oh, and I agree that this change is wrong too.

Comment 16

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications in MailNews (Port Bug 562048) (v4)

Comment 17

[[e-mail]]

Comment on attachment 651252 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (Port Bug 562048) (v4)

What happened to the changes to HandleJunkStatusChanged?

>+    var brandName = this.mBrandBundle.getString("brandShortName");
>+    var junkBarMsg = this.mStringBundle.getFormattedString('junkBarMessage',
>+                                                            [brandName]);
Nit: move these inside the if (!oldNotif) block.

>+    var displayName = headerParser.extractHeaderAddressName(aMsgHdr.author);
>+    var brandName = this.mBrandBundle.getString('brandShortName');
>+    var remoteContentMsg = this.mStringBundle.getFormattedString('remoteContentBarMessage', 
>+                                                                 [brandName]);
Ditto.

>+        linkLabel.setAttribute("value", addedLink);
[Wonder whether this should say linkLabel.textContent = addedLink?]

>+        bar.insertBefore(linkLabel, bar.lastChild);
I wonder whether bar.firstChild would be a better idea here.

>-    if (!checkMsgHdrPropertyIsNot("notAPhishMessage", kIsAPhishMessage))
You still haven't said why this line was deleted...

>+    var phishingMsgNote = this.mStringBundle.getString('phishingBarMessage');
[Ditto.]

>-    // Return receipts can be RFC 3798 "Disposition-Notification-To",
>-    // or non-standard "Return-Receipt-To".
>-    var mdnHdr = aMimeHdr.extractHeader("Disposition-Notification-To", false) ||
>-                 aMimeHdr.extractHeader("Return-Receipt-To", false);
>-    var fromHdr = aMimeHdr.extractHeader("From", false);
>-
>-    var mdnAddr = MailServices.headerParser
>-                              .extractHeaderAddressMailboxes(mdnHdr);
>-    var fromAddr = MailServices.headerParser
>-                               .extractHeaderAddressMailboxes(fromHdr);
>-
>-    var authorName = MailServices.headerParser
>-                                 .extractHeaderAddressName(aMsgHeader.mime2DecodedAuthor) ||
>-                     aMsgHeader.author;
>-
>-    var barMsg;
Don't change the above lines.

>+    this.mMsgNotificationBar.removeAllNotifications(true);
As I mentioned before, we shouldn't need this in suite.

>+  <stringbundle id="brand_bundle" src="chrome://branding/locale/brand.properties"/>
Sorry for not noticing before, but we already have this stringbundle, it's called bundle_brand. Please update mBrandbundle too of course. (Not sure why bundle_brand isn't in mailWindowOverlay.xul though.]

>   skin/classic/messenger/icons/check.png                                (messenger/icons/check.png)
>   skin/classic/messenger/icons/dot.png                                  (messenger/icons/dot.png)
>+  skin/classic/messenger/icons/info.png                                 (messenger/icons/info.png)
>+  skin/classic/messenger/icons/junk.png                                 (messenger/icons/junk.png)
>+  skin/classic/messenger/icons/phishing.png                             (messenger/icons/phishing.png)
>   skin/classic/messenger/icons/flagcol.png                              (messenger/icons/flagcol.png)
Nit: icons should be in alphabetical order. Also remove the old icons! (Applies to both Classic and Modern of course.)

>-
> .inline-toolbar {
>   -moz-appearance: none;
> }
>+
Nit: move this rule back to the end of the file.

Comment 18

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #17)
> 
> >-    if (!checkMsgHdrPropertyIsNot("notAPhishMessage", kIsAPhishMessage))
> You still haven't said why this line was deleted...
> 

I believe it's because kIsAPhishMessage is no longer used.  Though to
be honest, I'm not entirely sure if !checkMsgHdrPropertyIsNot is still
useful.

> >+    var phishingMsgNote = this.mStringBundle.getString('phishingBarMessage');
> [Ditto.]
> 
Sorry. Not entirely sure what this means.  Ditto as in why it's there or
something else?

Comment 19

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications in MailNews (v5)

Comment 20

[[e-mail]]

Comment on attachment 660427 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v5)

>+        linkLabel.textContent = "text-link";
>+        linkLabel.setAttribute("value", addedLink);
Oops this isn't what I said!

>-                 aMimeHdr.extractHeader("Return-Receipt-To", false);
>+                 aMimeHdr.extractHeader("Return-Receipt-To", false); // not
...
>-                                 .extractHeaderAddressName(aMsgHeader.mime2DecodedAuthor) ||
>-                     aMsgHeader.author;
>+                                 .extractHeaderAddressName(
>+                       aMsgHeader.mime2DecodedAuthor) || aMsgHeader.author;
I think this wasn't correctly merged; these changes should not be here.

>   clearMsgNotifications: function()
>   {
>   }
[We should remove the callers, but you can do that in a follow-up.]

>diff --git a/suite/themes/modern/messenger/messageHeader.css b/suite/themes/modern/messenger/messageHeader.css
>--- a/suite/themes/modern/messenger/messageHeader.css
>+++ b/suite/themes/modern/messenger/messageHeader.css
>@@ -182,9 +182,8 @@ mail-emailaddress:-moz-focusring {
> .collapsedHeaderValue {
>   margin: 0;
> }
> 
> .collapsedAttachmentButton {
>   list-style-image: url("chrome://messenger/skin/icons/message-mail-attach.gif");
>   -moz-margin-end: .5em;
> }
>-
Oops.

Comment 21

[[e-mail]]

You still didn't say what happened to the changes to HandleJunkStatusChanged that you had in one of the previous patches.

(In reply to Edmund Wong from comment #18)
> (In reply to comment #17)
> > >-    if (!checkMsgHdrPropertyIsNot("notAPhishMessage", kIsAPhishMessage))
> > You still haven't said why this line was deleted...
> I believe it's because kIsAPhishMessage is no longer used.  Though to
> be honest, I'm not entirely sure if !checkMsgHdrPropertyIsNot is still
> useful.
It's how the MsgIsNotAScam function works, so yes, it is still useful.

> > >+    var phishingMsgNote = this.mStringBundle.getString('phishingBarMessage');
> > [Ditto.]
> Sorry. Not entirely sure what this means.  Ditto as in why it's there or
> something else?
Ditto as in move inside the if (!oldNotif) block please.

Comment 22

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications in MailNews (v5)

Comment 23

[[e-mail]]

Comment on attachment 661209 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v5)

Wow, almost ready to test :-)

>-<!ENTITY phishingBarMessage.label "&brandShortName; thinks this message might be an email scam.">
>+phishingBarMessage=This message may be a scam.
Sorry, only just noticed that you lost the brand short name here. Any chance you can put it back?

>+phishingBarErrorMessage=This message doesn't appear to be a scam
Not used? (Not sure where this comes from.)

>+const kIsAPhishMessage = 0;
>+const kNotAPhishMessage = 1;
>+
>+const kMsgNotificationPhishingBar = 1;
>+const kMsgNotificationJunkBar = 2;
>+const kMsgNotificationRemoteImages = 3;
>+const kMsgNotificationMDN = 4;
>+
> // Per message header flags to keep track of whether the user is allowing remote
> // content for a particular message.
> // if you change or add more values to these constants, be sure to modify
> // the corresponding definitions in nsMsgContentPolicy.cpp
> const kNoRemoteContentPolicy = 0;
> const kBlockRemoteContent = 1;
> const kAllowRemoteContent = 2;
> 
>-const kIsAPhishMessage = 0;
>-const kNotAPhishMessage = 1;
>-
>-const kMsgNotificationPhishingBar = 1;
>-const kMsgNotificationJunkBar = 2;
>-const kMsgNotificationRemoteImages = 3;
>-const kMsgNotificationMDN = 4;
I don't think you meant to put all of the constants back. I also don't think you meant to put them back in the wrong place ;-)

>-  var junkBarWasDisplayed = gMessageNotificationBar.isFlagSet(kMsgNotificationJunkBar);
>+
>   gMessageNotificationBar.setJunkMsg(msgHdr);
>+  var junkNotif = gMessageNotficiationBar.getNotificationWithValue("junkContent");
> 
>   // Only reload message if junk bar display state has changed.
>-  if (msgHdr && junkBarWasDisplayed != gMessageNotificationBar.isFlagSet(kMsgNotificationJunkBar))
>+  if (msgHdr && junkNotif)
Ok, so you're using gMessageNotficiationBar.getNotificationWithValue("junkContent") to tell you if the junk bar is displayed, which is good. But you need to
a) Check the value *before* calling setJunkMsg, so that you can set junkBarWasDisplayed
b) Set isJunk *after* calling setJunkMsg, so that you can compare it with junkBarWasDisplayed
c) Compare the result with null because you want it to be a boolean for the != test

>-      if ((!isJunk && folder.isSpecialFolder(Components.interfaces.nsMsgFolderFlags.Inbox)) ||
>-          (isJunk && !folder.server.spamSettings.manualMark) ||
>-          (isJunk && folder.isSpecialFolder(Components.interfaces.nsMsgFolderFlags.Junk)))
>+      if ((folder.isSpecialFolder(Components.interfaces.nsMsgFolderFlags.Inbox)) ||
>+          (!folder.server.spamSettings.manualMark) ||
>+          (folder.isSpecialFolder(Components.interfaces.nsMsgFolderFlags.Junk)))
I don't see why this should change.

I'm hoping the rest is OK ;-)

Comment 24

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications in MailNews (v6)

Comment 25

[[e-mail]]

Comment on attachment 676996 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v6)

This looks ready to test, although there are some nits I want to get out of the way in case I forget to mention them after testing:

>-  var junkBarWasDisplayed = gMessageNotificationBar.isFlagSet(kMsgNotificationJunkBar);
>+  var junkNotif = gMessageNotficationBar.getNotificationWithValue("junkContent");
Nit: please rename this back to junkBarWasDisplayed

>+  var junkWasDisplayed = gMessageNotificationBar.getNotificationWithValue("junkContent");
Nit: please rename this to isJunk

>-      if ((!isJunk && folder.isSpecialFolder(Components.interfaces.nsMsgFolderFlags.Inbox)) ||
>-          (isJunk && !folder.server.spamSettings.manualMark) ||
>-          (isJunk && folder.isSpecialFolder(Components.interfaces.nsMsgFolderFlags.Junk)))
>+      if ((junkWasDisplayed && folder.isSpecialFolder(Components.interfaces.nsMsgFolderFlags.Inbox)) ||
>+          (junkWasDisplayed && !folder.server.spamSettings.manualMark) ||
>+          (junkWasDisplayed && folder.isSpecialFolder(Components.interfaces.nsMsgFolderFlags.Junk)))
I think you have a ! missing, but the rename should make it more obvious.

Comment 26

[[e-mail]]

Comment on attachment 676996 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications in MailNews (v6)

Testing delayed slightly because I used patch which can't handle images. Also I still have to work out how to trigger the other notifications...

>+        var buttons = [{
>+          label: this.mStringBundle.getString('junkBarInfoButton'),
>+          accessKey: this.mStringBundle.getString('junkBarInfoButtonKey'),
>+          popup: null,
>+          callback: function() {
>+            MsgJunkMailInfo(false);
>+          }
IIRC clicking the info button shouldn't dismiss the notification, so you need to return true to prevent this.

>+        }, {
>+          label: this.mStringBundle.getString('junkBarButton'),
>+          accessKey: this.mStringBundle.getString('junkBarButtonKey'),
>+          popup: null,
>+          callback: function() {
>+            JunkSelectedMessages(false);
>+          }
I'm actually slightly worried that you need to return true here too, since the backend updates the junk mail bar anyway.

>+        linkLabel.textContent = "text-link";
>+        linkLabel.setAttribute("value", addedLink);
Looks like we got our wires crossed here. The className (or "class" attribute) should be "text-link". The textContent should be addedLink.

Comment 27

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v7)

Comment 28

[[e-mail]]

Comment on attachment 687634 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications (v7)

This patch is based on v4, not v6. You must have been using the wrong tree :-(

Comment 29

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v8)

Comment 30

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v9)

addressing comment 25.

Comment 31

[[e-mail]]

Comment on attachment 724335 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications (v9)

>+  var junkBarWasDisplayed = gMessageNotificationBar.getNotificationWithValue("junkContent");
>   gMessageNotificationBar.setJunkMsg(msgHdr);
>+  var isJunk = gMessageNotificationBar.getNotificationWithValue("junkContent");
Oops, these need to say gMessageNotificationBar.mMsgNotificationBar.getNotificationWithValue

>+        linkLabel.setAttribute("value", addedLink);
linkLabel.textContent, and you also appear to need to set the flex to 1 otherwise it doesn't work with a narrow layout.

>-      barMsg = gMessengerBundle.getFormattedString("mdnBarMessageAddressDiffers",
>-                                                   [authorName, mdnAddr]);
>+      barMsg = bundle.getFormattedString("mdnBarMessageAddressDiffers",
>+                                         [authorName, mdnAddr]);
>     else
>-      barMsg = gMessengerBundle.getFormattedString("mdnBarMessageNormal",
>-                                                   [authorName]);
>+      barMsg = bundle.getFormattedString("mdnBarMessageNormal", [authorName]);
This change is wrong.

r=me with the above fixed.

Comment 32

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v10)

Comment 33

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v11)

Unbitrotted patch.

Comment 34

[[e-mail]]

Comment on attachment 780781 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications (v11)

Sorry, I hadn't realised you'd rerequested review. (Which is fortunate, because I spotted a couple of nits.)

>+        linkLabel.class = "text-link";
>+        linkLabel.textContent = addedLink;
>+        linkLabel.setAttribute("flex", "1");
I don't know how I didn't spot it before, but the property is className, not class. Also while you're there you can use linkLabel.flex = 1;

>     if (mdnAddr != fromAddr)
>-      barMsg = gMessengerBundle.getFormattedString("mdnBarMessageAddressDiffers",
>-                                                   [authorName, mdnAddr]);
>+      barMsg = mStringbundle.getFormattedString("mdnBarMessageAddressDiffers",
>+                                         [authorName, mdnAddr]);
>     else
>-      barMsg = gMessengerBundle.getFormattedString("mdnBarMessageNormal",
>-                                                   [authorName]);
>+      barMsg = mStringbundle.getFormattedString("mdnBarMessageNormal", [authorName]);
Typo: mStringBundle has a capital B (2x).

Comment 35

[[e-mail]]

(In reply comment #34)
> Sorry, I hadn't realised you'd rerequested review.
Oh, they were both today, that's what confused me.
(Pro tip: I know about -F8 to apply patches which haven't really bitrotted.)

Comment 36

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v12)

Comment 37

[Karsten Düsterloh]

Comment on attachment 781404 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications (v12)

Nice! :)

All I have are some nits regarding coding style and such.


>+# LOCALIZATION NOTE (junkBarMessage): %S is the brandname
>+junkBarMessage=%S thinks this message is junk.
>+phishingBarMessage=%S thinks this message may be an e-mail scam.

(Drive-by comment: ever since the introduction of these texts I wonder how complex SM has become to gain consciousness. ;-) )
(I'm not a native speaker, but maybe basing these sentences on the verb "to regard" would be more suitable?) 

>+  get mStringBundle() {
>+    delete this.mStringBundle;
>+
>+    return this.mStringBundle = document.getElementById('bundle_messenger');
>+  },
>+
>+  get mBrandBundle() {
>+    delete this.mBrandBundle;
>+
>+    return this.mBrandBundle = document.getElementById('bundle_brand');
>+  },
>+
>+  get mMsgNotificationBar() {
>+    delete this.mMsgNotificationBar;
>+
>+    return this.mMsgNotificationBar = document.getElementById('messagepanebox');
>+  },

Please adhere to the brace-on-its-own-line policy in all three getters. And maybe remove the empty line between the "delete" and the "return" lines.

>+    if (isJunk) {
>+      if (!oldNotif) {
…

Same for this entire block of changes: Please adhere to the brace-on-its-own-line policy.
You may also consider using "let" instead of "var" for declarations not on function level.

>+    if (!oldNotif) {

Same for this entire block of changes.

>+    if (phishingMsg) {

And this entire of block of changes.

>     var mdnHdr = aMimeHdr.extractHeader("Disposition-Notification-To", false) ||
>-                 aMimeHdr.extractHeader("Return-Receipt-To", false);
>+                 aMimeHdr.extractHeader("Return-Receipt-To", false); // not

Why this change? This comment isn't exactly enlightening. ;-)

>+    if (!oldNotif) {

Brace policy violation in this entire block of changes.

>+function allowRemoteContentForSender(authorEmailAddress, authorDisplayName)

Arguments should start with a lowercase "a", e.g. aAuthorEmailAddress, aAuthorDisplayName.


sr=me with these changes.

Comment 38

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v13)

I've changed two localization strings to use 'regards'.  asking for sr again

Comment 39

[Karsten Düsterloh]

Comment on attachment 785582 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications (v13)

>+junkBarMessage=%S regards this message is junk.
>+phishingBarMessage=%S regards that this message may be an e-mail scam.

Huh? That's no English, is it?
I was thinking of something like "%S regards this message as junk" and "%S regards this message as an e-mail scam".

sr=me with this.

Comment 40

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v14)

Patch for checkin

Comment 41

[Edmund Wong (:ewong)]

Attachment: Make use of the toolkit's notificationbox to display notifications (v15)

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/e904685c6ace

Comment 43

[Ian Neal]

Comment on attachment 787946 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications (v15)

>+++ b/suite/locales/en-US/chrome/mailnews/messenger.properties

>+draftMessageMsg=This is a draft message
>+draftMessageButton=Edit...
Shouldn't this end in an ellipsis?
>+draftMessageButtonKey=E

Comment 44

[Magnus Melin [:mkmelin]]

Actually it really should be no dots at all (yes, ellipsis if any of course). For thunderbird I'm getting rid of the ellipsis with the patch in bug 562048. Ellipsis implies further action needed. In this case Edit IS the action. Think "Save" vs "Save As..."

Comment 45

[[e-mail]]

As per comment #6, they are bogus. rs=me to remove them.

Comment 46

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #34)
> >     if (mdnAddr != fromAddr)
> >-      barMsg = gMessengerBundle.getFormattedString("mdnBarMessageAddressDiffers",
> >-                                                   [authorName, mdnAddr]);
> >+      barMsg = mStringbundle.getFormattedString("mdnBarMessageAddressDiffers",
> >+                                         [authorName, mdnAddr]);
> >     else
> >-      barMsg = gMessengerBundle.getFormattedString("mdnBarMessageNormal",
> >-                                                   [authorName]);
> >+      barMsg = mStringbundle.getFormattedString("mdnBarMessageNormal", [authorName]);
> Typo: mStringBundle has a capital B (2x).

Worse, it's wrong even after that because of the missing "this." prefix, which caused bug 955830. :-(

Comment 47

[Edmund Wong (:ewong)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #46)
> (In reply to [e-mail] from comment #34)
> > >     if (mdnAddr != fromAddr)
> > >-      barMsg = gMessengerBundle.getFormattedString("mdnBarMessageAddressDiffers",
> > >-                                                   [authorName, mdnAddr]);
> > >+      barMsg = mStringbundle.getFormattedString("mdnBarMessageAddressDiffers",
> > >+                                         [authorName, mdnAddr]);
> > >     else
> > >-      barMsg = gMessengerBundle.getFormattedString("mdnBarMessageNormal",
> > >-                                                   [authorName]);
> > >+      barMsg = mStringbundle.getFormattedString("mdnBarMessageNormal", [authorName]);
> > Typo: mStringBundle has a capital B (2x).
> 
> Worse, it's wrong even after that because of the missing "this." prefix,
> which caused bug 955830. :-(

My apologies for that mistake.  I see it's covered in 955830.  I guess I'll
just fix the ellipses then.