Closed Bug 1520338 Opened 9 months ago Closed 9 months ago

Messages API function to move or copy messages

Categories

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

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 66.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.

Attached patch 1520338-webext-messages-move-1.diff (obsolete) β€” β€” Splinter Review
Attachment #9036785 - Flags: feedback?(philipp)
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 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?
Attachment #9036785 - Flags: feedback?(philipp) → feedback+

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?

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.

Attached patch 1520338-webext-messages-move-2.diff (obsolete) β€” β€” Splinter Review

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.

Attachment #9036785 - Attachment is obsolete: true
Attachment #9038142 - Flags: feedback?(philipp)
Attachment #9038142 - Flags: feedback?(mkmelin+mozilla)
Depends on: 1521706
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});"
Attachment #9038142 - Flags: feedback?(mkmelin+mozilla) → feedback+

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

Attachment #9038142 - Attachment is obsolete: true
Attachment #9038142 - Flags: feedback?(philipp)
Attachment #9038730 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9038730 - Flags: review?(mkmelin+mozilla) → review+

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.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/132113b4ce51
Messages API function to move, copy, or delete messages; r=Fallen

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 66.0
You need to log in before you can comment on or make changes to this bug.