Bug 742538 - Removing Filelink URLs from a message body should not cause subsequent URL insertions to fail. r=bienvenu.
authorMike Conley <mconley@mozilla.com>
Mon, 16 Apr 2012 12:55:37 -0400
changeset 9912 974245c3a39b63bf6fd7c9fdd6ea376e612ff44c
parent 9911 de274c897ab776d37d2a7ea0e57fb84e12963621
child 9913 c377ad53f15c481a5d8135d55c47d2c39e9e3713
push id7546
push usermconley@mozilla.com
push dateMon, 16 Apr 2012 16:56:24 +0000
treeherdercomm-central@974245c3a39b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbienvenu
bugs742538
Bug 742538 - Removing Filelink URLs from a message body should not cause subsequent URL insertions to fail. r=bienvenu.
mail/components/compose/content/cloudAttachmentLinkManager.js
mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
--- a/mail/components/compose/content/cloudAttachmentLinkManager.js
+++ b/mail/components/compose/content/cloudAttachmentLinkManager.js
@@ -73,25 +73,30 @@ var gCloudAttachmentLinkManager = {
 
       let attachment = event.target.attachment;
       let provider = event.target.cloudProvider;
       this.cloudAttachments.push(attachment);
       this._insertItem(mailDoc, attachment, provider);
     }
     else if (event.type == "attachments-removed" ||
              event.type == "attachments-converted") {
-      let list = mailDoc.getElementById("cloudAttachmentList");
-      let items = list.getElementsByClassName("cloudAttachmentItem");
+
+      let list, items;
+      list = mailDoc.getElementById("cloudAttachmentList");
+
+      if (list)
+        items = list.getElementsByClassName("cloudAttachmentItem");
 
       for (let attachment in fixIterator(
            event.detail, Components.interfaces.nsIMsgAttachment)) {
         // Remove the attachment from the message body.
-        for (let i = 0; i < items.length; i++)
-          if (items[i].contentLocation == attachment.contentLocation)
-            list.removeChild(items[i]);
+        if (list && items)
+          for (let i = 0; i < items.length; i++)
+            if (items[i].contentLocation == attachment.contentLocation)
+              list.removeChild(items[i]);
 
         // Now, remove the attachment from our internal list.
         let index = this.cloudAttachments.indexOf(attachment);
         if (index != -1)
           this.cloudAttachments.splice(index, 1);
       }
 
       this._updateAttachmentCount(mailDoc);
@@ -246,22 +251,44 @@ var gCloudAttachmentLinkManager = {
     if (nodeIndex <= 0) {
       editor.insertLineBreak();
       nodeIndex = 1;
     }
     selection.collapse(mailBody, nodeIndex);
   },
 
   /**
+   * Attempts to find any elements with an id in aIDs, and sets those elements
+   * id attribute to the empty string, freeing up the ids for later use.
+   *
+   * @param aDocument the document to search for the elements.
+   * @param aIDs an array of id strings.
+   */
+  _resetNodeIDs: function(aDocument, aIDs) {
+    for each (let [, id] in Iterator(aIDs)) {
+      let node = aDocument.getElementById(id);
+      if (node)
+        node.id = "";
+    }
+  },
+
+  /**
    * Insert the header for the cloud attachment list, which we'll use to
    * as an insertion point for the individual cloud attachments.
    *
-   * @param aDocument the document to insert the header into
+   * @param aDocument the document to insert the header into.
    */
   _insertHeader: function(aDocument) {
+    // If there already exists a cloudAttachmentListRoot,
+    // cloudAttachmentListHeader or cloudAttachmentList in the document,
+    // strip them of their IDs so that we don't conflict with them.
+    this._resetNodeIDs(aDocument, ["cloudAttachmentListRoot",
+                                    "cloudAttachmentListHeader",
+                                    "cloudAttachmentList"]);
+
     let brandBundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
     let editor = GetCurrentEditor();
     let selection = editor.selection;
 
     // Save off the selection ranges so we can restore them later.
     let ranges = [];
     for (let i = 0; i < selection.rangeCount; i++)
       ranges.push(selection.getRangeAt(i));
@@ -352,16 +379,22 @@ var gCloudAttachmentLinkManager = {
    * Insert the information for a cloud attachment.
    *
    * @param aDocument the document to insert the item into
    * @param aAttachment the nsIMsgAttachment to insert
    * @param aProviderType the cloud storage provider
    */
   _insertItem: function(aDocument, aAttachment, aProvider) {
     let list = aDocument.getElementById("cloudAttachmentList");
+
+    if (!list) {
+      this._insertHeader(aDocument);
+      list = aDocument.getElementById("cloudAttachmentList");
+    }
+
     let node = aDocument.createElement("div");
     node.className = "cloudAttachmentItem";
     node.contentLocation = aAttachment.contentLocation;
 
     if (gMsgCompose.composeHTML) {
       node.style.border = "1px solid #CDCDCD";
       node.style.borderRadius = "5px";
       node.style.marginTop = "10px";
--- a/mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
+++ b/mail/test/mozmill/cloudfile/test-cloudfile-attachment-urls.js
@@ -203,22 +203,31 @@ function try_with_and_without_signature_
  * in HTML mode, and again in plaintext mode.
  *
  * @param aTest a test that takes no arguments.
  */
 function try_without_signature(aTest) {
   let oldSig = Services.prefs.getCharPref(kSigPrefKey);
   Services.prefs.setCharPref(kSigPrefKey, "");
 
+  try_with_plaintext_and_html_mail(aTest);
+  Services.prefs.setCharPref(kSigPrefKey, oldSig);
+}
+
+/**
+ * Helper function that runs a test function for HTML mail composition, and
+ * then again in plaintext mail composition.
+ *
+ * @param aTest a test that takes no arguments.
+ */
+function try_with_plaintext_and_html_mail(aTest) {
   aTest();
   Services.prefs.setBoolPref(kHtmlPrefKey, false);
   aTest();
   Services.prefs.setBoolPref(kHtmlPrefKey, true);
-
-  Services.prefs.setCharPref(kSigPrefKey, oldSig);
 }
 
 /**
  * Test that if we open up a composer and immediately attach a Filelink,
  * a linebreak is inserted before the containment node in order to allow
  * the user to write before the attachment URLs.  This assumes the user
  * does not have a signature already inserted into the message body.
  */
@@ -315,20 +324,17 @@ function test_inserts_linebreak_on_empty
 
   Services.prefs.setBoolPref(kHtmlPrefKey, true);
 }
 
 /**
  * Tests that removing all Filelinks causes the root node to be removed.
  */
 function test_removing_filelinks_removes_root_node() {
-  subtest_removing_filelinks_removes_root_node();
-  Services.prefs.setBoolPref(kHtmlPrefKey, false);
-  subtest_removing_filelinks_removes_root_node();
-  Services.prefs.setBoolPref(kHtmlPrefKey, true);
+  try_with_plaintext_and_html_mail(subtest_removing_filelinks_removes_root_node);
 }
 
 /**
  * Test for test_removing_filelinks_removes_root_node - can be executed
  * on both plaintext and HTML compose windows.
  */
 function subtest_removing_filelinks_removes_root_node() {
   let cw = prepare_some_attachments_and_reply([], kFiles);
@@ -664,20 +670,17 @@ function subtest_adding_filelinks_to_for
 }
 
 /**
  * Test that if we convert a Filelink from one provider to another, that the
  * old Filelink is removed, and a new Filelink is added for the new provider.
  * We test this on both HTML and plaintext mail.
  */
 function test_converting_filelink_updates_urls() {
-  subtest_converting_filelink_updates_urls();
-  Services.prefs.setBoolPref(kHtmlPrefKey, false);
-  subtest_converting_filelink_updates_urls();
-  Services.prefs.setBoolPref(kHtmlPrefKey, true);
+  try_with_plaintext_and_html_mail(subtest_converting_filelink_updates_urls);
 }
 
 /**
  * Subtest for test_converting_filelink_updates_urls that creates two
  * storage provider accounts, uploads files to one, converts them to the
  * other, and ensures that the attachment links in the message body get
  * get updated.
  */
@@ -707,20 +710,18 @@ function subtest_converting_filelink_upd
   }
 }
 
 /**
  * Test that if we convert a Filelink to a normal attachment that the
  * Filelink is removed from the message body.
  */
 function test_converting_filelink_to_normal_removes_url() {
-  subtest_converting_filelink_to_normal_removes_url();
-  Services.prefs.setBoolPref(kHtmlPrefKey, false);
-  subtest_converting_filelink_to_normal_removes_url();
-  Services.prefs.setBoolPref(kHtmlPrefKey, true);
+  try_with_plaintext_and_html_mail(
+    subtest_converting_filelink_to_normal_removes_url);
 }
 
 /**
  * Subtest for test_converting_filelink_to_normal_removes_url that adds
  * some Filelinks to an email, and then converts those Filelinks back into
  * normal attachments, checking to ensure that the links are removed from
  * the body of the email.
  */
@@ -743,8 +744,42 @@ function subtest_converting_filelink_to_
   }
 
   // At this point, the root should also have been removed.
   let mailBody = get_compose_body(cw);
   root = mailBody.querySelector("#cloudAttachmentListRoot");
   if (root)
     throw new Error("Should not have found the cloudAttachmentListRoot");
 }
+
+/**
+ * Tests that if the user manually removes the Filelinks from the message body
+ * that it doesn't break future Filelink insertions.
+ */
+function test_filelinks_work_after_manual_removal() {
+  try_with_plaintext_and_html_mail(subtest_filelinks_work_after_manual_removal);
+}
+
+/**
+ * Subtest that first adds some Filelinks to the message body, removes them,
+ * and then adds another Filelink ensuring that the new URL is successfully
+ * inserted.
+ */
+function subtest_filelinks_work_after_manual_removal() {
+  // Insert some Filelinks...
+  gMockFilePicker.returnFiles = collectFiles(kFiles, __file__);
+  let provider = new MockCloudfileAccount();
+  provider.init("someKey");
+  let cw = open_compose_new_mail();
+  cw.window.attachToCloud(provider);
+
+  let [root, list, urls] = wait_for_attachment_urls(cw, kFiles.length);
+
+  // Now remove the root node from the document body
+  let mailBody = get_compose_body(cw);
+  mailBody.removeChild(root);
+
+  gMockFilePicker.returnFiles = collectFiles(["./data/testFile3"], __file__);
+  cw.window.attachToCloud(provider);
+  [root, list, urls] = wait_for_attachment_urls(cw, 1);
+}
+
+