1747456 - `compose.sendMessage` options.mode is missing many send modes

Status: RESOLVED FIXED

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

Description

[siefkenj]

Steps to reproduce:

The compose.sendMessage(tabId, options) call allows options.mode === "default" || "sendNow" || "sendLater". However, there are many more modes supported.

https://searchfox.org/comm-central/rev/9d89afee9a36846454e2a4ef5226cb037c05b1e2/suite/mailnews/components/compose/content/MsgComposeCommands.js#1886

Seems to list 7 options. Of particular interest to me is saveAsDraft. But maybe most (all?) should be exposed?

Comment 1

[Alexander Bergmann]

For my own add-on Mail Merge having the two modes "saveAsDraft" and "saveAsTemplate" would be necessary.

Comment 2

[John Bieling (:TbSync)]

@Magnus: Before working on this, I need to address a few issues, which makes adapting the code difficult for me.

Issue #1:

The function CompleteGenericSendMessage is async, but is not awaited in GenericSendMessage, which causes what I call "async early exits".

For example:

function SendMessage() {
  pillifyRecipients();
  let sendInBackground = Services.prefs.getBoolPref(
    "mailnews.sendInBackground"
  );
  if (sendInBackground && AppConstants.platform != "macosx") {
    let count = [...Services.wm.getEnumerator(null)].length;
    if (count == 1) {
      sendInBackground = false;
    }
  }

  GenericSendMessage(
    sendInBackground
      ? Ci.nsIMsgCompDeliverMode.Background
      : Ci.nsIMsgCompDeliverMode.Now
  );
  ExitFullscreenMode();
}

GenericSendMessage returns before the first await is hit in CompleteGenericSendMessage which means ExitFullscreenMode() is executed BEFORE the actual send process. Do not know if that is intentional. Should we add a comment to point that out?

Same happens here:

function SaveAsDraft() {
  gAutoSaveKickedIn = false;
  gEditingDraft = true;

  GenericSendMessage(Ci.nsIMsgCompDeliverMode.SaveAsDraft);
  defaultSaveOperation = "draft";
}

Why do we set defaultSaveOperation after the call to GenericSendMessage?

I am asking all this, because I want to return the Promise so the caller can decide, whether to await the entire send process. I could do this:

function SaveAsDraft() {
  gAutoSaveKickedIn = false;
  gEditingDraft = true;

  return GenericSendMessage(Ci.nsIMsgCompDeliverMode.SaveAsDraft).then(() => {
    defaultSaveOperation = "draft";
  })
}

And also add a return here

This would however change the execution flow, defaultSaveOperation would now be set only once CompleteGenericSendMessage has finished, and not before the call to gMsgCompose.sendMsg.

Issue #2

gMsgCompose.sendMsg returns before the actual send has finished. The progress listener is monitoring the send process, which ends here.

So all the code after gMsgCompose.sendMsg is executed before the send has finished and also on failed sends. Including the aftersend event and telemetry collection. The catch around gMsgCompose.sendMsg does not catch any errors encountered during send.

Did that behaviour change with the switch to the JavaScript send implementation?
Would it be better to promisify and await the progress listener?

For the WebExt API caller, I would like to be able to await the entire send process, so I would like to change the code to await the progress listener there as well. Objections?

Comment 3

[Magnus Melin [:mkmelin]]

Duplicate of bug 1732554? Yes we should fix that.

Comment 4

[Ping Chen [:rnons] (away)]

Yes, I think issue#1 is bug 1732554. We should add await.

Issue #2
gMsgCompose.sendMsg returns before the actual send has finished. The progress listener is monitoring the send process, which ends here.
So all the code after gMsgCompose.sendMsg is executed before the send has finished and also on failed sends. Including the aftersend event and telemetry collection. The catch around gMsgCompose.sendMsg does not catch any errors encountered during send.

I think this is the same before the js rewrite, listeners are used to notify sending error and finished. nsIMsgCompose.sendMsg returns only means the message has been assembled and passed over to the smtp/nntp module. The communications with servers are async.

Would it be better to promisify and await the progress listener?

Currently we have several listeners (progress, sending, copy), they're difficult to follow. I think we have plan to rewrite nsMsgCompose.cpp (nsIMsgCompose.idl) in JS. Before that, perhaps it's easier to just wait for listeners.

Comment 5

[John Bieling (:TbSync)]

Attachment: Bug 1747456 - Allow compose.sendMessage() to await the entire send process and add compose.saveMessage(). r=mkmelin

Comment 6

[John Bieling (:TbSync)]

try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=0daae72edcce09674d6cfab6d276dac718c3d35b

1. I do not know why I get a clang-format error, it does not tell me what he does not like and I get no lint errors locally
2. I had to requestLongerTimeout for Linux, I already split the test and I probably need to split it more, but since this includes a string, I would like to get it landed before merge day.

Comment 7

[Ping Chen [:rnons] (away)]

I do not know why I get a clang-format error, it does not tell me what he does not like and I get no lint errors locally

Maybe related to bug 1771483, I think you can ignore the error.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/114f30010216
Allow compose.sendMessage() to await the entire send process and add compose.saveMessage(). r=mkmelin

Comment 9

[Magnus Melin [:mkmelin]]

Looks like debug test failure: https://treeherder.mozilla.org/logviewer?job_id=379535332&repo=comm-central&lineNumber=5030

Comment 10

[John Bieling (:TbSync)]

All platforms or just MacOS? Only that test (of the new API function) or also tests of core functionality?

Comment 11

[Magnus Melin [:mkmelin]]

Looks like all platforms for debug. Just that test I think.

Comment 12

[John Bieling (:TbSync)]

Attachment: Bug 1747456 - Make folder optional in MessageHeader (which it is) to fix debug test fails. r=mkmelin

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/70099087423a
Make folder optional in MessageHeader (which it is) to fix debug test fails. r=mkmelin