1488176 - Emails and Folders WebExtensions API

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

We should start to think about what an API to retrieve emails from the email store, and how that relates to folders and tags. Thunderbird is mainly built on the use of folders, but I think we should try to make the API not too specific to folders.

It would probably be good to draft the API in a shared doc before we make recommendations in this bug, but I wanted to get this started since emails are one of the more basic building blocks of Thunderbird.

Comment 1

[liliakai]

Can we get a target release for this feature? Seems to be a big blocker as far as being able to develop a useful WebExtensions-based add-on.

Do WebExtensions currently have access to the email & folder API used by legacy add-ons? Or will they?

Is there currently, in any of the stable/beta/nightly channels, any other way for pure WebExtensions to access emails?

Comment 2

[Geoff Lankow (:darktrojan)]

Target release? When it's done. I'm working on it now, so hopefully it will make it to 65.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1488176-webext-accounts-messages-1.diff

This doesn't do a lot yet, but the accounts API gives the extension enough information about folders to start interacting with the mail tabs API, and the messages API shows how the extension will interact with message flags.

What really needs to happen next is working out what a "message" object looks like, i.e. what you get when calling browser.messages.get.

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1488176-webext-accounts-messages-2.diff

It helps if I add half the files to the patch.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9021078 [details] [diff] [review]
1488176-webext-accounts-messages-2.diff

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

::: mail/components/extensions/parent/ext-accounts.js
@@ +17,5 @@
> +
> +  account.QueryInterface(Ci.nsIMsgAccount);
> +  let folders;
> +  let server = account.incomingServer;
> +  if (["imap", "nntp", "none", "pop3"].includes(server.type)) {

why this limitation?

@@ +18,5 @@
> +  account.QueryInterface(Ci.nsIMsgAccount);
> +  let folders;
> +  let server = account.incomingServer;
> +  if (["imap", "nntp", "none", "pop3"].includes(server.type)) {
> +    folders = [];

might as well declare it here too, and not earlier

@@ +50,5 @@
> +          let account = MailServices.accounts.getAccount(accountId);
> +          if (account) {
> +            return convertAccount(account);
> +          }
> +          return null;

huh? isn't this all just |return MailServices.accounts.getAccount(accountId);|

::: mail/components/extensions/parent/ext-mail.js
@@ +1053,5 @@
>  
>  /**
>   * A map of numeric identifiers to messages for easy reference.
>   */
>  var messageTracker = {

this must be from some other patch. It's not in tree

::: mail/components/extensions/parent/ext-messages.js
@@ +19,5 @@
> +        async update(messageId, newProperties) {
> +          let msgHdr = messageTracker.getMessage(messageId);
> +          if (!msgHdr) {
> +            return;
> +          }

I suppose this should throw an Error?

::: mail/locales/en-US/chrome/messenger/addons.properties
@@ +162,5 @@
>  webextPerms.description.find=Read the text of all open tabs
>  webextPerms.description.geolocation=Access your location
>  webextPerms.description.history=Access browsing history
>  webextPerms.description.management=Monitor extension usage and manage themes
> +webextPerms.description.messages=Read your email messages

looks like updating too

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #3)
> What really needs to happen next is working out what a "message" object
> looks like, i.e. what you get when calling browser.messages.get.

Yeah that's not easy. We need to figure that out for internal usage too... Would be nice if we could do something graphql(-ish?) so the caller could decide how much of the actual data to include.

Comment 7

[Geoff Lankow (:darktrojan)]

(In reply to Magnus Melin [:mkmelin] from comment #5)
> ::: mail/components/extensions/parent/ext-mail.js
> @@ +1053,5 @@
> >  
> >  /**
> >   * A map of numeric identifiers to messages for easy reference.
> >   */
> >  var messageTracker = {
> 
> this must be from some other patch. It's not in tree

I see you haven't read bug 1499617 yet :-)

> ::: mail/components/extensions/parent/ext-accounts.js
> @@ +17,5 @@
> > +
> > +  account.QueryInterface(Ci.nsIMsgAccount);
> > +  let folders;
> > +  let server = account.incomingServer;
> > +  if (["imap", "nntp", "none", "pop3"].includes(server.type)) {
> 
> why this limitation?

I don't want to include chat accounts ("im" type). I don't know if any other types exist, but I think it's better to be explicit about what we do include than what we don't include.

> @@ +50,5 @@
> > +          let account = MailServices.accounts.getAccount(accountId);
> > +          if (account) {
> > +            return convertAccount(account);
> > +          }
> > +          return null;
> 
> huh? isn't this all just |return
> MailServices.accounts.getAccount(accountId);|

That would pass an nsIMsgAccount to WebExtensions. And it doesn't include the exclusion above.

> ::: mail/locales/en-US/chrome/messenger/addons.properties
> @@ +162,5 @@
> >  webextPerms.description.find=Read the text of all open tabs
> >  webextPerms.description.geolocation=Access your location
> >  webextPerms.description.history=Access browsing history
> >  webextPerms.description.management=Monitor extension usage and manage themes
> > +webextPerms.description.messages=Read your email messages
> 
> looks like updating too

Yeah, I don't have a complete picture of what permissions look like yet. (Actually this string has the wrong name too, should be "messagesRead" to actually work.) At the moment my thought is:
* Read message headers and make non-destructive changes
* Read complete messages
* Move, delete, any other changes that could destroy the message

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #7)
> (In reply to Magnus Melin [:mkmelin] from comment #5)
> > ::: mail/components/extensions/parent/ext-mail.js
> > @@ +1053,5 @@
> > >  
> > >  /**
> > >   * A map of numeric identifiers to messages for easy reference.
> > >   */
> > >  var messageTracker = {
> > 
> > this must be from some other patch. It's not in tree
> 
> I see you haven't read bug 1499617 yet :-)
> 
> > ::: mail/components/extensions/parent/ext-accounts.js
> > @@ +17,5 @@
> > > +
> > > +  account.QueryInterface(Ci.nsIMsgAccount);
> > > +  let folders;
> > > +  let server = account.incomingServer;
> > > +  if (["imap", "nntp", "none", "pop3"].includes(server.type)) {
> > 
> > why this limitation?
> 
> I don't want to include chat accounts ("im" type). I don't know if any other
> types exist, but I think it's better to be explicit about what we do include
> than what we don't include.

There's internally at least rss and movemail types, but of course 3rd party account types should also be considered. Maybe better to just exclude the "im" type which is indeed not fitting the picture.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1488176-webext-accounts-messages-3.diff

Just an update because it's needed to test the latest patch in bug 1499617.

Comment 10

[Mihai]

Can't wait to see this in action!
Is there a branch that one can compile directly, as opposed to applying patches?

Comment 11

[Geoff Lankow (:darktrojan)]

No there isn't, sorry.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1488176-webext-accounts-messages-4.diff

Every time I come to post this, I get delayed by something. Here it is at last.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1488176-webext-accounts-messages-test-1.diff

Comment 14

[liliakai]

Nice to see this coming along.

It looks like `browser.messages.getFull` will return a JS object representing the parsed mime message.

As an extension developer, I also want a way to get the "original" message, meaning a single Blob containing the full headers and mime-encoded contents.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9029343 [details] [diff] [review]
1488176-webext-accounts-messages-4.diff

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

::: mail/components/extensions/parent/ext-mail.js
@@ +1184,5 @@
> +    };
> +  },
> +
> +  _createEnumerator(array) {
> +    let count =  array.length;

nit: double space
but you don't need this tmp variable

::: mail/components/extensions/parent/ext-messages.js
@@ +21,5 @@
> +this.messages = class extends ExtensionAPI {
> +  getAPI(context) {
> +    return {
> +      messages: {
> +        async list({ accountId, path }) {

not sure I'm too happy about this api

Could we instead pass in a limit parameter, and an optional start(offset) parameter?

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9029352 [details] [diff] [review]
1488176-webext-accounts-messages-test-1.diff

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

::: mail/components/extensions/test/browser/browser_ext_mailTabs.js
@@ +254,5 @@
> +      }
> +      browser.test.assertEq(expectedCount, actual.messages.length);
> +    }
> +
> +    // Because of bad design, we must for the WebExtensions mechanism to load ext-mailTabs.js,

must what?

Comment 17

[Geoff Lankow (:darktrojan)]

> Could we instead pass in a limit parameter, and an optional start(offset) parameter?

If I know what extension coders are like, they'll use a massive limit parameter, which might work on their powerful machine but cripple a user's 10-year-old piece of junk. Better to put the user in charge with a preference, even if nobody ever changes it. Also, if an offset parameter is used we could go to any point in the list, which means we need to keep the whole list. The only sensible offset is the next message (why would you want to skip messages or go back?), which is what my patch already does.

Comment 18

[Magnus Melin [:mkmelin]]

(In reply to Geoff Lankow (:darktrojan) from comment #17)
> > Could we instead pass in a limit parameter, and an optional start(offset) parameter?
> 
> If I know what extension coders are like, they'll use a massive limit
> parameter, which might work on their powerful machine but cripple a user's
> 10-year-old piece of junk. 

But there may well be use cases to just get last 5-10 messages or so.

> Also, if an offset parameter is
> used we could go to any point in the list, which means we need to keep the
> whole list. 

Doesn't seems necessary to keep. 

>  The only sensible offset is the next message (why would you want
> to skip messages or go back?), which is what my patch already does.

If you have a limit of 10, offset of 10,20,30... is what you'd likely want yes.
Surely you may want to go one page back. Just look at any page with paged results.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Something else to keep in mind about limit/offset based APIs: if the message list changes inbetween calls, then your offset will be wrong and you may miss a message.

Comment 20

[Magnus Melin [:mkmelin]]

It could be nice to be able to give out a cursor (similar to IDBCursor), but I don't know if that's feasible.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/da68367e23e5
Temporarily disable tests and code awaiting bug 1488176; rs=me

Comment 22

[Geoff Lankow (:darktrojan)]

Oops, this one wasn't supposed to be resolved.

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9029343 [details] [diff] [review]
1488176-webext-accounts-messages-4.diff

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

Some drive by feedback as suggestions:

::: mail/components/extensions/parent/ext-accounts.js
@@ +5,5 @@
> +ChromeUtils.defineModuleGetter(this, "MailServices", "resource:///modules/MailServices.jsm");
> +var { fixIterator } = ChromeUtils.import("resource:///modules/iteratorUtils.jsm", null);
> +
> +function convertAccount(account) {
> +  account.QueryInterface(Ci.nsIMsgAccount);

In my experience a QI will often have the intended side effect, but there are situations (especially with missing nsIClassInfo) where it will not. I'd suggest assigning the qi result to a variable and using that instead.

@@ +23,5 @@
> +
> +  return {
> +    id: account.key,
> +    name: account.incomingServer.prettyName,
> +    hostName: account.incomingServer.hostName,

Would it make sense to call this incomingServerName so we can later extend this to also provide the outgoing server used?

::: mail/components/extensions/schemas/accounts.json
@@ +19,5 @@
> +    "namespace": "accounts",
> +    "permissions": [
> +      "accountsRead"
> +    ],
> +    "types": [

too bad the schema doesn't include a type for the account return type. Maybe we can add one (even if unused) so it shows up in the documentation?

@@ +23,5 @@
> +    "types": [
> +      {
> +        "id": "MailFolder",
> +        "type": "object",
> +        "description": "A folder object, as returned by the <var>list</var> and <var>get</var> methods.",

I probably just haven't found this yet, but is there a query function that will allow searching for folders with certain options?

::: mail/components/extensions/schemas/messages.json
@@ +35,5 @@
> +      },
> +      {
> +        "name": "continueList",
> +        "type": "function",
> +        "description": "Returns the next chunk of messages in a list. See :doc:`how-to/messageLists` for more information.",

Where is this doc? I wonder if we could return an object that has an iterator which automatically gets the next chunks as necessary. Might not work with all the cloning and such that wx apis do.

@@ +59,5 @@
> +      },
> +      {
> +        "name": "getFull",
> +        "type": "function",
> +        "description": "Returns a specified message.",

Can we have a type for the message again just for documentation purposes?

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9029343 [details] [diff] [review]
1488176-webext-accounts-messages-4.diff

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

Also, the folders api could use a few events so people know when to refresh their folders list etc.

::: mail/components/extensions/schemas/accounts.json
@@ +40,5 @@
> +      }
> +    ],
> +    "functions": [
> +      {
> +        "name": "list",

Having an accounts list api makes total sense. I'm wondering if it makes sense to provide a way to list subfolders for a given folder?

Comment 25

[Geoff Lankow (:darktrojan)]

Attachment: 1488176-webext-accounts-messages-5.diff

These patches are getting really out-of-date.

Comment 26

[Geoff Lankow (:darktrojan)]

Attachment: 1488176-webext-accounts-messages-test-2.diff

Comment 27

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #23)
> > +  return {
> > +    id: account.key,
> > +    name: account.incomingServer.prettyName,
> > +    hostName: account.incomingServer.hostName,
> 
> Would it make sense to call this incomingServerName so we can later extend
> this to also provide the outgoing server used?

I've got rid of it altogether. It didn't serve any useful purpose, but I thought it might've when originally writing this code. Also not adding it now prevents us from being hamstrung later.

> too bad the schema doesn't include a type for the account return type. Maybe
> we can add one (even if unused) so it shows up in the documentation?

We talked about documentation, and I said I thought I could add this here but that's not the case. Still, the readthedocs documentation has return values because I add them in.

> I probably just haven't found this yet, but is there a query function that
> will allow searching for folders with certain options?

No, but we can add that and maybe extend the folder object later. Let's get this landed.

> ::: mail/components/extensions/schemas/messages.json
> @@ +35,5 @@
> > +      },
> > +      {
> > +        "name": "continueList",
> > +        "type": "function",
> > +        "description": "Returns the next chunk of messages in a list. See :doc:`how-to/messageLists` for more information.",
> 
> Where is this doc? I wonder if we could return an object that has an
> iterator which automatically gets the next chunks as necessary. Might not
> work with all the cloning and such that wx apis do.

https://thunderbird-webextensions.readthedocs.io/en/latest/how-to/messageLists.html

I've tried returning enumerators and generators through various child process shenanigans and nothing works. It seems to be that have to pass all the data from WebExtensions land to the extension in one go. Considering that, I think my proposed solution is pretty good.

> 
> @@ +59,5 @@
> > +      },
> > +      {
> > +        "name": "getFull",
> > +        "type": "function",
> > +        "description": "Returns a specified message.",
> 
> Can we have a type for the message again just for documentation purposes?

That's explained on the RTD page for this API. It's a shame that kind of detail can't be in the schema, but that's how it is.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #24)
> Also, the folders api could use a few events so people know when to refresh
> their folders list etc.

Yes, let's add that in a follow-up. This patch was supposed to be a starting point for things like that, but it's been stuck here for so long…

> Having an accounts list api makes total sense. I'm wondering if it makes
> sense to provide a way to list subfolders for a given folder?

How about folders.map(f => f.path.startsWith("/foo/"))?

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9033933 [details] [diff] [review]
1488176-webext-accounts-messages-5.diff

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

This looks good, just some nits and one item to consider:

::: mail/components/extensions/parent/ext-messages.js
@@ +4,5 @@
> +
> +ChromeUtils.defineModuleGetter(this, "MailServices", "resource:///modules/MailServices.jsm");
> +ChromeUtils.defineModuleGetter(this, "MsgHdrToMimeMessage", "resource:///modules/gloda/mimemsg.js");
> +
> +function convertPart(part) {

Can you document this function?

@@ +61,5 @@
> +            }
> +          }
> +        },
> +        async listTags() {
> +          return MailServices.tags.getAllTags({}).map(function({ key, tag, color, ordinal }) {

This could probably be an arrow function. I'm not sure what mail does, but calendar even uses eslint's prefer-arrow-callback.

::: mail/components/extensions/schemas/messages.json
@@ +59,5 @@
> +      },
> +      {
> +        "name": "getFull",
> +        "type": "function",
> +        "description": "Returns a specified message.",

Can you expand the description here to show how get/getFull differ? This reminded me of the windows API, where instead of using two functions there is a "populate" argument. See https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/windows/getAll . Do you think it makes sense to do this here as well?

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9033935 [details] [diff] [review]
1488176-webext-accounts-messages-test-2.diff

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

::: mail/components/extensions/test/browser/browser_ext_mailTabs.js
@@ +235,2 @@
>      window.gFolderTreeView.selectFolder(folderMap.get(newFolderPath));
> +    await new Promise(executeSoon);

Does this work ok with the aName argument? Might be best to just be verbose in executing this.

::: mail/components/extensions/test/xpcshell/test_ext_messages.js
@@ +12,5 @@
> +  account = createAccount();
> +  rootFolder = account.incomingServer.rootFolder;
> +  subFolders = [...rootFolder.subFolders];
> +  createMessages(subFolders[0], 99);
> +  createMessages(subFolders[1], 1);

This is somewhat misusing add_task. I think what you can do is:

function run_test() {
  account = createAccount();
  // ...remaining setup here...

  run_next_test();
}

Comment 30

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #29)

Comment on attachment 9033935 [details] [diff] [review]
1488176-webext-accounts-messages-test-2.diff

Review of attachment 9033935 [details] [diff] [review]:

::: mail/components/extensions/test/browser/browser_ext_mailTabs.js
@@ +235,2 @@

 window.gFolderTreeView.selectFolder(folderMap.get(newFolderPath));

• await new Promise(executeSoon);

Does this work ok with the aName argument? Might be best to just be verbose
in executing this.

I don't understand the question.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/18f7dcdd1bed
Accounts and messages WebExtensions API; r=Fallen
https://hg.mozilla.org/comm-central/rev/7acd73ce23af
Accounts and messages WebExtensions API - tests; r=Fallen
https://hg.mozilla.org/comm-central/rev/02def84cbd9c
Backed out changeset da68367e23e5

Comment 32

[Geoff Lankow (:darktrojan)]

Damn, I caused a failure on Windows:

comm/mail/components/extensions/test/xpcshell/test_ext_accounts.js | test_accounts - [test_accounts : 311] undefined - Expected: /Trash/Ϟ, Actual: /Trash/b52bc214 - false == true

I know what causes this and I thought I'd fixed it before, but clearly whatever I did about it didn't make it into the patch.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/35c4cf91abd0
Temporarily disable failing test; rs=bustage-fix

Comment 34

[Geoff Lankow (:darktrojan)]

It's failing because of NS_MsgHashIfNecessary. I think we should just ignore it and update the test.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/00d6b98310b3
Disable failing test in a way that doesn't make it worse; rs=bustage-fix

Comment 36

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Geoff Lankow (:darktrojan) from comment #30)

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #29)

Comment on attachment 9033935 [details] [diff] [review]
1488176-webext-accounts-messages-test-2.diff

Review of attachment 9033935 [details] [diff] [review]:
::: mail/components/extensions/test/browser/browser_ext_mailTabs.js
@@ +235,2 @@

 window.gFolderTreeView.selectFolder(folderMap.get(newFolderPath));

• await new Promise(executeSoon);

Does this work ok with the aName argument? Might be best to just be verbose
in executing this.

I don't understand the question.

Sorry for being unclear, I agree the aName thing was not well explained. I meant that the Promise constructor takes a function(resolve, reject) as its argument. executeSoon's signature is executeSoon(callback, aName). So you are passing the rejection function as the aName argument of executeSoon, so it will show in the logs as "function () { [native code] }". You could consider just using await new Promise(resolve => executeSoon(resolve));.

Comment 37

[Geoff Lankow (:darktrojan)]

I've decided to keep the encoded paths on Windows, since there are already reasons the path may not be what a developer might expect.

Comment 38

[Geoff Lankow (:darktrojan)]

Attachment: 1488176-permission-string.diff

I want to change one of the permission strings to be more accurate. It doesn't appear to be on Pontoon yet, so this shouldn't be a problem as long as it lands soon.

Comment 39

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9036753 [details] [diff] [review]
1488176-permission-string.diff

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

Doesn't this need a string name change due to changing meaning? R=me with that

Comment 40

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Ok read your comment, please proceed

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/172542e8cd4a
Change permission string to be more accurate; r=Fallen DONTBUILD