Bug 1899335 - Fix inconsistency regarding the usage of properties with name "id" in Manifest V3. r=Standard8,mkmelin

We should only use `id` inside a primary type definition. For example
the `MailFolder` type defines an object representing a folder, and it
has an id to reference that `MailFolder`. If a `MailFolder` is used
elsewhere, it should be a named id, for example `folderId`.

We correctly do this in about 99%. However we have these exceptions:

* mailTabs.MailTab: This is not a primary type, it uses tab IDs from
  the `tabs` API
* compose.ComposeRecipient: It uses node IDs from the `contacts` API
  or the `mailinglist` API
* spaces.querry: It should use `spaceId`, compare with the `folders` API

This patch fixes that for Manifest V3. We have this issue also in the
documentation of parameters, but that has no influence on the code of
extensions and can be changed later.

Differential Revision: https://phabricator.services.mozilla.com/D211914

--HG--
rename : mail/components/extensions/test/browser/browser_ext_compose_begin_headers.js => mail/components/extensions/test/browser/browser_ext_compose_begin_headers_mv3.js
extra : moz-landing-system : lando

mail/components/extensions/parent/ext-compose.js

@@ -72,15 +72,18 @@ async function parseComposeRecipientList(
     if (!("addressBookCache" in this)) {
       await extensions.asyncLoadModule("addressBook");
     }
+
+    // Manifest V2 uses `id`, Manifest V3 uses `nodeId`.
+    const nodeId = recipient.id || recipient.nodeId;
     if (recipient.type == "contact") {
-      const contactNode = this.addressBookCache.findContactById(recipient.id);
+      const contactNode = this.addressBookCache.findContactById(nodeId);
 
       if (
         requireSingleValidEmail &&
         !isValidAddress(contactNode.item.primaryEmail)
       ) {
         throw new ExtensionError(
-          `Contact does not have a valid email address: ${recipient.id}`
+          `Contact does not have a valid email address: ${nodeId}`
         );
       }
       recipients.push(
@@ -94,9 +97,7 @@ async function parseComposeRecipientList(
         throw new ExtensionError("Mailing list not allowed.");
       }
 
-      const mailingListNode = this.addressBookCache.findMailingListById(
-        recipient.id
-      );
+      const mailingListNode = this.addressBookCache.findMailingListById(nodeId);
       recipients.push(
         MailServices.headerParser.makeMimeAddress(
           mailingListNode.item.dirName,

mail/components/extensions/parent/ext-mailTabs.js

@@ -60,7 +60,6 @@ function convertMailTab(tab, context) {
   const fixApiModeName = name => (name == "smart" ? "unified" : name);
 
   const mailTabObject = {
-    id: tab.id,
     windowId: tab.windowId,
     active: tab.active,
     layout: LAYOUTS[gDynamicPaneConfig],
@@ -68,6 +67,12 @@ function convertMailTab(tab, context) {
     folderModesEnabled: about3Pane.folderPane.activeModes.map(fixApiModeName),
   };
 
+  if (context.extension.manifest.manifest_version < 3) {
+    mailTabObject.id = tab.id;
+  } else {
+    mailTabObject.tabId = tab.id;
+  }
+
   mailTabObject.folderPaneVisible = paneLayout.folderPaneVisible;
   mailTabObject.messagePaneVisible = paneLayout.messagePaneVisible;
   const sortType = SORT_TYPE_MAP.get(gViewWrapper?.primarySortType);

mail/components/extensions/parent/ext-spaces.js

@@ -148,9 +148,14 @@ this.spaces = class extends ExtensionAPI {
    * @returns {boolean}
    */
   matchSpace(space, queryInfo) {
+    // Manifest V2.
     if (queryInfo.id != null && space.id != queryInfo.id) {
       return false;
     }
+    // Manifest V3.
+    if (queryInfo.spaceId != null && space.id != queryInfo.spaceId) {
+      return false;
+    }
     if (queryInfo.name != null && space.name != queryInfo.name) {
       return false;
     }

mail/components/extensions/schemas/compose.json

@@ -28,7 +28,13 @@
             "properties": {
               "id": {
                 "type": "string",
-                "description": "The ID of a contact or mailing list from the :doc:`contacts` and :doc:`mailingLists`."
+                "max_manifest_version": 2,
+                "description": "The ID of a contact or mailing list from the :doc:`contacts` or :doc:`mailingLists`."
+              },
+              "nodeId": {
+                "type": "string",
+                "min_manifest_version": 3,
+                "description": "The ID of a contact or mailing list node from the :doc:`addressBook.contacts` or :doc:`addressBook.mailingLists`."
               },
               "type": {
                 "type": "string",

mail/components/extensions/schemas/mailTabs.json

@@ -13,6 +13,11 @@
         "type": "object",
         "properties": {
           "id": {
+            "max_manifest_version": 2,
+            "type": "integer"
+          },
+          "tabId": {
+            "min_manifest_version": 3,
             "type": "integer"
           },
           "windowId": {
@@ -282,7 +287,7 @@
               {
                 "type": "array",
                 "items": {
-                  "$ref": "MailTab"
+                  "$ref": "mailTabs.MailTab"
                 }
               }
             ]
@@ -292,13 +297,13 @@
       {
         "name": "get",
         "type": "function",
-        "description": "Get the properties of a mail tab.",
+        "description": "Get the :ref:`mailTabs.MailTab` properties of a mail tab.",
         "async": "callback",
         "parameters": [
           {
             "name": "tabId",
             "type": "integer",
-            "description": "ID of the requested mail tab. Throws if the requested tab is not a mail tab."
+            "description": "ID of the requested mail tab. Throws if the requested <value>tabId</value> does not belong to a mail tab."
           },
           {
             "type": "function",
@@ -306,7 +311,7 @@
             "optional": true,
             "parameters": [
               {
-                "$ref": "MailTab"
+                "$ref": "mailTabs.MailTab"
               }
             ]
           }
@@ -316,7 +321,7 @@
         "name": "getCurrent",
         "type": "function",
         "max_manifest_version": 2,
-        "description": "Get the properties of the active mail tab, if the active tab is a mail tab. Returns <value>undefined</value> otherwise.",
+        "description": "Get the :ref:`mailTabs.MailTab` properties of the active mail tab. Returns <value>undefined</value>, if the active tab is not a mail tab.",
         "async": "callback",
         "parameters": [
           {
@@ -325,7 +330,7 @@
             "optional": true,
             "parameters": [
               {
-                "$ref": "MailTab",
+                "$ref": "mailTabs.MailTab",
                 "optional": true,
                 "description": "This may return undefined"
               }
@@ -341,7 +346,7 @@
         "parameters": [
           {
             "name": "createProperties",
-            "$ref": "MailTabProperties",
+            "$ref": "mailTabs.MailTabProperties",
             "optional": true,
             "default": {}
           },
@@ -352,7 +357,7 @@
             "parameters": [
               {
                 "name": "mailTab",
-                "$ref": "MailTab",
+                "$ref": "mailTabs.MailTab",
                 "description": "Details about the created mail tab. Will contain the ID of the new tab."
               }
             ]
@@ -374,7 +379,7 @@
           },
           {
             "name": "updateProperties",
-            "$ref": "MailTabProperties"
+            "$ref": "mailTabs.MailTabProperties"
           },
           {
             "type": "function",
@@ -383,7 +388,7 @@
             "parameters": [
               {
                 "name": "mailTab",
-                "$ref": "MailTab",
+                "$ref": "mailTabs.MailTab",
                 "description": "Details about the updated mail tab."
               }
             ]
@@ -521,7 +526,7 @@
                 "optional": true
               },
               "text": {
-                "$ref": "QuickFilterTextDetail",
+                "$ref": "mailTabs.QuickFilterTextDetail",
                 "description": "Shows only messages matching the supplied text.",
                 "optional": true
               }

mail/components/extensions/schemas/spaces.json

@@ -12,7 +12,7 @@
                 "type": "string"
               },
               {
-                "$ref": "ColorArray"
+                "$ref": "spaces.ColorArray"
               }
             ],
             "optional": true,
@@ -60,7 +60,7 @@
                 "type": "string"
               },
               {
-                "$ref": "ColorArray"
+                "$ref": "spaces.ColorArray"
               }
             ],
             "optional": "omit-key-if-missing",
@@ -219,6 +219,14 @@
             "properties": {
               "id": {
                 "type": "integer",
+                "max_manifest_version": 2,
+                "description": "The id of the space.",
+                "optional": true,
+                "minimum": 1
+              },
+              "spaceId": {
+                "type": "integer",
+                "min_manifest_version": 3,
                 "description": "The id of the space.",
                 "optional": true,
                 "minimum": 1

mail/components/extensions/test/browser/browser.ini

@@ -43,6 +43,7 @@ support-files = data/cloudFile1.txt data/cloudFile2.txt
 [browser_ext_compose_begin_body.js]
 [browser_ext_compose_begin_bug1691254.js]
 [browser_ext_compose_begin_forward.js]
+[browser_ext_compose_begin_headers_mv3.js]
 [browser_ext_compose_begin_headers.js]
 [browser_ext_compose_begin_identity.js]
 [browser_ext_compose_begin_new.js]

mail/components/extensions/test/browser/browser_ext_compose_begin_headers.js

@@ -162,6 +162,7 @@ add_task(async function testHeaders() {
   const extension = ExtensionTestUtils.loadExtension({
     files,
     manifest: {
+      manifest_version: 2,
       background: { scripts: ["utils.js", "background.js"] },
       permissions: ["accountsRead", "addressBooks", "messagesRead"],
     },

mail/components/extensions/test/browser/browser_ext_compose_begin_headers_mv3.js

@@ -0,0 +1,190 @@
+/* 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/. */
+
+var { MailServices } = ChromeUtils.importESModule(
+  "resource:///modules/MailServices.sys.mjs"
+);
+
+const account = createAccount("pop3");
+createAccount("local");
+MailServices.accounts.defaultAccount = account;
+
+addIdentity(account);
+
+const rootFolder = account.incomingServer.rootFolder;
+rootFolder.createSubfolder("test", null);
+const folder = rootFolder.getChildNamed("test");
+createMessages(folder, 4);
+
+add_task(async function testHeaders() {
+  const files = {
+    "background.js": async () => {
+      async function checkHeaders(expected) {
+        const [createdWindow] = await createdWindowPromise;
+        browser.test.assertEq("messageCompose", createdWindow.type);
+        browser.test.sendMessage("checkHeaders", expected);
+        await window.waitForMessage();
+        const removedWindowPromise = window.waitForEvent("windows.onRemoved");
+        browser.windows.remove(createdWindow.id);
+        await removedWindowPromise;
+      }
+
+      const accounts = await browser.accounts.list(true);
+      browser.test.assertEq(2, accounts.length, "number of accounts");
+      const popAccount = accounts.find(a => a.type == "pop3");
+      const folder = popAccount.rootFolder.subFolders.find(
+        f => f.name == "test"
+      );
+      const { messages } = await browser.messages.list(folder.id);
+      browser.test.assertEq(4, messages.length, "number of messages");
+
+      const addressBook = await browser.addressBooks.create({
+        name: "Baker Street",
+      });
+      const contacts = {
+        sherlock: await browser.addressBooks.contacts.create(
+          addressBook,
+          `BEGIN:VCARD\r\nVERSION:4.0\r\nFN:Sherlock Holmes\r\nEMAIL;PREF=1:sherlock@bakerstreet.invalid\r\nEND:VCARD\r\n`
+        ),
+        john: await browser.addressBooks.contacts.create(
+          addressBook,
+          `BEGIN:VCARD\r\nVERSION:4.0\r\nFN:John Watson\r\nEMAIL;PREF=1:john@bakerstreet.invalid\r\nEND:VCARD\r\n`
+        ),
+      };
+      const listNodeId = await browser.addressBooks.mailingLists.create(
+        addressBook,
+        {
+          name: "Holmes and Watson",
+          description: "Tenants221B",
+        }
+      );
+      await browser.addressBooks.mailingLists.addMember(
+        listNodeId,
+        contacts.sherlock
+      );
+      await browser.addressBooks.mailingLists.addMember(
+        listNodeId,
+        contacts.john
+      );
+
+      let createdWindowPromise;
+
+      // Start a new message.
+
+      createdWindowPromise = window.waitForEvent("windows.onCreated");
+      await browser.compose.beginNew();
+      await checkHeaders({});
+
+      // Start a new message, with a subject and recipients as strings.
+
+      createdWindowPromise = window.waitForEvent("windows.onCreated");
+      await browser.compose.beginNew({
+        to: "Sherlock Holmes <sherlock@bakerstreet.invalid>",
+        cc: "John Watson <john@bakerstreet.invalid>",
+        subject: "Did you miss me?",
+      });
+      await checkHeaders({
+        to: ["Sherlock Holmes <sherlock@bakerstreet.invalid>"],
+        cc: ["John Watson <john@bakerstreet.invalid>"],
+        subject: "Did you miss me?",
+      });
+
+      // Start a new message, with a subject and recipients as string arrays.
+
+      createdWindowPromise = window.waitForEvent("windows.onCreated");
+      await browser.compose.beginNew({
+        to: ["Sherlock Holmes <sherlock@bakerstreet.invalid>"],
+        cc: ["John Watson <john@bakerstreet.invalid>"],
+        subject: "Did you miss me?",
+      });
+      await checkHeaders({
+        to: ["Sherlock Holmes <sherlock@bakerstreet.invalid>"],
+        cc: ["John Watson <john@bakerstreet.invalid>"],
+        subject: "Did you miss me?",
+      });
+
+      // Start a new message, with a subject and recipients as contacts.
+
+      createdWindowPromise = window.waitForEvent("windows.onCreated");
+      await browser.compose.beginNew({
+        to: [{ nodeId: contacts.sherlock, type: "contact" }],
+        cc: [{ nodeId: contacts.john, type: "contact" }],
+        subject: "Did you miss me?",
+      });
+      await checkHeaders({
+        to: ["Sherlock Holmes <sherlock@bakerstreet.invalid>"],
+        cc: ["John Watson <john@bakerstreet.invalid>"],
+        subject: "Did you miss me?",
+      });
+
+      // Start a new message, with a subject and recipients as a mailing list.
+
+      createdWindowPromise = window.waitForEvent("windows.onCreated");
+      await browser.compose.beginNew({
+        to: [{ nodeId: listNodeId, type: "mailingList" }],
+        subject: "Did you miss me?",
+      });
+      await checkHeaders({
+        to: ["Holmes and Watson <Tenants221B>"],
+        subject: "Did you miss me?",
+      });
+
+      // Reply to a message.
+
+      createdWindowPromise = window.waitForEvent("windows.onCreated");
+      await browser.compose.beginReply(messages[0].id);
+      await checkHeaders({
+        to: [messages[0].author.replace(/"/g, "")],
+        subject: `Re: ${messages[0].subject}`,
+      });
+
+      // Forward a message.
+
+      createdWindowPromise = window.waitForEvent("windows.onCreated");
+      await browser.compose.beginForward(
+        messages[1].id,
+        "forwardAsAttachment",
+        {
+          to: ["Mycroft Holmes <mycroft@bakerstreet.invalid>"],
+        }
+      );
+      await checkHeaders({
+        to: ["Mycroft Holmes <mycroft@bakerstreet.invalid>"],
+        subject: `Fwd: ${messages[1].subject}`,
+      });
+
+      // Forward a message inline. This uses a different code path.
+
+      createdWindowPromise = window.waitForEvent("windows.onCreated");
+      await browser.compose.beginForward(messages[2].id, "forwardInline", {
+        to: ["Mycroft Holmes <mycroft@bakerstreet.invalid>"],
+      });
+      await checkHeaders({
+        to: ["Mycroft Holmes <mycroft@bakerstreet.invalid>"],
+        subject: `Fwd: ${messages[2].subject}`,
+      });
+
+      await browser.addressBooks.delete(addressBook);
+      browser.test.notifyPass("finished");
+    },
+    "utils.js": await getUtilsJS(),
+  };
+  const extension = ExtensionTestUtils.loadExtension({
+    files,
+    manifest: {
+      manifest_version: 3,
+      background: { scripts: ["utils.js", "background.js"] },
+      permissions: ["accountsRead", "addressBooks", "messagesRead"],
+    },
+  });
+
+  extension.onMessage("checkHeaders", async expected => {
+    await checkComposeHeaders(expected);
+    extension.sendMessage();
+  });
+
+  await extension.startup();
+  await extension.awaitFinish("finished");
+  await extension.unload();
+});

mail/components/extensions/test/browser/browser_ext_mailTabs_create.js

@@ -219,7 +219,7 @@ add_task(async function test_create_mv3() {
       const mailTab = await browser.mailTabs.create(test);
       window.assertDeepEqual(expected, mailTab);
       await window.sendMessage("checkDisplayedFolder", expected);
-      await browser.tabs.remove(mailTab.id);
+      await browser.tabs.remove(mailTab.tabId);
     }
 
     const [accountId] = await window.waitForMessage();

mail/components/extensions/test/browser/browser_ext_mailTabs_folderModes.js

@@ -34,7 +34,7 @@ add_task(async () => {
     const tagFolderLabel1 = await browser.folders.getTagFolder("$label1");
     const unifiedInbox = await browser.folders.getUnifiedFolder("inbox");
 
-    const [{ id: mailTabId }] = await browser.mailTabs.query({
+    const [{ tabId: mailTabId }] = await browser.mailTabs.query({
       active: true,
       currentWindow: true,
     });

mail/components/extensions/test/browser/browser_ext_spaces.js

@@ -961,10 +961,10 @@ async function test_query({ permissions }) {
       hasManagement ? [space_1, space_2] : []
     );
 
-    await query({ id: space_1.id }, [space_1]);
-    await query({ id: other_1.id }, [other_1]);
-    await query({ id: space_2.id }, [space_2]);
-    await query({ id: other_11.id }, [other_11]);
+    await query({ spaceId: space_1.id }, [space_1]);
+    await query({ spaceId: other_1.id }, [other_1]);
+    await query({ spaceId: space_2.id }, [space_2]);
+    await query({ spaceId: other_11.id }, [other_11]);
     await query({ name: "space_1" }, [other_1, space_1]);
     await query({ name: "space_2" }, [space_2]);
     await query({ name: "space_11" }, [other_11]);