Closed Bug 562048 Opened 14 years ago Closed 11 years ago

Notification bar needs to look slicker (Remote Content, Junk, Scam, Send Return Receipt, Edit Draft)

Categories

(Thunderbird :: Message Reader UI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 28.0

People

(Reporter: andreasn, Assigned: mkmelin)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: ux-consistency)

Attachments

(12 files, 42 obsolete files)

68.13 KB, image/png
Details
28.08 KB, image/png
Details
8.42 KB, patch
Details | Diff | Splinter Review
2.71 KB, text/plain
Details
84.02 KB, image/jpeg
Details
22.29 KB, image/png
Details
39.34 KB, image/png
bwinton
: ui-review+
Details
50.13 KB, image/png
bwinton
: ui-review-
Details
33.78 KB, image/png
bwinton
: ui-review+
Details
142.74 KB, image/png
Details
216.98 KB, image/png
Details
109.03 KB, patch
bwinton
: review+
mkmelin
: ui-review+
Details | Diff | Splinter Review
The Firefox notification bar on the Mac looks really slick. It would be nice to use a similar style for Thunderbird as well.
Keywords: ux-consistency
So I was actually going to file a bug sometime (I thought I had, but I can't find it) on the fact that we should replace our current notification bar in messages with the toolkit one - the one we currently has is made up "manually", and replacing it with the current toolkit notification bar would be a bonus and allow us to get the same styling.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Hints for implementation:

The existing xul "notification" bar for messages is defined here:

http://hg.mozilla.org/comm-central/annotate/4a6a1e8f4389/mail/base/content/mailWindowOverlay.xul#l1852

The main code that drives the xul is here:

http://hg.mozilla.org/comm-central/annotate/4a6a1e8f4389/mail/base/content/mailWindowOverlay.js#l2539

If you look for notificationbar in existing xul code, you'll see various - Thunderbird has them in content tabs and on its main window.

Generally, you're looking at completely replacing the xul notification bar in mailWindowOverlay.xul (id="msgNotificationBar") with a <notificationbar> that wraps the messagepane browser element.

I would suggest keeping some of the gMessageNotificationBar structure for easy of use. Note that the notificationbar element can do its own prioritisation (iirc) if you give it the right data.

I'd suggest initially doing:

- basic replacement of the msgNotificationBar with the notificationbar element
- get some notifications being displayed at appropriate times.
- Just hard-code strings until you've got the basics working.

At that point, you could try for an initial ui-review, and I'm sure Andreas will help with some styling.
OS: Mac OS X → All
Hardware: x86 → All
(In reply to comment #2)
> At that point, you could try for an initial ui-review, and I'm sure Andreas
> will help with some styling.

Yup, just let me know if/when there is anything I can help out with.
Attached patch Rough patch (obsolete) — Splinter Review
This patch really sucks right now, but it kind of works.
Makes use of <notificationbox> and <notification> for all the notifications.
This also makes the notifications smaller, bug 540106

Known issues:
If a notification is closed, the next in turn will show up.
Icons are wrong.
Attached patch updated patch (obsolete) — Splinter Review
Sets warning on all the notifications to make them look that Firefox-slick on Mac OS X.
Attachment #468689 - Attachment is obsolete: true
Attachment #511007 - Flags: feedback?(bugzilla)
Attached patch Example patch (obsolete) — Splinter Review
This is closer to what I was thinking of. It removes all of the existing xul bars and replaces them by a single notificationbar element.

The notificationbar is then fed individual displays by the appropraite function.

This example works reasonably well for junk emails, I've not updated anything else.

There's a bunch of work still to do:

- removing all the items associated with the old set-up (strings, css etc)
- revising the existing code to work with the new bars
- localisation of the javascript strings
- making sure we've got a chrome icon at the same place in the theme on all platforms

and probably a bit more.

However, that's where I would be starting from.
Comment on attachment 511007 [details] [diff] [review]
Made the notification bar look slicker. (v1)

I think if we're going to make a change here, we may as well try going the whole way and reimplementing this with proper use of notificationbar, which will be better for extensions as well.
Attachment #511007 - Flags: feedback?(bugzilla) → feedback-
Comment on attachment 511015 [details] [diff] [review]
Example patch

Notes for whom it may concern:

>+      var button = [{
[Since it's an array, it's normally named buttons, even if there's only one.]

>+      this.mMsgNotificationBar.appendNotification("This message is junk", 1, "chrome://messenger/skin/icons/junk.png", this.mMsgNotificationBar.PRIORITY_INFO_MEDIUM, button);
Text will need to be localised of course.

>+//    this.updateMsgNotificationBar(kMsgNotificationJunkBar, isJunk);
Need to remove the notification if it's not junk.

>+    this.mMsgNotificationBar.removeAllNotifications(true);
>+    // XXX fixme
>+//    this.mBarStatus = 0;
>+//    this.mMsgNotificationBar.selectedIndex = 0;
>+//    this.mMsgNotificationBar.collapsed = true;
Well, that's the equivalent, although it will remove extension notifications, which may or may not be desired. There's another call that only removes notifications due for removal.

>+                    <!-- XXX May want to actually wrap this around messagepanewrapper. XXX Does this want to be within the messagepanewrapper hbox? -->
>+                    <notificationbox id="msgNotificationBar"/>
>                     <!-- The messagepanewrapper hbox exists to allow
>                          extensions to add sidebars to the message. -->
>                     <hbox id="messagepanewrapper" flex="1">
By comparison with other <notificationbox>es, the ideal location is inside the messagepanewrapper wrapped around the messagepane <browser>.
(In reply to comment #10)
> Text will need to be localised of course.

That's what "- localisation of the javascript strings" meant ;-)

> >+//    this.updateMsgNotificationBar(kMsgNotificationJunkBar, isJunk);
> Need to remove the notification if it's not junk.

If clear notifications does a removeAllNotifications, then it isn't necessary to remove the notification.

> >+    this.mMsgNotificationBar.removeAllNotifications(true);
> >+    // XXX fixme
> >+//    this.mBarStatus = 0;
> >+//    this.mMsgNotificationBar.selectedIndex = 0;
> >+//    this.mMsgNotificationBar.collapsed = true;
> Well, that's the equivalent, although it will remove extension notifications,
> which may or may not be desired. There's another call that only removes
> notifications due for removal.

Yes, we need to check the callers to that function. I was assuming it was generally just called on change of message/display, and therefore I think we should always clear all notifications (e.g. think of going from a message display to the start page).
Blocks: 633937
Attachment #511007 - Attachment is obsolete: true
Attachment #513639 - Flags: ui-review?
Attachment #513639 - Flags: ui-review? → ui-review?(nisses.mail)
(In reply to comment #12)
> Created attachment 513639 [details] [diff] [review]
> Made the notification bar look slicker.

Thanks for the patch Edmund!
I seem to have some issues getting it to function correctly on my system.
When running this patch, Thunderbird seemed to become very confused about what messages should pop up a "remote content" bar, a "return recipient" bar etc.
It also spits out some errors about linkLabel not being defined in the error console. I think this might be related. Do you get the same behavior on your system running the patch?

Visually, it seem that making the notification type=info makes the bar all gray on Linux as it sets the background-color to -moz-dialog. Removing that in DOM inspector seems to make it have it's regular color (infobackground) again. Haven't tried on other systems yet.
Just changed the names of the properties.  Also moved the notification bar
before the message header.  The 'meat' of the patch hasn't been changed.
Attachment #513639 - Attachment is obsolete: true
Attachment #513862 - Flags: ui-review?(nisses.mail)
Attachment #513639 - Flags: ui-review?(nisses.mail)
(In reply to comment #14)
> Just changed the names of the properties.  Also moved the notification bar
> before the message header.  The 'meat' of the patch hasn't been changed.

I'm not convinced by this change. It will mean that if a notification bar is displayed the header will be displayed lower down. This will mean that the position of the buttons in the header will change.

For example, if I'm busy archiving messages in a folder, and I hit a message with a notification bar, then suddenly the archive button isn't going to be in the same place, and I'm going to potentially hit a notification bar icon instead.

(In reply to comment #13)
> Visually, it seem that making the notification type=info makes the bar all gray
> on Linux as it sets the background-color to -moz-dialog. Removing that in DOM
> inspector seems to make it have it's regular color (infobackground) again.
> Haven't tried on other systems yet.

This is probably because we're using PRIORITY_INFO_MEDIUM everywhere and grey is the information option.

There's probably two parts to this. Firstly, change the respective priorities to match the notification type, so I would probably suggest:

- Phishing: PRIORITY_CRITICAL_MEDIUM
- Junk: PRIORITY_WARNING_HIGH
- Remote Content: PRIORITY_WARNING_MEDIUM
- MDN (Return Receipt): PRIORITY_INFO_MEDIUM

I suspect Critical is red, and Warning is yellow.

However, if we're not happy with the default colours of those, then we can add some styling just to the notification bar in the message header to alter those colours.
Comment on attachment 513862 [details] [diff] [review]
Made the notification bar look slicker. (v2)

Just a couple of fly-by comments.

>@@ -2556,16 +2556,44 @@ var gMessageNotificationBar =

Once this is a bit nearer completion, you should be able to remove mBarStatus and mBarFlagValues from gMessageNotificationBar.

>+    var junkBarMsg = stringBundle.getString('junkBarMessage') + " ";
>+    var junkBarNotJunk = stringBundle.getString('junkBarButton') + " ";
>+    var junkBarInfo  = stringBundle.getString('junkBarInfoButton') + " ";

Not sure why you're adding spaces to these, it shouldn't be necessary.

>+    if (isJunk) {
>+      var buttons = [{
>+         label: junkBarInfo,
>+         accessKey: junkBarInfoKey,

I think you could skip the intermediate variables for these.

>+      this.mMsgNotificationBar.appendNotification(junkBarMsg, 1,
>+        "chrome://messenger/skin/icons/folder-junk.gif",
>+        this.mMsgNotificationBar.PRIORITY_INFO_MEDIUM,
>+        buttons);
>+
>+      this.updateMsgNotificationBar(kMsgNotificationJunkBar, isJunk);

updateMsgNotificationBar should just go away because the appendNotification is doing the equivalent.

>+    var stringBundle = document.getElementById('bundle_messenger');
>+    var brandBundle = document.getElementById('brand_bundle');

Might be worth just making these member variables of gMessageNotificationBar in the same way that mMsgNotificationBar is.
Attachment #513862 - Attachment is obsolete: true
Attachment #514113 - Flags: ui-review?(nisses.mail)
Attachment #513862 - Flags: ui-review?(nisses.mail)
I would really like to see this get done. :)
Yay! This is making great progress!

Junk bar:
* Missing junk icon to the left
* the big [  ?  ]-button looks a bit heavy. I think we want to make this just an icon. Probably something similar to the (?) icon in the address book [1].

Remote content bar:
* I was unable to trigger this at all.

Draft message:
* Put the edit button on the right
* (This is kind of out of the scope of this bug, but...) it would be neat to fix the bold+italic text to just regular text here.
Attached patch info iconsSplinter Review
Here is a patch with new info icons for all platforms (linux, mac, windows xp/aero) to use for the info icon.
I think it would be possible to continue using CSS for the images.

https://developer.mozilla.org/en/XUL/notificationbox#m-appendNotification
appendNotification( label , value , image , priority , buttons )

This creates a notification inside the notiifcation box with a "value" attribute set to the second parameter of appendNotification()

So:

this.mMsgNotificationBar.appendNotification(junkBarMsg,
  "junk-message",
  null,
  this.mMsgNotificationBar.PRIORITY_WARNING_HIGH,
  buttons);

Then you could use some CSS like:

{
#msgNotificationBar > notification[value="junk-message"] {
  list-style-image: url("chrome://messenger/skin/icons/junk.png");
}

This makes things easier for third party themes.
Attachment #514113 - Attachment is obsolete: true
Attachment #531310 - Flags: ui-review?(nisses.mail)
Attachment #514113 - Flags: ui-review?(nisses.mail)
Ewong, where is the bit where you set the class attribute to "infoButton"??
Comment on attachment 531310 [details] [diff] [review]
Made the notification bar look slicker. (v4)

In general this looks good. Some small issues I ran into:
* "To protect your privacy, &brandShortName; has blocked..." in the remote content bar. It should probably say Thunderbird and/or Shredder.
* If viewing a message with remote content right after another and then closing the bar in the second message, another one (from the previous message?) is still around.
* Icon scaling issues for remote content, at least under Linux. I'll come up with some updated graphics for this and attach to the bug.
Attachment #531310 - Flags: ui-review?(nisses.mail) → ui-review-
It also seems that the phishing bar does no longer work.
Attachment #531310 - Attachment is obsolete: true
Attachment #537346 - Flags: ui-review?(nisses.mail)
Attachment #537346 - Attachment is obsolete: true
Attachment #537452 - Flags: ui-review?(nisses.mail)
Attachment #537346 - Flags: ui-review?(nisses.mail)
Comment on attachment 537452 [details] [diff] [review]
Made the notification bar look slicker. (v6)

Edmund told me there were still some issues with this patch (specifically regards triggering phishing), so setting the Flag to feedback instead of ui-review and trying out this as soon as I'm done with my build.
Attachment #537452 - Flags: ui-review?(nisses.mail) → feedback?(nisses.mail)
It seems this patch no longer apply. Bitrotted?
unbitrotted patch
Attachment #537452 - Attachment is obsolete: true
Attachment #546423 - Flags: feedback?(nisses.mail)
Attachment #537452 - Flags: feedback?(nisses.mail)
Comment on attachment 546423 [details] [diff] [review]
Made the notification bar look slicker. (v7)

The phishing part isn't done.  isPhishingMsg could require a new parameter aUrl.
Comment on attachment 546423 [details] [diff] [review]
Made the notification bar look slicker. (v7)

In general, nice progress!
One small thing I noticed is that the close button in the remote content bar triggers the animation, but don't make the bar go away. Apart from that, things are looking sweet!
Attachment #546423 - Flags: feedback?(nisses.mail) → feedback+
I started working on bug 631688, but I think the best solution to that bug is included in this patch. Edmund, let me know if you're still working on this or if I can take over.
(In reply to comment #34)
> I started working on bug 631688, but I think the best solution to that bug
> is included in this patch. Edmund, let me know if you're still working on
> this or if I can take over.

Yup. Still working on it.
With phishing added, but need feedback.  Remote Content notification bar is faulty. From the DOM, it seems to be generated more than once.
Attachment #546423 - Attachment is obsolete: true
Attachment #551028 - Flags: feedback?(nisses.mail)
the OnMsgHasRemoteContent callback can be called multiple times per message; I'm not completely sure, but it may be once per remote URL in the message.

There's an example in the MDN docs for notificationbox of either getting the existing notification for a particular key, or creating a new one; see https://developer.mozilla.org/en/Code_snippets/Alerts_and_Notifications#Using_notification_box

You'll need to do this for remote content, and it might be a good idea to do it for all the other notifications too.

I'm also getting an JavaScript exception because your patch doesn't include a declaration for checkMsgHdrPropertyIsNot which you're using in setPhishingMsg()
Attachment #551028 - Attachment is obsolete: true
Attachment #552067 - Flags: feedback?(irving)
Attachment #551028 - Flags: feedback?(nisses.mail)
Attachment #552067 - Flags: feedback?(irving) → feedback?(nisses.mail)
Sorry for the massive delay; aside from vacation, I got hung up trying to figure out why I couldn't add non-button elements to the notificationbox XUL. Next step is to ask on IRC #xul...

I did come up with a fix for multiple copies of the notification, I've attached a patch to go on top of ewong's v9 patch. Most of the change is just indentation, *except* the appendNotification() call where I replaced a null parameter with a string tag for the type of notification.

I also tried a version with all the non-button elements turned into buttons; the notificationbox displayed them correctly but bwinton vetoed the UI because it looked really ugly.
Comment on attachment 552067 [details] [diff] [review]
Made the notification bar look slicker. (v9)

Since the patch by Irving works slightly better, I'm setting this particular patch to feedback minus.
Attachment #552067 - Flags: feedback?(nisses.mail) → feedback-
Comment on attachment 552067 [details] [diff] [review]
Made the notification bar look slicker. (v9)

Realized that Irving's patch of course need this patch applied first (I had them mixed up in my queue). So yeah, giving feedback+ to this. Having a combined patch would be great, and having phishing working would be even better! :)
Attachment #552067 - Flags: feedback- → feedback+
Included irving's patch changes.

Phishing is still MIA.
Attachment #552067 - Attachment is obsolete: true
Attachment #563271 - Flags: feedback?(nisses.mail)
Attachment #563271 - Flags: feedback?(nisses.mail) → feedback+
Unbitrotted patch
Attachment #563271 - Attachment is obsolete: true
Attachment #575077 - Flags: feedback?(nisses.mail)
Comment on attachment 575077 [details] [diff] [review]
Made  the notification bar look slicker. (v11)

Phishing is still MIA.
With this patch, my debug build of Daily dumps an exception message to standard error when I display a phishing message:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "checkMsgHdrPropertyIs is not defined" {file: "chrome://messenger/content/mailWindowOverlay.js" line: 2762}]' when calling method: [nsIUrlListManagerCallback::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///Users/ireid/tbird/objdir-comm-central-default/mozilla/dist/DailyDebug.app/Contents/MacOS/components/nsUrlClassifierListManager.js :: <TOP_LEVEL> :: line 25"  data: yes]
************************************************************

It looks like you copied the phishing message classification code from the comm-central/suite source tree into the Thunderbird tree. You didn't need to do this; Thunderbird already has similar code in comm-central/source/mail/base/content/phishingDetector.js. This code runs before (and calls) setPhishingMsg(), so we already know the message is a scam; we don't need to do extra tests in setPhishingMsg().

When I took out the extra scam checking code and made setPhishingMsg() just display the notification, it works for me. I've attached a patch, again to apply over the v11 patch.
Apply ewong's v11 patch first, and then this patch. The "this message may be a scam" message should show in the notification box.

There are still some issues to clean up after this:

 - icons don't appear in the notification box in my build
 - "allow remote content for all messages from <address>" link doesn't show up in remote content notification
Attachment #557555 - Attachment is obsolete: true
incl irving's changes.
Attachment #575077 - Attachment is obsolete: true
Attachment #577261 - Flags: feedback?(irving)
Attachment #575077 - Flags: feedback?(nisses.mail)
Comment on attachment 577261 [details] [diff] [review]
Made  the notification bar look slicker. (v12)

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

You didn't remove all the code you had previously pulled in from SeaMonkey; everything from the "+" sign on a line by itself (mailWindowOverlay.js around line 3378, just before "function checkMsgHdrPropertyIs(aProperty, aValue)") through to the end of the file should be removed. That's checkMsgHdrPropertyIs, msgHdrForCurrentMessage, GetLoadedMessage, and isMsgEmailScam.

Similarly, you modified phishingDetector.js to pass the aUrl parameter to setPhishingMsg(); this is no longer needed, so you can revert that change.

I missed one thing in my update to your v11 patch - in mailWindowOverlay.js, the button callback in setPhishingMsg() (around line 2765) should call IgnorePhishingWarning(), not MsgIsNotAScam() - the latter is only implemented in SeaMonkey, not TB.


Once these are cleaned up, we still need to get the icons to show up properly in the notification windows, and re-implement the other features currently available in Thunderbird:

For the Phishing / Scam Detection window, the "Disable scam detection for all messages" link

For the Remote Content window, the "Always load remote content from <address>" link
Attachment #577261 - Flags: feedback?(irving) → feedback+
Attachment #577261 - Attachment is obsolete: true
Attachment #577915 - Flags: feedback?(irving)
Comment on attachment 577915 [details] [diff] [review]
Made  the notification bar look slicker. (v13)

Minor bit-rot on the latest attachment, one of the themes didn't patch cleanly:

patching file mail/themes/qute/jar.mn
Hunk #1 FAILED at 221
Hunk #2 FAILED at 437
2 out of 2 hunks FAILED -- saving rejects to file mail/themes/qute/jar.mn.rej


Aside from that...

In 3 pane view, when I switch focus to a message with both remote images and a fake URL to trigger the scam warning(https://bugzilla.mozilla.org/attachment.cgi?id=592136), I see the following on standard error:

JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 2791: mdnBarMsg is null
Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-ewong/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/remote-blocked.png
WARNING: We should have hit the document element...: file /Users/ireid/tbird/comm-central/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 213
Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-ewong/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/remote-blocked.png
Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-ewong/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/question.png
WARNING: We should have hit the document element...: file /Users/ireid/tbird/comm-central/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 213
Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-ewong/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/question.png



Looking at the patch:

In phishingDetector.js

> 213 	if (aLocalListStatus == 0 /* PROT_ListWarden.IN_BLACKLIST */ ||
> 214          (aLocalListStatus == 2 /* PROT_ListWarden.PROT_ListWarden.NOT_FOUND */ && aFailsStaticTests))
> 215        gMessageNotificationBar.setPhishingMsg(aUrl);

setPhishingMsg() doesn't take any parameters, so remove aUrl


In messenger.properties

> 519 remoteContentBarMessage=To protect your privacy, %S has blocked remote content in this message.

Should we have a localization note on this line saying that %S is the product name?



We're still missing some of the extra actions from the existing notifications, such as the "always accept remote content from <address>" link. I think we're going to need some help from the XUL team to figure out how to implement those.
Attachment #577915 - Flags: feedback?(irving) → feedback+
Unbitrotted.
Attachment #577915 - Attachment is obsolete: true
Attachment #600611 - Flags: ui-review?(nisses.mail)
Attachment #600611 - Attachment is obsolete: true
Attachment #600614 - Flags: ui-review?(nisses.mail)
Attachment #600611 - Flags: ui-review?(nisses.mail)
Comment on attachment 600614 [details] [diff] [review]
Made the notification bar look slicker. (v15)

I tested v15 of the patch on Mac, and these are the issues I identified:
* Junk mail bar lacks an icon
* Remote content bar don't show
* Phishing bar don't show
* Draft message bar is white (should be yellow) and the button is different from the other notification bars
Attachment #600614 - Flags: ui-review?(nisses.mail) → ui-review-
Attachment #600614 - Attachment is obsolete: true
Attachment #610118 - Flags: feedback?(irving)
(In reply to Andreas Nilsson (:andreasn) from comment #55)
> Comment on attachment 600614 [details] [diff] [review]
> Made the notification bar look slicker. (v15)
> 
> I tested v15 of the patch on Mac, and these are the issues I identified:
> * Junk mail bar lacks an icon
> * Remote content bar don't show
> * Phishing bar don't show

Current progress on this bug: 
- icon added to Junk mail bar
- Phishing bar is now showing.

* Having major difficulties with getting the Remote Content Bar to show
  (with and without my patch).  irving's phishing example does give me
  the phishing bar, but no Remote Content bar (again, with and without my
  patch applied).

> * Draft message bar is white (should be yellow) and the button is different
> from the other notification bars
 
  Not exactly sure where this draft message bar is.
(In reply to Edmund Wong (:ewong) from comment #57)

> > * Draft message bar is white (should be yellow) and the button is different
> > from the other notification bars
>  
>   Not exactly sure where this draft message bar is.


Never mind.  Found it.
Attachment #610499 - Flags: review?(nisses.mail)
Attachment #610118 - Attachment is obsolete: true
Attachment #610118 - Flags: feedback?(irving)
(In reply to Edmund Wong (:ewong) from comment #59)
> Created attachment 610499 [details] [diff] [review]
> Made the notification bar look slicker. (v17)

This version puts up multiple copies of the "remote content" notification. These lines:

+    var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("remoteContent");
+    if (oldNotif == null) {
+      var bar =
+        this.mMsgNotificationBar.appendNotification(remoteContentMsg, 1,
+          "chrome://messenger/skin/icons/remote-blocked.png",
+          this.mMsgNotificationBar.PRIORITY_WARNING_MEDIUM,
+          buttons);


You need to pass the same identifier string "remoteContent" as the second parameter to mMsgNotificationBar.appendNotification(), as you use in the getNotificationWithValue() call, like so:

+    var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("remoteContent");
+    if (oldNotif == null) {
+      var bar =

*** change 1 to "remoteContent"
+        this.mMsgNotificationBar.appendNotification(remoteContentMsg, "remoteContent",
***

+          "chrome://messenger/skin/icons/remote-blocked.png",
+          this.mMsgNotificationBar.PRIORITY_WARNING_MEDIUM,
+          buttons);

Other than that, looking good.
I think this patch has too much javascript in it for me to do code review, but I'm happy to do a ui-review for you instead.

He're some feedback in the meantime:
* Draft - OK!
* Remote content - Needs icon (I will fix this)
* Junk mail - Needs icon (I will fix this)
* Scam - when this pops up I get a bunch of other bars under it that I also have to close. This sounds similar to what Irving experienced above.

I'm really happy with the progress on this bug. It's starting to look really slick on the mac now!
the Draft Message should use (atm) info.png, but it's not showing.  Need some help with understanding why it's failing to find chrome://messenger/skin/icons/info.png.
Attachment #610499 - Attachment is obsolete: true
Attachment #611356 - Flags: ui-review?(nisses.mail)
Attachment #610499 - Flags: review?(nisses.mail)
Attached patch new icons (obsolete) — Splinter Review
This patch (to be applied on top of Edmunds v18 patch) should take care of the missing icons on OSX and the bad scaling of icons on Windows and Linux.
Comment on attachment 611356 [details] [diff] [review]
Made the notification bar look slicker. (v18)

canceling review as we'll have a new patch with the new graphics up soon.
Attachment #611356 - Flags: ui-review?(nisses.mail)
This is really close now!
Two issues I've identified that's a regression compared to what we have now:
* If a scam message have several suspicious links, this patch will trigger one scam notification bar for each.
* Closing the junk message works fine, but pressing the "Not junk" button sometimes trigger a new junk mail notification.

Apart from that, all looks good!
Attached patch combined patch (obsolete) — Splinter Review
If qfold worked as it should this should be a combined patch with Edmunds v18 from attachment 611356 [details] [diff] [review] and my new icons from attachment 613622 [details] [diff] [review]
Attachment #611356 - Attachment is obsolete: true
Attachment #613622 - Attachment is obsolete: true
We can add XUL elements directly as children of the Notification object, as in the example below (this is on top of andreasn's most recent version of the patch).

The appendNotification call (just above this diff) returns a XUL Notification object, and adds the buttons as the last direct children of the Notification. We can add additional children to appear before or after the buttons; the example below puts the child after, but we want to change that so it comes before.

Using just this change, the button moves over to make room for the label but the label text isn't actually visible. I haven't figured out what the problem with that is yet.

--- a/mail/base/content/mailWindowOverlay.js
+++ b/mail/base/content/mailWindowOverlay.js
@@ -2750,19 +2750,17 @@ var gMessageNotificationBar =
       var linkLabel = bar.ownerDocument.createElementNS(XULNS, "label");
       var addedLink = this.stringBundle.getFormattedString('alwaysLoadRemoteContentForSender2', 
                        [emailAddress ? emailAddress : aMsgHdr.author]);
 
       linkLabel.className = "text-link";
       linkLabel.setAttribute("value", addedLink);
       linkLabel.onclick = allowRemoteContentForSender;
 
-      var nContainer = bar.ownerDocument.getAnonymousElementByAttribute(bar,
-                        "anonid", "messageText");
-      nContainer.appendChild(linkLabel);
+      bar.appendChild(linkLabel);
     }
   },
 
   setPhishingMsg: function()
   {
     var phishingMsgNote = this.stringBundle.getString('phishingBarMessage');
 
     var buttons = [{
Here's a patch that gets "Always allow remote content from..." working - there were a number of things that needed fixing, including inserting our XUL in the right place and fixing a typo in the XUL namespace string.

Also cleaned up the onClick callback to pass parameters through rather than re-calculating the sender name/address.

Still seeing multiple copies of the "scam" notice when there is more than one bad URL in the message.


There is another approach, which I haven't tried yet, which is to attach a popup menu to the button; the only example I could find in the browser source tree is at http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#495 / http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.xul#261
Attachment #614572 - Flags: feedback?(ewong)
Attachment #614572 - Flags: feedback?(ewong) → feedback+
Patch that includes irving's diff.
Attachment #619028 - Flags: ui-review?(nisses.mail)
It looks like I get a [object MouseEvent] filled out as the e-mail address if I press the "always load remote content..." with patch v19.

Apart from that everything looks good. ui-r+ on patch v19 with that fixed.
Attachment #619028 - Attachment is obsolete: true
Attachment #619028 - Flags: ui-review?(nisses.mail)
Attachment #629568 - Flags: ui-review?(nisses.mail)
Wow! As I could find out by chance today in a real-world experience, the phishing warning works nice with v20 and is very eye-catching. :-)
Seems there have been some bitrot in a couple of files.
Attachment #629568 - Attachment is obsolete: true
Attachment #629568 - Flags: ui-review?(nisses.mail)
Attachment #632239 - Flags: ui-review?(nisses.mail)
Fixed the v21 unbitrotted patch.
Attachment #632239 - Attachment is obsolete: true
Attachment #632239 - Flags: ui-review?(nisses.mail)
Attachment #632518 - Flags: ui-review?(nisses.mail)
Attached image screenshot of oddity
This is a similar visual glitch that happens in bug #759744 and not directly related to this bug, but I thought I should still mention it.
There is an issue that the above screenshot has &brandShortName; in it though.
v22 patch somehow lost the phishing part changes.
Attachment #632518 - Attachment is obsolete: true
Attachment #632518 - Flags: ui-review?(nisses.mail)
Attachment #632534 - Flags: ui-review?
Attachment #632534 - Flags: ui-review?
Fixed oddity and phishing from v21.
Attachment #632534 - Attachment is obsolete: true
Attachment #632538 - Flags: ui-review?
Attachment #632538 - Flags: ui-review? → ui-review?(nisses.mail)
Edmund, I hate to throw another wrinkle onto this patch, but can you make sure it doesn't have the same problem as bug 761654?
All seems to work fine (tested on Linux and Mac so far). There is a scrollbar appearing sometimes, but that's a bug about notification bars in TB overall, so that's being taken care of in bug #759744. Will test on Windows before I give ui-r+
Comment on attachment 632538 [details] [diff] [review]
Make  the notification bar look slicker. (v24)

Looks good on Windows too. ui-r+!
Attachment #632538 - Flags: ui-review?(nisses.mail) → ui-review+
Attachment #632538 - Flags: review?(bwinton)
Comment on attachment 632538 [details] [diff] [review]
Make  the notification bar look slicker. (v24)

So, first off, the Remote Content notification still looks a little weird for me, as shown in http://dl.dropbox.com/u/2301433/Screenshots/remoteContent.png

>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -2629,52 +2629,129 @@ var gMessageNotificationBar =
>   get mMsgNotificationBar() {
>     delete this.mMsgNotificationBar;
>+    delete this.stringBundle;
>+    delete this.brandBundle;
>+
>+    this.stringBundle = document.getElementById('bundle_messenger');
>+    this.brandBundle = document.getElementById('brand_bundle');

Why do we need to do this every time we get the notification bar?

>   setJunkMsg: function(aMsgHdr)
>   {
>     var isJunk = false;
>-
>-    if (aMsgHdr)
>-    {
>+    var junkBarMsg = this.stringBundle.getString('junkBarMessage');
>+
>+    if (aMsgHdr) {
>       var junkScore = aMsgHdr.getStringProperty("junkscore");
>       isJunk = ((junkScore != "") && (junkScore != "0"));
>     }
> 
>-    this.updateMsgNotificationBar(kMsgNotificationJunkBar, isJunk);
>-
>-    goUpdateCommand('button_junk');

Given that mconley has a bug open to move more things to commands (so that we can track them in Test Pilot), this seems like an odd change to make.

>@@ -2683,38 +2760,71 @@ var gMessageNotificationBar =
>-    let mdnBarMsg = document.getElementById("mdnBarMessage");
[snip…]
>+    var mdnBarMsg2 = this.stringBundle.getString('mdnBarMessage');

Why not just call this mdnBarMsg?

>+    this.mMsgNotificationBar.appendNotification(mdnBarMsg2, 1,
>+      "chrome://messenger/skin/icons/info.png",
>+      this.mMsgNotificationBar.PRIORITY_INFO_MEDIUM,
>+      buttons);

The other functions check stuff like:
>+    var oldNotif = this.mMsgNotificationBar.getNotificationWithValue("phishingContent");
>+    if (oldNotif == null) {

Why aren't we doing that here?

>+  setDraftEditMessage: function() {
[snip…]
>+    this.mMsgNotificationBar.appendNotification(draftMsgNote, "draftMsgContent",
>+      "chrome://messenger/skin/icons/info.png",
>+      this.mMsgNotificationBar.PRIORITY_WARNING_HIGH,
>+      buttons);

Or here?

>@@ -2750,35 +2860,18 @@ function LoadMsgWithRemoteContent()
>-function allowRemoteContentForSender()
>+function allowRemoteContentForSender(authorEmailAddress, authorDisplayName)
> {
>-  // get the sender of the msg hdr
>-  var msgHdr = gMessageDisplay.displayedMessage;

Hurray for less reliance on globals!  :)

>+++ b/mail/base/content/mailWindowOverlay.xul
>@@ -2002,65 +2003,16 @@
>-<!-- The msgNotificationBar appears on top of the message and displays
>-     information like: junk, contains remote images, or is a suspected phishing
>-     URL.
>--->

I think we might want to keep this comment, but…

>+++ b/mail/base/content/messageWindow.xul
>@@ -142,18 +142,17 @@
>-      <hbox id="editMessageBox"/>
>-      <deck id="msgNotificationBar"/>
>+      <notificationbox id="msgNotificationBar"/>

put it here instead.

>+++ b/mail/base/content/messenger.xul
>@@ -492,18 +492,17 @@
>-                      <hbox id="editMessageBox"/>
>-                      <deck id="msgNotificationBar"/>
>+                      <notificationbox id="msgNotificationBar"/>

and maybe here, too.

>+++ b/mail/locales/en-US/chrome/messenger/messenger.properties
>@@ -289,16 +289,36 @@ feedsAcctType=Feeds
>+junkBarMessage=This message is Junk mail.

Huh, that's not the same as the previous message ("&brandShortName; thinks this message is junk mail.").  And seems to posses a certainty I don't think is warranted.

>+remoteContentBarMessage=To protect your privacy, %S has blocked remote content in this message.

This is going to need a localization note (https://developer.mozilla.org/en/Localization_notes) explaining what %S is.

>+phishingBarMessage=This message may be a scam.
>+phishingBarIgnoreButton=Ignore Warning
>+phishingBarIgnoreButtonKey=I
>+phishingBarErrorMessage=This message doesn't appear to be a scam

What happened to the "Disable scam detection for all messages" message?  Do we not offer that option anymore, and if not, why not?


Aside from that, it seems reasonable, but I think I'm going to say r- so that I can double-check the fixes, and also because I'm some crazy masochist who likes reviewing 1150-line, 57.71 KB patches.  :P

Thanks,
Blake.
Attachment #632538 - Flags: review?(bwinton) → review-
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #83)
> Comment on attachment 632538 [details] [diff] [review]
> Make  the notification bar look slicker. (v24)
> 
> So, first off, the Remote Content notification still looks a little weird
> for me, as shown in
> http://dl.dropbox.com/u/2301433/Screenshots/remoteContent.png

This is probably because of bug #759744 not been checked in yet.
I need a bit of a feedback on this patch.  After unbitrotting it after updating
my tree,  now for RemoteContent notification bar, when I click on a message
that has it, the "New Contact" dialog appears.  I click cancel and it appears
again, ad infinitum until I close the main TB window (which requires precision
clicking).
Attachment #647473 - Flags: feedback?(irving)
Comment on attachment 647473 [details] [diff] [review]
Make the notification bar look slicker. (v25)

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

Some of the graphics don't seem to be there on the Mac version yet:

Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-default/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/remote-blocked.png
Chrome file doesn't exist: /Users/ireid/tbird/objdir-comm-central-default/mozilla/dist/DailyDebug.app/Contents/MacOS/chrome/classic/skin/classic/messenger/icons/phishing.png

::: mail/base/content/mailWindowOverlay.js
@@ +2719,5 @@
> +          this.mMsgNotificationBar.PRIORITY_WARNING_MEDIUM,
> +          buttons);
> +
> +      if (emailAddress) {
> +        var XULNS = "http://www.mozilla.org/keymaster.gatekeeper/there.is.only.xul";

In v.24 of the patch, where this worked, this line was

var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

(slash, rather than dot, between keymaster and gatekeeper). I think this error was in some earlier work in progress on this patch, seems to have regressed.

@@ +2727,5 @@
> +
> +        linkLabel.className = "text-link";
> +        linkLabel.setAttribute("value", addedLink);
> +        linkLabel.onclick = allowRemoteContentForSender(emailAddress,
> +                                                        displayName);

In v.24 this was

        linkLabel.onclick = function(){allowRemoteContentForSender(emailAddress,                                                                      displayName); };

which was correct.

@@ +2755,5 @@
> +          "chrome://messenger/skin/icons/phishing.png",
> +          this.mMsgNotificationBar.PRIORITY_CRITICAL_MEDIUM,
> +          buttons);
> +
> +      var XULNS = "http://www.mozilla.org/keymaster.gatekeeper/there.is.only.xul";

Again, should be

var XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
Attachment #647473 - Flags: feedback?(irving) → feedback+
How does this look in MacOSX?
Attachment #647473 - Attachment is obsolete: true
Attachment #651257 - Flags: feedback?(irving)
Hm, I don't know why, but with v26 I don't get any notification bar at all on OS X (checked with some Facebook mails which normally gives me the yellow notification bar). I build this patch into comm-beta, because comm-central doesn't build for me currently, but I don't think this could be the reason (it applied cleanly and your older patch worked just fine).
Comment on attachment 651257 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications (v26)

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

After I manually cleaned up the errors in the patch, it passed a couple of quick checks but would still need full testing

::: mail/base/content/mailWindowOverlay.js
@@ +2700,5 @@
>                         .getFormattedString("alwaysLoadRemoteContentForSender2",
>                                             [emailAddress ? emailAddress : aMsgHdr.author]);
> +    var displayName = headerParser.extractHeaderAddressName(aMsgHdr.author);
> +    var brandName = this.brandBundle.getString('brandShortName');
> +    var remoteContentMsg = this.mMtringBundle.getFormattedString('remoteContentBarMessage', 

should be mStringBundle

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +151,5 @@
>  sentFolderName=Sent
>  draftsFolderName=Drafts
> +temp# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Looks like a big chunk of stuff got moved or pasted accidentally?
Attachment #651257 - Flags: feedback?(irving) → feedback-
Attachment #651257 - Attachment is obsolete: true
Attachment #655267 - Flags: feedback?(irving)
Comment on attachment 655267 [details] [diff] [review]
Make use of the toolkit's notificationbox to display notifications (v27)

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

The patch for mail/locales/en-US/chrome/messenger/messenger.properties still inserts a duplicate copy of lines 4 through 152, and removes text starting partway through line 549 until partway through line 691 of the original file. Please review all the differences in this file carefully and revert any that don't relate specifically to this bug.
Attachment #655267 - Flags: feedback?(irving) → feedback-
(In reply to Irving Reid (:irving) from comment #91)
> Comment on attachment 655267 [details] [diff] [review]
> Make use of the toolkit's notificationbox to display notifications (v27)
> 
> Review of attachment 655267 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch for mail/locales/en-US/chrome/messenger/messenger.properties still
> inserts a duplicate copy of lines 4 through 152, and removes text starting
> partway through line 549 until partway through line 691 of the original
> file. Please review all the differences in this file carefully and revert
> any that don't relate specifically to this bug.

Thanks for the feedback.  I'm completely baffled as to what happened with
that.
Ewong still working on this ?
Yes he is but he's got some higher priority stuff to deal with first.
Because I wanted to try the latest patch out, but it was bitrotted, I unbitrotted it and fixed the comments for messenger.properties in #91. Maybe this also will help others, therefore I've added it here.
Sorry, patch accidently missed some of the images...
Attachment #692451 - Attachment is obsolete: true
Attachment #692511 - Flags: review?(irving)
Simply removed all whitespace that was in the patch.
Attachment #692511 - Attachment is obsolete: true
Attachment #692511 - Flags: review?(irving)
Attachment #695687 - Flags: review?(irving)
By the way, I don't think it's a good idea to hardcode all the images, because this will interfere with Bug 781333 and we will need 2x images of all png's (see also Bug 800123).
Comment on attachment 695687 [details] [diff] [review]
v27 - unbitrotted to current trunk (2012-12-26)

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

I'm seeing warning messages on stderr,

WARNING: We should have hit the document element...: file /Users/ireid/tbird/comm-central/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 177

I suspect these are relatively harmless side effects of the GetNotificationBox call, but I can't remember for sure.

The code works with my tests, as it stands - it still needs UI review and cleaning up of the icon references as pointed out by Nomis101.
Attachment #695687 - Flags: review?(irving) → feedback+
Attachment #655267 - Attachment is obsolete: true
Attachment #632538 - Attachment is obsolete: true
Attachment #614572 - Attachment is obsolete: true
Assignee: ewong → nobody
Status: ASSIGNED → NEW
Keywords: helpwanted
Attached image Scam warning
Attachment #468697 - Attachment is obsolete: true
Attachment #511015 - Attachment is obsolete: true
Attachment #575742 - Attachment is obsolete: true
Attachment #613971 - Attachment is obsolete: true
Attachment #702433 - Flags: ui-review?(bwinton)
Attached image Junk Message warning
Attachment #702437 - Flags: ui-review?(bwinton)
Comment on attachment 702433 [details]
Scam warning

I think we should be using a white shield logo, and a white "x", but other than that, ui-r=me!

Thanks,
Blake.
Attachment #702433 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 702435 [details]
Remote content warning

We seem to be saying "remote content" a lot.  Could we change the "always load" message to be a little shorter, so that the main message doesn't have to wrap?
Attachment #702435 - Flags: ui-review?(bwinton) → ui-review-
Comment on attachment 702436 [details]
Message Disposition Notification question

As the first attachment, the icon and "x" should be in white.  I also think we can tighten up the wording so that it doesn't take two lines.
Attachment #702436 - Flags: ui-review?(bwinton) → ui-review-
Comment on attachment 702437 [details]
Junk Message warning

What is the "?"?  (Other than that, I like it!)
Attachment #702437 - Flags: ui-review?(bwinton) → ui-review+
(In reply to Blake Winton (:bwinton) from comment #107)
> Comment on attachment 702437 [details]
> Junk Message warning
> 
> What is the "?"?  (Other than that, I like it!)

The "?" button brings up one of those pull-down overlays from the top of the window, containing a brief explanation of our junk mail system.
Okay, let's pick a better label for that button, too, then.  ;)
Comment on attachment 702436 [details]
Message Disposition Notification question

(MDN, not DSN)

This already have better text, the patch just isn't using what we have.
Attachment #702436 - Attachment description: Delivery Status Notification warning → Message Disposition Notification question
(In reply to Blake Winton (:bwinton) from comment #105)
> Comment on attachment 702435 [details]
> Remote content warning
> 
> We seem to be saying "remote content" a lot.  Could we change the "always
> load" message to be a little shorter, so that the main message doesn't have
> to wrap?

Or perhaps have something like what the firefox popup blocker uses? (A "Preferences" button.)
I think this bug would be well worth persuing...
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Keywords: helpwanted
Attached patch proposed fix, v28 (obsolete) — Splinter Review
This is an updated version of what was earlier put together. This bug is long enough to give anyone an headache trying to read it. I think earlier colors of the phishing and remote content icons was requested to be changed. I haven't done that but besides that the patch should be working. 

There was an info icon added earlier in the patches, i removed that as we can choose not to specify one and get the toolkit notification info icon by using a suitable priority.

Not in this bug, but for remote content it would be a huge win to be able to whitelist content from some certain domains (not senders, but from the domain the content is on). This new bar should make ui for that way easier. 

I'll attach some screenshots
Attachment #695687 - Attachment is obsolete: true
Attachment #702436 - Attachment is obsolete: true
Attachment #770401 - Flags: ui-review?(bwinton)
Attachment #770401 - Flags: review?(bwinton)
Attached image screenshots v28 before
Attached image screenshots v28 after
Comment on attachment 770401 [details] [diff] [review]
proposed fix, v28

ui-r=me, based on the screenshots.  One thing I will suggest is that the black looks really dark.  Can we change it to the yellow that got a previous ui-r+ earlier in this bug?

(review will come later, but if anyone else wants to steal it, please go ahead…)
Attachment #770401 - Flags: ui-review?(bwinton) → ui-review+
The notification colors are all not "tampered" with, they come from what toolkit is using and depends on what priority you set for the notification. I didn't change what was given ui-r before, but i guess that screenshot is from another os (osx? mine were on ubuntu). Or toolkit changed from yellow to black for warning notifications. Let me know if you still want it changed.

One thing i'd still change is the [Edit...] button on drafts. That should be just [Edit] (no dots)
(In reply to Magnus Melin from comment #117)
> I didn't change what was given ui-r before, but i guess that screenshot is
> from another os (osx? mine were on ubuntu).
Yes, the screenshots with the yellow bar are from OS X.
Okay, I'll check out how it looks when I review the code on Mac…

Thanks!
Comment on attachment 770401 [details] [diff] [review]
proposed fix, v28

So, I think this is probably okay, but every time I run it and try to flag a message as junk (with the "j" key), I get the following assertion:

Assertion failure: _mOwningThread.GetThread() == PR_GetCurrentThread() (nsMsgFolderNotificationService not thread-safe), at /Volumes/SSD/Programming/thunderbird/comm-central/mailnews/base/src/nsMsgFolderNotificationService.cpp:17

So, uh, obviously that needs to get fixed first.  ;)

>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -2764,158 +2759,265 @@ function HandleJunkStatusChanged(folder)
>   // Only reload message if junk bar display state has changed.
>-  if (msgHdr && junkBarWasDisplayed != gMessageNotificationBar.isFlagSet(kMsgNotificationJunkBar))
>+  if (msgHdr && junkBarWasDisplayed != gMessageNotificationBar.isShowingJunkNotification())
>   {
>+

I don't think the extra blank line here adds much.

>-      // If the current row  isn't going to change, reload to show sanitized or
>+// XXXmagnus: the below doesn't really work for non-inbox...
>+
>+      // If the current row isn't going to change, reload to show sanitized or

We should fix that before committing.  ;)

> var gMessageNotificationBar =
> {
>+  get mStringBundle() {
>+    delete this.stringBundle;
>+    return this.stringBundle = document.getElementById("bundle_messenger");
>+  },
>+
>+  get mBrandBundle() {
>+    delete this.brandBundle;
>+    return this.brandBundle = document.getElementById("brand_bundle");

Why are we deleting it every time we get it?  Can't we cache it at all?

>+++ b/mail/base/jar.mn
>@@ -10,17 +10,17 @@ messenger.jar:
>-    content/messenger/mailWindowOverlay.js          (content/mailWindowOverlay.js)
>+*   content/messenger/mailWindowOverlay.js          (content/mailWindowOverlay.js)

So, as I understand it, this means that we're going to pre-process the mailWindowOverlay.js file, which is something I think we try to avoid doing.  Is there an attribute on something we could use instead?  At worst, we could make a variable of our own.  Something like http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/telemetry/TelemetryPing.js#123

>+++ b/mail/test/mozmill/composition/test-attachment-reminder.js
>@@ -6,61 +6,64 @@
> // I'm not sure why this is the ID. But it is. :/
>+// XXX for ^^^ nBox.appendNotification("", "1",    in MsgComposeCommands.js
> const kNotificationID = "1";

Can we change the comment above, instead of adding an "XXX"?

Apart from those, I'm pretty happy with the patch.  r=me with the above things fixed.

Thanks,
Blake.
Attachment #770401 - Flags: review?(bwinton) → review+
(In reply to Blake Winton (:bwinton) from comment #120)
> >+  get mBrandBundle() {
> >+    delete this.brandBundle;
> >+    return this.brandBundle = document.getElementById("brand_bundle");
> 
> Why are we deleting it every time we get it?  Can't we cache it at all?

Yeah i don't know why its done like this. It seems to be a common pattern though. Anyone know why it's done this way?
I have not seen this pattern anywhere. The bundles are usually cached.
> Why are we deleting it every time we get it?  Can't we cache it at all?
Yes this code pattern is supposed to cache the bundle. Unfortunately it's been cargo culted, and cargo culted *wrong*.

+  get mStringBundle() {
+    delete this.stringBundle;
+    return this.stringBundle = document.getElementById("bundle_messenger");
+  },

Should be:
+  get stringBundle() {
+    delete this.stringBundle;
+    return this.stringBundle = document.getElementById("bundle_messenger");
+  },
So it's supposed to be a lazy getter which deletes itself the first time it's called. The second time around it's a javascript property instead.

Better MXR query:
http://mxr.mozilla.org/mozilla-central/search?string=delete+this.&find=%2Fbrowser%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
OK, so the only difference is that the 'bundle' should be the same string in: 
get bundle() {
delete this.bundle;
return this.bundle = ... ;
}

So the patch needs to be fixed with this.
Attached patch proposed fix, v29 (obsolete) — Splinter Review
This should address the review comments. 
Added Application.platformIsWindows for platform checking. I also had to fix some more tests.

I don't see the assertion, so i assume that's something unrelated that has gone away.
Attachment #770401 - Attachment is obsolete: true
Attachment #778871 - Flags: ui-review+
Attachment #778871 - Flags: review?(bwinton)
Unbitrotted
Attachment #778871 - Attachment is obsolete: true
Attachment #778871 - Flags: review?(bwinton)
Attachment #814173 - Flags: ui-review+
Attachment #814173 - Flags: review?(bwinton)
Blake: ping on the re-review
Comment on attachment 814173 [details] [diff] [review]
proposed fix, v30

So, I can't build on OS X 10.9, but I went through the differences between this and the previously r+ed version, and they all make sense to me, so I'll say r=me based on that.  (Obviously, I'm assuming the tests all pass and stuff too.  ;)

Thanks,
Blake.
Attachment #814173 - Flags: review?(bwinton) → review+
(In reply to Magnus Melin from comment #129)
> Blake: ping on the re-review

Magnus can you start a try build ?
Try looks good.
https://hg.mozilla.org/comm-central/rev/752e81dfb822 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 28.0
Blocks: 935179
Depends on: 938783
Blocks: 926473
No longer blocks: 926473
Depends on: 926473
No longer blocks: 935179
Depends on: 935179
Depends on: 939982
Depends on: 968660
Depends on: 1005687
Adding some keywords to summary to improve archaeological search results.
Had a hard time figuring out that this is where [Show Remote Content] Button was replaced by [Options] dropdown button.
Depends on: 1062961
Summary: notification bar needs to look slicker → Notification bar needs to look slicker (Remote Content, Junk, Scam, Send Return Receipt, Edit Draft)
Blocks: 539416
Blocks: 457296
Depends on: 1064501
You need to log in before you can comment on or make changes to this bug.