296758 - Adding phishing detection to mailnews

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Ian Neal]

This is the equivalent of TB bug 279191 and includes the rewrite of "JunkBar".

Comment 1

[Ian Neal]

Attachment: Provisional Patch v0.1a

This patch:
* Rewrites "Junk Bar"
* Adds phishing support to mailnews
* Deals with precedence of msg bars better than TB - remembers if mail was
possible phish or had remote content even when other msg bars take precedence

Comment 2

[Ian Neal]

I think in contentAreaClick.js the line:
 if (!gPrefBranch.getBoolPref("mail.phishing.detection.enabled"))
needs to be changed to:
 if (!pref.getBoolPref("mail.phishing.detection.enabled"))

so that it works in both navigator and mailnews.

Comment 3

[Ian Neal]

Attachment: Tweaked provisional patch v0.1b

Changes since v0.1a:
* Changed contentAreaClick.js so that it finds the correct bundle depending on
if it is used from browser or mailnews
* Changed contentAreaClick.js so that it can find pref value.

Comment 4

[Ian Neal]

Attachment: Revised patch v0.1c

Changes since v0.1b:
* Add missing mailnews.js file changes
* This bug is just to do with mailnews so let's not phish detect for browsers
(that can be another patch if it is needed)

I need someone to test on xlinks to make sure my changes to contentAreaClick.js
have not broken anything.

Comment 5

[Ian Neal]

To get Remote Image message bar working there are three options I can see:
1) Drop extensions/permissions/nsMailnewsContentBlocker.cpp and use
mailnews/base/src/nsMsgContentPolicy.cpp with the nsMsgCookiePolicy bits ifdef'd
out so only TB uses them.
2) Drop extensions/permissions/nsMailnewsContentBlocker.cpp and use
mailnews/base/src/nsMsgContentPolicy.cpp with the nsMsgCookiePolicy bits moved
out into a separate file for TB to use.
3) Keep using extensions/permissions/nsMailnewsContentBlocker.cpp and copy all
the extra bits in from mailnews/base/src/nsMsgContentPolicy.cpp (i.e. all of it
except the nsMsgCookiePolicy bits).

I'd say my least favourite option is the 3rd one, but it is possible TB folks
will not let the first two options through. Any thoughts?

Comment 6

[Ian Neal]

cc'ing mvl as may have a view because wrote the original
nsMailnewsContentBlocker.cpp

Comment 7

[Karsten Düsterloh]

Comment on attachment 185460 [details] [diff] [review]
Revised patch v0.1c

>Index: mailnews/base/resources/content/mailWindow.js
>===================================================================
>-function loadStartPage() {
[...]
>-                // first, clear out the charset setting.
>-                messenger.setDisplayCharset("");

What happened to this?

>+      var startpage = pref.getComplexValue("mailnews.start_page.url",
>+                                           Components.interfaces.nsIPrefLocalizedString).data;
>+      if (startpage != "")

If startpage is null (for whatever broken background reason), this will be
true, so better use
|if (startpage)|.

>Index: mailnews/base/resources/content/mailWindowOverlay.js
>===================================================================
> function HandleJunkStatusChanged(folder)
[...]
>+    if (GetNumSelectedMessages() == 1)
>+      var msgHdr = messenger.messageServiceFromURI(loadedMessage).messageURIToMsgHdr(loadedMessage);
>+      if (msgHdr)
>+        gMessageNotificationBar.setJunkMsg(msgHdr);
>     else
>-      SetUpJunkBar(null);
>+      gMessageNotificationBar.setJunkMsg(null);

This means, that if msgHdr is invalid, we don't even call setJunkMsg...
Better move the actual call to setJunkMsg out of the if.

>+  setJunkMsg: function (aMsgHdr)

Inferior nit: remove the space before (.

>+    return (isJunk && isCurrentlyNotJunk) || (!isJunk && !isCurrentlyNotJunk);

You mean |return isJunk == isCurrentlyNotJunk;| I suppose? ;-)
But why return this at all? It isn't used anywhere and all the other
gMessageNotificationBar.set*Msg functions don't return their state either, so
just kick it...

>+  setRemoteContentMsg: function (aMsgHdr)

Inferior nit: remove the space before (.

>+  // aUrl is the nsIURI for the message currently loaded in the message pane
>+  setPhishingMsg: function(aUrl)
>+  {
>+    var msgURI = GetLoadedMessage();
>+    if (msgURI && !(/type=x-message-display/.test(msgURI)))
>+    {
>+      var msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI);
>+      // if we've explicitly marked this message as not being an email scam, then don't
>+      // bother checking it with the phishing detector.
>+      if (msgHdr && msgHdr.getUint32Property("notAPhishMessage"))
>+        return; 
>+    }
>+
>+    this.updateMsgNotificationBar(kMsgNotificationPhishingBar, isMsgEmailScam(aUrl));
>+  },

Another too-early-for-early-return case: if the return is triggered,
updateMsgNotificationBar isn't called at all...

>+  barStatus: 0,

Make that mBarStatus.

>+  // private method used to set our message notification deck to the correct value...
>+  updateMsgNotificationBar: function(aIndex, aSet)

I'm quite uncomfortable with all those "1 << (something - 1)" stuff getting
computed all over again with every call to updateMsgNotificationBar. How about
adding another member to gMessageNotificationBar:

// flag bit values for mBarStatus, indexed by kMsgNotificationXXX
mBarFlagValues: [
		  0, // kMsgNotificationNoStatus,
		  1, // 1 << (kMsgNotificationJunkBar - 1)
		  2, // 1 << (kMsgNotificationRemoteImages - 1)
		  4  // 1 << (kMsgNotificationPhishingBar - 1)
		],

Don't put this stuff into the global scope, as noone outside of
gMessageNotificationBar should be seeing it.

>+    var status = 0;

You should set status to kMsgNotificationNoStatus to make it more visible
what's going on.

>+    if (aIndex != kMsgNotificationNoStatus) {

Keep the overall file style and move the { onto its own line.

>+      var chunk = 1 << (aIndex - 1);

With mBarFlagValues above, this could just be 
 var chunk = this.mBarFlagValues[aIndex];

>+    if (status & 1 << (kMsgNotificationJunkBar - 1))

and
 if (status & this.mBarFlagValues[kMsgNotificationJunkBar])
etc.

>+    if (status == 0)

if (!status)

>+function LoadMsgWithRemoteContent()
>+{
>+  // we want to get the msg hdr for the currently selected message
>+  // change the "remoteContentBar" property on it
>+  // then reload the message
>+
>+  var msgURI = GetLoadedMessage();
>+  var msgHdr = null;
>+    
>+  if (msgURI && !(/type=x-message-display/.test(msgURI)))
>+  {
>+    msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI);

Move the |var msgHdr| here (even though the JS parser doesn't care, it helps
reading the code later).

>+function MsgIsNotAScam()
>+{
>+  // we want to get the msg hdr for the currently selected message
>+  // change the "isPhishingMsg" property on it
>+  // then reload the message
>+
>+  var msgURI = GetLoadedMessage();
>+  var msgHdr = null;

Same here.

>Index: mailnews/base/resources/content/mailWindowOverlay.xul
>===================================================================
>+    <button label="&junkInfoButton.label;" oncommand="MsgJunkMailInfo(false)"/>
>+    <button label="&notJunkButton.label;" oncommand="JunkSelectedMessages(false);"/>

I suppose that keyboard users know that they can hit J/Shift-J (etc.) to do
this ...

>+    <button label="&loadRemoteContentButton.label;" oncommand="LoadMsgWithRemoteContent();"/>

... but how do they access this functionality?

>+    <button label="&removePhishingBarButton.label;" oncommand="MsgIsNotAScam();"/>

And this one?
Hide and seek via Tab key can't be the solution.

>Index: themes/classic/messenger/primaryToolbar.css
>Index: themes/modern/messenger/primaryToolbar.css
>===================================================================
>+  list-style-image: url("chrome://messenger/skin/icons/junkBar.gif");
[...]
>+  list-style-image: url("chrome://messenger/skin/icons/remote-blocked.png"); 

Then we need these icons, too...

>Index: xpfe/communicator/resources/content/contentAreaClick.js
>===================================================================
>+  function linkNodeForClickEvent(event)
[...]
>@@ -144,56 +145,49 @@
>       default:
>         linkNode = findParentNode(event.originalTarget, "a");
>         // <a> cannot be nested.  So if we find an anchor without an
>         // href, there is no useful <a> around the target
>         if (linkNode && !linkNode.hasAttribute("href"))
>           linkNode = null;
>         break;
>     }
>-    var href;
>-    if (linkNode) {
>-      href = new XPCNativeWrapper(linkNode, "href").href;
>-    } else {
>+
>+    if (!linkNode) {
>       // Try simple XLink
>       linkNode = target;
>-      while (linkNode) {
>-        if (linkNode.nodeType == Node.ELEMENT_NODE) {
>-          var wrapper = new XPCNativeWrapper(linkNode, "getAttributeNS()");
>-          href = wrapper.getAttributeNS("http://www.w3.org/1999/xlink", "href");
>-          break;
>-        }
>+      while (linkNode.nodeType != Node.ELEMENT_NODE) {
>         linkNode = linkNode.parentNode;
>       }
>-      if (href && href != "") {
>-        var baseURI = new XPCNativeWrapper(linkNode, "baseURI").baseURI;
>-        href = makeURLAbsolute(baseURI, href);
>-      }

While XPCWrappers should be on by default (as per attachment 184151 [details] [diff] [review] of bug
281988), I don't see why makeURLAbsolute should be removed. Can you explain
this? (The pre-phishing-detector Thunderbird version of this function is quite
old!)

>-  function makeURLAbsolute( base, url ) 

See above.


AFAICT, the following stuff is taken from phishingDetector.js. I don't think it
belongs into contentAreaClick.js, especially since it's focused on mail and
contentAreaClick.js is used by navigator.xul, too. It'd be best to have the our
own phishingDetector.js (and this would help us keeping that stuff synced).


>+    if (!aSilentMode && isPhishingURL) // allow the user to over ride the decision

Inferior nit: override

>+function hostNameIsIPAddress(aHostName, aUnobscuredHostName)
[...]
>+    // convert the dword into its component IP parts. 
>+    ipComponents = new Array;
>+    ipComponents[0] = (aHostName >> 24) & 255;
>+    ipComponents[1] = (aHostName >> 16) & 255;
>+    ipComponents[2] = (aHostName >>  8) & 255;
>+    ipComponents[3] = (aHostName & 255);

ipComponents =
[
  (aHostName >> 24) & 255,
  (aHostName >> 16) & 255,
  (aHostName >>  8) & 255,
  aHostName & 255
];

should be slightly more performant.

>+      //where one component is hex, another is octal, etc. 

Inferior nit: add a space behind //

>+  var hostName = ipComponents[0] + '.' +  ipComponents[1] + '.' + ipComponents[2] + '.' + ipComponents[3];

var hostName = ipComponents.join(".");

>+function isIPv4HostName(aHostName)
>+{
>+  var ipv4HostRegExp = new RegExp(/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/);  // IPv4
>+  // treat 0.0.0.0 as an invalid IP address
>+  if (ipv4HostRegExp.test(aHostName) && aHostName != '0.0.0.0')
>+    return true;
>+  else
>+    return false;

return ipv4HostRegExp.test(aHostName) && aHostName != '0.0.0.0';

>+function confirmSuspiciousURL(aPhishingType, aSuspiciousHostName)
>+{
>+  var brandShortName = gBrandBundle.getString("brandShortName");
>+  var titleMsg = gMessengerBundle.getString("confirmPhishingTitle");
>+  var dialogMsg;

var dialogMsg = null; 

is somewhat 'nicer' than calling ConfirmEx with an 'undefined' value.

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

In general, i don't like forking. I also don't mind if
nsMailnewsContentBlocker.cpp gets dropped, if there is a better replacement. I
wanted it to split it out from the browser content policy stuff, because the
mailnews rules are very simple: just block everything.

Comment 9

[Ian Neal]

Attachment: patch with phishingDetector.js v0.1d

Changes since v0.1c
* Re-wrote patch to use phishingDetector.js
* Added keys for Not Scam/Show Remote Content and also menuitems for them
* Fixed issue with emails with v-cards showing up as being scams
* Fixed issue with emails with v-cards causing an exception as some of their
linkNodes can have blank hrefs.

There may well be a better way of testing if we're using contentAreaClick.js
from mailnews than is currently in there...

Comment 10

[Karsten Düsterloh]

(In reply to comment #5)

I'd say that this is the most "unhackish" solution:

> 2) Drop extensions/permissions/nsMailnewsContentBlocker.cpp and use
> mailnews/base/src/nsMsgContentPolicy.cpp with the nsMsgCookiePolicy bits
> moved out into a separate file for TB to use.
[...]
> I'd say my least favourite option is the 3rd one, but it is possible TB folks
> will not let the first two options through.

Actually, I don't expect major resistance there: David, what's your say?

Comment 11

[David :Bienvenu]

Scott's really the expert on the content policy stuff - adding him to the cc list.

Comment 12

[Karsten Düsterloh]

Comment on attachment 186276 [details] [diff] [review]
patch with phishingDetector.js v0.1d

>Index: mailnews/base/resources/content/mail3PaneWindowCommands.js
>===================================================================
>+      case "cmd_markAsShowRemote":
>+      case "cmd_markAsNotPhish":
>         return (GetNumSelectedMessages() > 0 );

These items should only be enabled if the respective state was set by the
phishing detector.

>Index: mailnews/base/resources/content/mailWindow.js
>===================================================================
> function messagePaneOnClick(event)
> {
>   // if this is stand alone mail (no browser)
>   // or this isn't a simple left click, do nothing, and let the normal code execute
>   if (event.button != 0 || event.metaKey || event.ctrlKey || event.shiftKey || event.altKey)
>   {
>-    contentAreaClick(event);
>-    return;
>+    return contentAreaClick(event);
>   }
> 
>   // try to determine the href for what you are clicking on.  
>   // for example, it might be "" if you aren't left clicking on a link
>-  var href = hrefForClickEvent(event);
>-  if (!href) 
>-    return;
>+  var linkNode = linkNodeForClickEvent(event);
>+  if (!linkNode || !linkNode.href) 
>+    return true;
>+  var href = linkNode.href;

If we change from hrefForClickEvent to linkNodeForClickEvent, the
makeURLAbsolute call for href is lost - that's not good. We have at least two
occasions where this should still be done (here and in contentAreaClick), so
I'd say it would be useful to accompany linkNodeForClickEvent with this (from
hrefForClickEvent):

function getHrefOfLinkNode(aLinkNode)
{
  var href;
  if (aLinkNode)
    href = aLinkNode.href;
  else {
    // Try simple XLink
    while (aLinkNode) {
      if (aLinkNode.nodeType == Node.ELEMENT_NODE) {
	href = aLinkNode.getAttributeNS("http://www.w3.org/1999/xlink",
"href");
	break;
      }
      aLinkNode = aLinkNode.parentNode;
    }
    if (href)
      href = makeURLAbsolute(aLinkNode.baseURI, href);
  }
  return href;
}

So this

>+  var href = linkNode.href;

becomes

var href = getHrefOfLinkNode(linkNode);

>Index: mailnews/base/resources/content/mailWindowOverlay.js
>===================================================================
>+  // aUrl is the nsIURI for the message currently loaded in the message pane
>+  setPhishingMsg: function(aUrl)
>+  {
>+    var phishingMsg = true;
>+    var msgURI = GetLoadedMessage();
>+    if (msgURI && !(/type=x-message-display/.test(msgURI)))
>+    {
>+      var msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI);
>+      // if we've explicitly marked this message as not being an email scam, then don't
>+      // bother checking it with the phishing detector.
>+      if (msgHdr && msgHdr.getUint32Property("notAPhishMessage"))
>+        phishingMsg = false; 

phishingMsg = !msgHdr || !msgHdr.getUint32Property("notAPhishMessage");

(And move the var msgHdr below the comment.)

>+    }
>+
>+    phishingMsg = phishingMsg ? isMsgEmailScam(aUrl) : false;

Setting boolean variables to boolean constants in dependance of boolean
expressions is just ugly. ;-)

if (phishingMsg)
  phishingMsg = isMsgEmailScam(aUrl);

>Index: mailnews/base/resources/content/mailWindowOverlay.xul
>===================================================================
>+        <menuitem label="&markAsShowRemoteCmd.label;"
>+                  accesskey="&markAsShowRemoteCmd.accesskey;"
>+                  command="cmd_markAsShowRemote"/>
>+        <menuitem label="&markAsNotPhishCmd.label;"
>+                  accesskey="&markAsNotPhishCmd.accesskey;"
>+                  command="cmd_markAsNotPhish"/>
>         <menuitem label="&markAsJunkCmd.label;"
>                   accesskey="&markAsJunkCmd.accesskey;"
>                   command="cmd_markAsJunk"/>
>         <menuitem label="&markAsNotJunkCmd.label;"
>                   accesskey="&markAsNotJunkCmd.accesskey;"
>                   command="cmd_markAsNotJunk"/>
>         <menuitem label="&recalculateJunkScoreCmd.label;"
>                   accesskey="&recalculateJunkScoreCmd.accesskey;"

Maybe move these items after the junk stuff, as that is more likely to be used?
Feel free to argue. ;-)
(If moving here, then so at the next two occurences, too.)

>Index: mailnews/base/resources/content/messageWindow.js
>===================================================================
>+      case "cmd_markAsShowRemote":
>+      case "cmd_markAsNotPhish":
>         return (gCurrentMessageUri != null);

See mail3PaneWindowCommands.js above.

>Index: mailnews/base/resources/content/phishingDetector.js
>===================================================================
>+
>+  var hostName = ipComponents.join(".");
>+
>+  // only set aUnobscuredHostName if we are looking at an IPv4 host name
>+  if (isIPv4HostName(hostName))

Move the var line after the comment.

>Index: mailnews/base/resources/locale/en-US/messenger.dtd
>===================================================================
> <!-- junk bar -->
>+<!-- Remote Content Bar -->
>+<!-- Phshing bar Bar -->

I'd sell a bar and buy an i. ;-)
And use the same capitalization on all three lines.

>Index: xpfe/communicator/resources/content/contentAreaClick.js
>===================================================================
>-  function hrefForClickEvent(event)

getHrefOfLinkNode should reside here.

>+  function linkNodeForClickEvent(event)

>   function contentAreaClick(event) 
>   {
>     if (!event.isTrusted) {
>       return true;
>     }
> 
>     var isKeyPress = (event.type == "keypress");
>-    var href = hrefForClickEvent(event);
>-    if (href) {
>+    var linkNode = linkNodeForClickEvent(event);

Add 
      var href = getHrefOfLinkNode(linkNode);

>+    if (linkNode && linkNode.href) {

if (linkNode && href) {

>       if (isKeyPress) {
>-        openNewTabWith(href, true, event.shiftKey);
>+        openNewTabWith(linkNode.href, true, event.shiftKey);

No change needed here then.

>         event.preventBubble();
>       }
>       else {
>-        handleLinkClick(event, href, null);
>+        handleLinkClick(event, linkNode.href, null);

No change needed here then.

The remote content backend issue should go into another diff. :)

Comment 13

[Karsten Düsterloh]

Ah, two late changes:

> function getHrefOfLinkNode(aLinkNode)
> {
>   var href;

var href = "";

> >+  function linkNodeForClickEvent(event)
> 
> >   function contentAreaClick(event) 
> >   {
> >     if (!event.isTrusted) {
> >       return true;
> >     }
> > 
> >     var isKeyPress = (event.type == "keypress");
> >-    var href = hrefForClickEvent(event);
> >-    if (href) {
> >+    var linkNode = linkNodeForClickEvent(event);
> 
> Add 
>       var href = getHrefOfLinkNode(linkNode);
> 
> >+    if (linkNode && linkNode.href) {
> 
> if (linkNode && href) {

  if (href) 
does suffice here then.

Comment 14

[Ian Neal]

Attachment: patch with href and linknode patch v0.1e

Changes since v0.1e
* Moved phishing/remote content menu items to have junk mail menu items
* Phishing/remote content menu items checks msghdr properties for enablement
* Changed function in contentAreaClick.js to return both href and linkNode as
they are both needed
* Corrects some spelling mistakes
* Other review comments addressed

Comment 15

[Karsten Düsterloh]

Comment on attachment 187247 [details] [diff] [review]
patch with href and linknode patch v0.1e

>Index: mailnews/base/resources/content/mail3PaneWindowCommands.js
>===================================================================
>+      case "cmd_markAsShowRemote":
>+        return (GetNumSelectedMessages() > 0 && checkMsgHdrProperty("remoteContentPolicy", 2));
>+      case "cmd_markAsNotPhish":
>+        return (GetNumSelectedMessages() > 0 && checkMsgHdrProperty("notAPhishMessage", 1));

Please use constants for these values (1 and 2), the more shared the better.

>Index: mailnews/base/resources/content/mailWindowOverlay.js
>===================================================================
>+  setRemoteContentMsg: function(aMsgHdr)
>+  {  
>+    var blockRemote = aMsgHdr &&
>+                      aMsgHdr.getUint32Property("remoteContentPolicy") == kBlockRemoteContent;
>+    this.updateMsgNotificationBar(kMsgNotificationRemoteImages, blockRemote);
>+  },

Shouldn't this, too, call checkMsgHdrProperty?

>+  setPhishingMsg: function(aUrl)
>+  {
>+    // if we've explicitly marked this message as not being an email scam, then don't
>+    // bother checking it with the phishing detector.
>+    var phishingMsg = false;
>+    if (!checkMsgHdrProperty("notAPhishMessage", 0))

Again, a "talking" constant would be better.

>+function LoadMsgWithRemoteContent()
>+{
>+  // we want to get the msg hdr for the currently selected message
>+  // change the "remoteContentBar" property on it
>+  // then reload the message
>+
>+  var msgURI = GetLoadedMessage();
>+    
>+  if (msgURI && !(/type=x-message-display/.test(msgURI)))
>+  {
>+    var msgHdr = messenger.messageServiceFromURI(msgURI).messageURIToMsgHdr(msgURI);
>+    if (msgHdr)
>+    {
>+      msgHdr.setUint32Property("remoteContentPolicy", kAllowRemoteContent);

Now you've factored out the getUint32Property patterns into
checkMsgHdrProperty, it'd make sense to make that a set of
getMsgHdrProperty/setMsgHdrProperty and factor out the above pattern also?

>Index: mailnews/base/resources/content/messageWindow.js
>===================================================================
>+      case "cmd_markAsShowRemote":
>+        return (gCurrentMessageUri != null && checkMsgHdrProperty("remoteContentPolicy", 2));
>+      case "cmd_markAsNotPhish":
>+        return (gCurrentMessageUri != null && checkMsgHdrProperty("notAPhishMessage", 1));

See mail3PaneWindowCommands.js.

>Index: xpfe/communicator/resources/content/contentAreaClick.js
>===================================================================
>+      if (href && href != "")

|if (href)| does suffice, because Boolean("") == false.


So, nearly there, I'd say. :)
No structural differences left, only the above style and refactoring nits...

Comment 16

[Ian Neal]

Attachment: setMsgHdrPropertyAndReload patch v0.1f

Changes since v0.1e:
* Moved to using constants instead of values in mailWindowOverlay.js,
mail3PaneWindowCommands.js and messageWindow.js
* Factored out setting of property to create setMsgHdrPropertyAndReload
function
* Made requested change to contentAreaClick.js

setRemoteContentMsg already does equivalent check to that in
checkMsgHdrPropertyIsNot and is passed aMsgHdr so does not need to work it out
-> left function alone

Comment 17

[Karsten Düsterloh]

Comment on attachment 187982 [details] [diff] [review]
setMsgHdrPropertyAndReload patch v0.1f

>Index: mailnews/base/resources/content/mailWindowOverlay.js
>===================================================================
>+const kIsAPhishMessage = 0;
>+const kNotAPhishMessage = 1;

Move these down right before the kMsgNotification* constants to emphasize their
scope.


r=me with that. :)

Comment 18

[Ian Neal]

Attachment: moved constants patch v0.1g

Changes since v0.1f:
* Moved the Phish constants in mailWindowOverlay.js to just before the
kMsgNotification* ones

Carrying forward r= and requesting sr=

Comment 19

[Ian Neal]

Comment on attachment 188065 [details] [diff] [review]
moved constants patch v0.1g

Rqeuesting sr from David as he sr'd the TB equivalent of this bug

Comment 20

[Ian Neal]

Attachment: Unbitrotted patch v0.1h

Changes since v0.1g:
* Some unbitrotting

Carrying forward r= and requesting sr=

Comment 21

[Ian Neal]

Comment on attachment 189414 [details] [diff] [review]
Unbitrotted patch v0.1h

Requesting approval for reasonably low risk, suite only patch

Comment 22

[Ian Neal]

Attachment: Re-unbitrotted patch v0.1i (Checked in)

This is the patch that was actually checked in. Only difference was some of
changes in contentAreaClick.js had been checked in as part of Bug 295582

Comment 23

[Ian Neal]

Comment on attachment 191987 [details] [diff] [review]
Re-unbitrotted patch v0.1i (Checked in)

Checking in
mailnews/jar.mn;
new revision: 1.99; previous revision: 1.98
mailnews/mailnews.js;
new revision: 3.248; previous revision: 3.247
mailnews/base/resources/content/mail3PaneWindowCommands.js;
new revision: 1.145; previous revision: 1.144
mailnews/base/resources/content/mail3PaneWindowVertLayout.xul;
new revision: 1.106; previous revision: 1.105
mailnews/base/resources/content/mailWindow.js;
new revision: 1.100; previous revision: 1.99
mailnews/base/resources/content/mailWindowOverlay.js;
new revision: 1.221; previous revision: 1.220
mailnews/base/resources/content/mailWindowOverlay.xul;
new revision: 1.298; previous revision: 1.297
mailnews/base/resources/content/messageWindow.js;
new revision: 1.105; previous revision: 1.104
mailnews/base/resources/content/messageWindow.xul;
new revision: 1.81; previous revision: 1.80
mailnews/base/resources/content/messenger.xul;
new revision: 1.258; previous revision: 1.257
mailnews/base/resources/content/msgHdrViewOverlay.js;
new revision: 1.141; previous revision: 1.140
mailnews/base/resources/content/msgMail3PaneWindow.js;
new revision: 1.282; previous revision: 1.281
mailnews/base/resources/content/phishingDetector.js;
initial revision: 1.1
mailnews/base/resources/locale/en-US/messenger.dtd;
new revision: 1.202; previous revision: 1.201
mailnews/base/resources/locale/en-US/messenger.properties;
new revision: 1.122; previous revision: 1.121
themes/classic/jar.mn;
new revision: 1.136; previous revision: 1.135
themes/classic/messenger/primaryToolbar.css;
new revision: 1.12; previous revision: 1.11
themes/classic/messenger/icons/remote-blocked.png;
initial revision: 1.1
themes/modern/jar.mn;
new revision: 1.150; previous revision: 1.149
themes/modern/messenger/primaryToolbar.css;
new revision: 1.17; previous revision: 1.16
themes/modern/messenger/icons/remote-blocked.png;
initial revision: 1.1
xpfe/communicator/resources/content/contentAreaClick.js;
new revision: 1.50; previous revision: 1.49
done

Comment 24

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050809
SeaMonkey/1.0a] (nightly) (WXP)

(I have 2 Pop3 accounts in my profile, junk control activated)
Seeing this exception: (not sure yet at which step)
{{
Error: [Exception... "Component returned failure code: 0x80004002
(NS_NOINTERFACE) [nsIIOService.newURI]"  nsresult: "0x80004002 (NS_NOINTERFACE)"
 location: "JS frame :: chrome://messenger/content/phishingDetector.js ::
isPhishingURL :: line 107"  data: no]
Source File: chrome://messenger/content/phishingDetector.js
Line: 107
}}

Comment 25

[[e-mail]]

Comment on attachment 191987 [details] [diff] [review]
Re-unbitrotted patch v0.1i (Checked in)

>Index: themes/modern/messenger/primaryToolbar.css
...
>+/* ::::: message notification bar style rules ::::: */
> 
>-#junkBar {
>+.msgNotificationBar {
>   border-bottom: 1px solid;
>   -moz-border-bottom-colors: #000000;
>+  -moz-appearance: toolbox;
>   background-color: #C7BC8F;
>+  color: black;
> }
Modern should never use -moz-appearance: ...
also Modern style is to use #000000 instead of colour names.