Bug 574989 - Use priorities to deal with multipart/alternative messages with reversed plain text and HTML parts. r=jorgk
authorTerje Braten <bugzilla@braten.be>
Sat, 04 Jun 2016 04:52:00 +0200
changeset 19418 ab68d375e7e49cac789e133ee4e94d0fed7535eb
parent 19417 33a058a610941ba5497c80461047a524e5374f45
child 19419 d1ef2c74866235d4e6ec2a83faca5722bd484d5d
push id11951
push usermozilla@jorgk.com
push dateSat, 04 Jun 2016 14:47:55 +0000
treeherdercomm-central@6b49a9fa86f6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorgk
bugs574989
Bug 574989 - Use priorities to deal with multipart/alternative messages with reversed plain text and HTML parts. r=jorgk
mail/test/mozmill/message-window/test-alt-rel-text.eml
mail/test/mozmill/message-window/test-triple-alt.eml
mail/test/mozmill/message-window/test-view-plaintext.js
mailnews/mime/src/mimemalt.cpp
mailnews/mime/src/mimemalt.h
new file mode 100644
--- /dev/null
+++ b/mail/test/mozmill/message-window/test-alt-rel-text.eml
@@ -0,0 +1,50 @@
+From: test <test@test.com>
+Subject: test multipart, alternative first, with image
+To: test2 <test2@test.com>
+Date: Sat, 27 Feb 2016 17:11:45 +0100
+MIME-Version: 1.0
+Content-Type: multipart/alternative;
+ boundary="------------alternative"
+
+This is a multi-part message in MIME format.
+--------------alternative
+Content-Type: text/plain; charset=UTF-8; format=flowed
+Content-Transfer-Encoding: 8bit
+
+Plain Text
+
+--------------alternative
+Content-Type: multipart/related;
+ boundary="------------related"
+
+
+--------------related
+Content-Type: text/html; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+<html>
+  <head>
+    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
+  </head>
+  <body bgcolor="#FFFFFF" text="#000000">
+    HTML Body<br>
+    <img src="cid:part1" alt=""><br>
+  </body>
+</html>
+
+--------------related
+Content-Type: image/png
+Content-Transfer-Encoding: base64
+Content-ID: <part1>
+
+iVBORw0KGgoAAAANSUhEUgAAAAYAAAALCAIAAADTMGvBAAAAEUlEQVQImWPgsi5DQwxDWggA
+lCEwN+YGfiYAAAAASUVORK5CYII=
+--------------related--
+
+--------------alternative
+Content-Type: text; charset=UTF-8;
+Content-Transfer-Encoding: 8bit
+
+Unspecified text type (not text/plain)
+
+--------------alternative--
new file mode 100644
--- /dev/null
+++ b/mail/test/mozmill/message-window/test-triple-alt.eml
@@ -0,0 +1,36 @@
+From: "Test tester" <test_mail@example.com>
+To: "Robert Recipient" <robert@example.com>
+Subject: Alternative with 3 parts
+Date: Wed, 26 Sep 2001 09:16:49 -0400
+MIME-Version: 1.0
+Content-Type: multipart/alternative;
+	boundary="------------alternative"
+
+This is a multi-part message in MIME format.
+
+--------------alternative
+Content-Type: text/plain; charset="iso-8859-1"
+Content-Transfer-Encoding: 7bit
+
+Plain Text
+
+--------------alternative
+Content-Type: text/html; charset="iso-8859-1"
+Content-Transfer-Encoding: quoted-printable
+
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
+<HTML><HEAD></HEAD>
+<BODY>
+
+HTML Body<p>
+
+</BODY>
+</HTML>
+
+--------------alternative
+Content-Type: text; charset="iso-8859-1"
+Content-Transfer-Encoding: 8bit
+
+Unspecified text type (not text/plain)
+
+--------------alternative--
--- a/mail/test/mozmill/message-window/test-view-plaintext.js
+++ b/mail/test/mozmill/message-window/test-view-plaintext.js
@@ -87,19 +87,26 @@ function test_view() {
   checkSingleMessage("./test-alt-rel.eml",                 "Plain Text", "HTML Body");
   checkSingleMessage("./test-alt-rel-with-attach.eml",     "Plain Text", "HTML Body");
 
   // 4) HTML part missing
   // 5) Plain part missing
   checkSingleMessage("./test-alt-HTML-missing.eml",        "Plain Text", "Plain Text");
   checkSingleMessage("./test-alt-plain-missing.eml",       "HTML Body",  "HTML Body");
 
+  // 6) plain and HTML parts reversed in order
+  checkSingleMessage("./test-alt-plain-HTML-reversed.eml", "Plain Text", "HTML Body");
+
+  // 7) 3 alt. parts with 2 plain and 1 HTML part
+  checkSingleMessage("./test-triple-alt.eml",              "Plain Text", "HTML Body");
+
+  // 8) 3 alt. parts with 2 plain and 1 multipart/related
+  checkSingleMessage("./test-alt-rel-text.eml",            "Plain Text", "HTML Body");
+
   // Now some cases that don't work yet.
-  // 6) multipart/related with embedded multipart/alternative
-  // 7) plain and HTML parts reversed in order
+  // 9) multipart/related with embedded multipart/alternative
   checkSingleMessage("./test-rel-alt.eml",                 "HTML Body",  "HTML Body");
-  checkSingleMessage("./test-alt-plain-HTML-reversed.eml", "Plain Text", "Plain Text");
 }
 
 function teardownModule() {
   Services.prefs.clearUserPref("mailnews.display.prefer_plaintext");
   Services.prefs.clearUserPref("mailnews.display.html_as");
 }
--- a/mailnews/mime/src/mimemalt.cpp
+++ b/mailnews/mime/src/mimemalt.cpp
@@ -107,16 +107,19 @@ static int MimeMultipartAlternative_pars
 static int MimeMultipartAlternative_create_child(MimeObject *);
 static int MimeMultipartAlternative_parse_child_line (MimeObject *, const char *,
                             int32_t, bool);
 static int MimeMultipartAlternative_close_child(MimeObject *);
 
 static int MimeMultipartAlternative_flush_children(MimeObject *, bool, priority_t);
 static priority_t MimeMultipartAlternative_display_part_p(MimeObject *self,
                              MimeHeaders *sub_hdrs);
+static priority_t MimeMultipartAlternative_prioritize_part(char *content_type,
+                             bool prefer_plaintext);
+
 static int MimeMultipartAlternative_display_cached_part(MimeObject *,
                                                         MimeHeaders *,
                                                         MimePartBufferData *,
                                                         bool);
 
 static int
 MimeMultipartAlternativeClassInitialize(MimeMultipartAlternativeClass *clazz)
 {
@@ -358,63 +361,106 @@ MimeMultipartAlternative_close_child(Mim
   return 0;
 }
 
 
 static priority_t
 MimeMultipartAlternative_display_part_p(MimeObject *self,
                     MimeHeaders *sub_hdrs)
 {
+  priority_t priority = PRIORITY_UNDISPLAYABLE;
   char *ct = MimeHeaders_get (sub_hdrs, HEADER_CONTENT_TYPE, true, false);
   if (!ct)
-    return PRIORITY_UNDISPLAYABLE;
+    return priority;
 
   /* RFC 1521 says:
      Receiving user agents should pick and display the last format
      they are capable of displaying.  In the case where one of the
      alternatives is itself of type "multipart" and contains unrecognized
      sub-parts, the user agent may choose either to show that alternative,
      an earlier alternative, or both.
    */
-  priority_t priority = PRIORITY_UNDISPLAYABLE;
 
-  // prefer_plaintext pref
-  nsIPrefBranch *prefBranch = GetPrefBranch(self->options);
-  bool prefer_plaintext = false;
-  if (prefBranch)
-    prefBranch->GetBoolPref("mailnews.display.prefer_plaintext",
-                            &prefer_plaintext);
-  if (prefer_plaintext
-      && self->options->format_out != nsMimeOutput::nsMimeMessageSaveAs
-      && !PL_strncasecmp(ct, "text/plain", 10)
-     )
-    // if the user prefers plaintext and this is the plaintext part...
-  {
-    priority = PRIORITY_HIGHEST;
-  } else {
-    MimeObjectClass *clazz = mime_find_class (ct, sub_hdrs, self->options, true);
-    if (clazz && clazz->displayable_inline_p(clazz, sub_hdrs)) {
-      /* TODO:
-        If the user prefers plaintext and if this is a multipart that
-        contains a text/plain part (and it is not a converted/downgraded
-        text/html part) then the priority should be a bit less than
-        PRIORITY_HIGHEST, but above PRIORITY_NORMAL.
-        (A text/plain part inside a multipart is more likely to be
-        only a part of the message, but if there are multiple multipart
-        sub-parts then one that contains a true text/plain should be chosen
-        over a multipart that does not contain a true text/plain.)
-        But I do not know how to test for this.  - Terje BrĂ¥ten.
-      */
-      priority = PRIORITY_NORMAL;
+  MimeObjectClass *clazz = mime_find_class (ct, sub_hdrs, self->options, false);
+  if (clazz && clazz->displayable_inline_p(clazz, sub_hdrs)) {
+    // prefer_plaintext pref
+    bool prefer_plaintext = false;
+    nsIPrefBranch *prefBranch = GetPrefBranch(self->options);
+    if (prefBranch) {
+      prefBranch->GetBoolPref("mailnews.display.prefer_plaintext",
+                              &prefer_plaintext);
     }
+    prefer_plaintext = prefer_plaintext
+           && (self->options->format_out != nsMimeOutput::nsMimeMessageSaveAs);
+
+    priority = MimeMultipartAlternative_prioritize_part(ct, prefer_plaintext);
   }
+
   PR_FREEIF(ct);
   return priority;
 }
 
+/**
+* RFC 1521 says we should display the last format we are capable of displaying.
+* But for various reasons (mainly to improve the user experience) we choose
+* to ignore that in some cases, and rather pick one that we prioritize.
+*/
+static priority_t
+MimeMultipartAlternative_prioritize_part(char *content_type,
+                                         bool prefer_plaintext)
+{
+  /*
+   * PRIORITY_NORMAL is the priority of text/html, multipart/..., etc. that
+   * we normally display. We should try to have as few exceptions from
+   * PRIORITY_NORMAL as possible
+   */
+
+  /* (with no / in the type) */
+  if (!PL_strcasecmp(content_type, "text")) {
+    if (prefer_plaintext) {
+       /* When in plain text view, a plain text part is what we want. */
+       return PRIORITY_HIGH;
+    }
+    /* We normally prefer other parts over the unspecified text type. */
+    return  PRIORITY_TEXT_UNKNOWN;
+  }
+
+  if (!PL_strncasecmp(content_type, "text/", 5)) {
+    char *text_type = content_type + 5;
+
+    if (!PL_strncasecmp(text_type, "plain", 5)) {
+      if (prefer_plaintext) {
+        /* When in plain text view,
+           the text/plain part is exactly what we want */
+        return PRIORITY_HIGHEST;
+      }
+      /*
+       * Because the html and the text part may be switched,
+       * or we have an extra text/plain added by f.ex. a buggy virus checker,
+       * we prioritize text/plain lower than normal.
+       */
+      return PRIORITY_TEXT_PLAIN;
+    }
+
+    /* Need to white-list all text/... types that are or could be implemented. */
+    if (!PL_strncasecmp(text_type, "html", 4) ||
+        !PL_strncasecmp(text_type, "enriched", 8) ||
+        !PL_strncasecmp(text_type, "richtext", 8) ||
+        !PL_strncasecmp(text_type, "calendar", 8) ||
+        !PL_strncasecmp(text_type, "rtf", 3)) {
+      return PRIORITY_NORMAL;
+    }
+
+    /* We prefer other parts over unknown text types. */
+    return PRIORITY_TEXT_UNKNOWN;
+  }
+
+  return PRIORITY_NORMAL;
+}
+
 static int
 MimeMultipartAlternative_display_cached_part(MimeObject *obj,
                                              MimeHeaders *hdrs,
                                              MimePartBufferData *buffer,
                                              bool do_display)
 {
   int status;
   bool old_options_no_output_p;
--- a/mailnews/mime/src/mimemalt.h
+++ b/mailnews/mime/src/mimemalt.h
@@ -19,17 +19,20 @@ typedef struct MimeMultipartAlternative 
 
 struct MimeMultipartAlternativeClass {
   MimeMultipartClass multipart;
 };
 
 extern "C" MimeMultipartAlternativeClass mimeMultipartAlternativeClass;
 
 enum priority_t {PRIORITY_UNDISPLAYABLE,
+                 PRIORITY_TEXT_UNKNOWN,
+                 PRIORITY_TEXT_PLAIN,
                  PRIORITY_NORMAL,
+                 PRIORITY_HIGH,
                  PRIORITY_HIGHEST};
 
 struct MimeMultipartAlternative {
   MimeMultipart multipart;      /* superclass variables */
 
   MimeHeaders **buffered_hdrs;    /* The headers of pending parts */
   MimePartBufferData **part_buffers;  /* The data of pending parts
                                          (see mimepbuf.h) */