1520338 - Messages API function to move or copy messages

Status: RESOLVED FIXED

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

Description

[Geoff Lankow (:darktrojan)]

This should be pretty easy, using the message copy service. We'll need a new permission, since I don't think messagesRead should cover it. I originally planned to call it messagesWrite, but I may change my mind.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1520338-webext-messages-move-1.diff

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9036785 [details] [diff] [review]
1520338-webext-messages-move-1.diff

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

::: mail/components/extensions/parent/ext-messages.js
@@ +59,5 @@
> +            {
> +              OnStartCopy() {
> +                console.log("OnStartCopy");
> +              },
> +              OnProgress(aProgress, aProgressMax) {

can we drop the a's here

::: mail/locales/en-US/chrome/messenger/addons.properties
@@ +163,5 @@
>  webextPerms.description.geolocation=Access your location
>  webextPerms.description.history=Access browsing history
>  webextPerms.description.management=Monitor extension usage and manage themes
>  webextPerms.description.messagesRead=Read your email messages and mark or tag them
> +webextPerms.description.messagesWrite=Read your email messages and mark, tag, move, copy, or delete them

Maybe this should only focus on the modifying part?

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9036785 [details] [diff] [review]
1520338-webext-messages-move-1.diff

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

API looks good. When you do the tests, maybe it would make sense to have one that ensures the write permission is actually adhered to.

::: mail/components/extensions/parent/ext-messages.js
@@ +42,5 @@
> +        }
> +
> +        let sourceSet = folderMap.get(sourceFolder);
> +        if (!sourceSet) {
> +          sourceSet = new Set();

You could consider using a DefaultMap like the one defined in https://searchfox.org/comm-central/source/common/src/ChromeManifest.jsm#303

@@ +51,5 @@
> +
> +
> +      for (let [sourceFolder, sourceSet] of folderMap.entries()) {
> +        let messages = toXPCOMArray(sourceSet.values(), Ci.nsIMutableArray);
> +        await new Promise((resolve) => {

This would wait for the first copy to finish before it does the next. Can you either collect the messages up front into one array and then copy them all, or save the promises and then await Promise.all() ?

@@ +66,5 @@
> +              SetMessageKey(aKey) {
> +                console.log("SetMessageKey", aKey);
> +              },
> +              GetMessageId(aMessageId) {
> +                console.log("GetMessageId", aMessageId);

I think we can drop the debug messages here?

@@ +70,5 @@
> +                console.log("GetMessageId", aMessageId);
> +              },
> +              OnStopCopy(aStatus) {
> +                console.log("OnStopCopy", aStatus);
> +                resolve();

In the final version you'll probably want to handle the case where copying some messages fails.

@@ +74,5 @@
> +                resolve();
> +              },
> +            },
> +            null, // msgWindow,
> +            false // allowUndo

Would we want to allow the caller to specify if an undo is allowed?

::: mail/locales/en-US/chrome/messenger/addons.properties
@@ +163,5 @@
>  webextPerms.description.geolocation=Access your location
>  webextPerms.description.history=Access browsing history
>  webextPerms.description.management=Monitor extension usage and manage themes
>  webextPerms.description.messagesRead=Read your email messages and mark or tag them
> +webextPerms.description.messagesWrite=Read your email messages and mark, tag, move, copy, or delete them

I agree with Magnus here. IIUC the permission is for copying and moving, but the developer would not actually be able to read the email, correct?

Comment 4

[Geoff Lankow (:darktrojan)]

I've since discovered I can't do the permissions in a way that messagesWrite implies messagesRead, so that string will be changing.

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

This would wait for the first copy to finish before it does the next. Can
you either collect the messages up front into one array and then copy them
all, or save the promises and then await Promise.all() ?

I can't see the first option working as for some reason the source folder must be specified. The second option sounds okay, but can we do multiple operations like this at the same time? What if there's many simultaneous operations?

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

If starting many copy ops at the same time will result in performance or dataloss issues we should be sure to fix them in the copy service. We could chunk the promises array, but I think in WX code is the wrong place to do rate limiting.

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1520338-webext-messages-move-2.diff

Still a WIP.

I've added the delete message this was missing plus some testing. It seems that deleteMessages is synchronous in some cases and not in others, which I haven't yet dealt with. I've got nowhere with allowUndo, that could be because I don't have a window.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9038142 [details] [diff] [review]
1520338-webext-messages-move-2.diff

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

::: mail/components/extensions/parent/ext-messages.js
@@ +80,5 @@
> +      } catch (ex) {
> +        Cu.reportError(ex);
> +        if (isMove)
> +          throw new ExtensionError("Unexpected error moving messages");
> +        throw new ExtensionError("Unexpected error copying messages");

you probably want to make the ex included, for clarity
throw new ExtensionError(`Unexpected error copying messages: ${ex});"

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1520338-webext-messages-move-3.diff

With bug 1521706 fixed I think this is ready for review.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9038730 [details] [diff] [review]
1520338-webext-messages-move-3.diff

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

Stealing the review again, r=philipp with these comments considered:

::: mail/components/extensions/parent/ext-messages.js
@@ +79,5 @@
> +        await Promise.all(promises);
> +      } catch (ex) {
> +        Cu.reportError(ex);
> +        if (isMove)
> +          throw new ExtensionError(`Unexpected error moving messages: ${ex}`);

{} for one line ifs, I think we have this in eslint?

::: mail/locales/en-US/chrome/messenger/addons.properties
@@ +163,5 @@
>  webextPerms.description.geolocation=Access your location
>  webextPerms.description.history=Access browsing history
>  webextPerms.description.management=Monitor extension usage and manage themes
>  webextPerms.description.messagesRead=Read your email messages and mark or tag them
> +webextPerms.description.messagesWrite=Move, copy, or delete your email messages

For the sake of keeping this general, maybe it should also incorporate the possibility of creating messages. Not that we have an API for it now, but then we cover create, modify, delete operations. Feel free to summarize that in a different way than just adding Create.

Comment 10

[Geoff Lankow (:darktrojan)]

I think it's better if I rename that permission "messagesMove" or something like it. "messagesWrite" was never the right thing, but it's what I started using and never changed it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/132113b4ce51
Messages API function to move, copy, or delete messages; r=Fallen