Closed Bug 1488176 Opened 6 years ago Closed 6 years ago

Emails and Folders WebExtensions API

Categories

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

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 66.0

People

(Reporter: Fallen, Assigned: darktrojan)

References

Details

Attachments

(3 files, 5 obsolete files)

1488176-webext-accounts-messages-5.diff
(deleted), patch
Fallen
: review+
Details | Diff | Splinter Review
1488176-webext-accounts-messages-test-2.diff
(deleted), patch
Fallen
: review+
Details | Diff | Splinter Review
1488176-permission-string.diff
(deleted), patch
Fallen
: review+
Details | Diff | Splinter Review
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.
Component: General → Add-Ons: Extensions API
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?
Flags: needinfo?(philipp)
Target release? When it's done. I'm working on it now, so hopefully it will make it to 65.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Flags: needinfo?(philipp)
Attached patch 1488176-webext-accounts-messages-1.diff (obsolete) (deleted) β€” β€” Splinter Review
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.
Attached patch 1488176-webext-accounts-messages-2.diff (obsolete) (deleted) β€” β€” Splinter Review
It helps if I add half the files to the patch.
Attachment #9020964 - Attachment is obsolete: true
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
(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.
(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
(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.
Attached patch 1488176-webext-accounts-messages-3.diff (obsolete) (deleted) β€” β€” Splinter Review
Just an update because it's needed to test the latest patch in bug 1499617.
Attachment #9021078 - Attachment is obsolete: true
Can't wait to see this in action! Is there a branch that one can compile directly, as opposed to applying patches?
Flags: needinfo?(geoff)
No there isn't, sorry.
Flags: needinfo?(geoff)
Attached patch 1488176-webext-accounts-messages-4.diff (obsolete) (deleted) β€” β€” Splinter Review
Every time I come to post this, I get delayed by something. Here it is at last.
Attachment #9022534 - Attachment is obsolete: true
Attachment #9029343 - Flags: review?(mkmelin+mozilla)
Attached patch 1488176-webext-accounts-messages-test-1.diff (obsolete) (deleted) β€” β€” Splinter Review
Attachment #9029352 - Flags: review?(mkmelin+mozilla)
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 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 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?
> 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.
(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.
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.
It could be nice to be able to give out a cursor (similar to IDBCursor), but I don't know if that's feasible.
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/da68367e23e5 Temporarily disable tests and code awaiting bug 1488176; rs=me
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Oops, this one wasn't supposed to be resolved.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 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?
Attached patch 1488176-webext-accounts-messages-5.diff (deleted) β€” β€” Splinter Review
These patches are getting really out-of-date.
Attachment #9029343 - Attachment is obsolete: true
Attachment #9029343 - Flags: review?(mkmelin+mozilla)
Attachment #9033933 - Flags: review?(philipp)
Attachment #9033933 - Flags: review?(mkmelin+mozilla)
Attachment #9029352 - Attachment is obsolete: true
Attachment #9029352 - Flags: review?(mkmelin+mozilla)
Attachment #9033935 - Flags: review?(philipp)
Attachment #9033935 - Flags: review?(mkmelin+mozilla)
(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 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?
Attachment #9033933 - Flags: review?(philipp)
Attachment #9033933 - Flags: review?(mkmelin+mozilla)
Attachment #9033933 - Flags: review+
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(); }
Attachment #9033935 - Flags: review?(philipp)
Attachment #9033935 - Flags: review?(mkmelin+mozilla)
Attachment #9033935 - Flags: review+

(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.

Pushed by geoff@darktrojan.net:
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

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0

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.

Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/35c4cf91abd0 Temporarily disable failing test; rs=bustage-fix

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

Whiteboard: [Thunderbird-testfailure: X Windows only]
Whiteboard: [Thunderbird-testfailure: X Windows only] → [Thunderbird-testfailure: X Windows only][Thunderbird-disabled-test]
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/00d6b98310b3 Disable failing test in a way that doesn't make it worse; rs=bustage-fix

(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));.

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.

Whiteboard: [Thunderbird-testfailure: X Windows only][Thunderbird-disabled-test]
Blocks: 1520338
Attached patch 1488176-permission-string.diff (deleted) β€” β€” Splinter Review

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.

Attachment #9036753 - Flags: review?(philipp)
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
Attachment #9036753 - Flags: review?(philipp) → review+

Ok read your comment, please proceed

Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/172542e8cd4a Change permission string to be more accurate; r=Fallen DONTBUILD
Blocks: 1520367
Depends on: 1531591
Depends on: 1531593
Depends on: 1531317
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: