321355 - 'Edit as New Message' of draft overwrites/deletes the original message (wrongly behaves like 'Edit draft')

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[OstGote!]

It seems that bug 288702 has broken some of the original behavior. Some were fixed in the meantime, so in bug 292568 (overwriting only for drafts & templates, see bug 292568 comment 11) and with bug 317396 (overwriting only for drafts, see bug 317396 comment 9). 
BUT this is still a different behaviour as Mozilla 1.7/TB 1.0 and as expected by many users if someone does a "Edit As New" (see bug 292568 comment 14, bug 288062 comment 11). The command is "Edit (Message) As New" not simply Edit ...

Expected Result: Edit a draft message as new and later saving should NOT overwrite the original message in the drafts folder.

Comment 1

[Peter Lairo]

"loss of data" -> Severity = Critical

Comment 3

[Jamie Nettles]

This is a horrible, horrible bug.  This bug destroys data!  It makes "Edit as New" completely meaningless when applied to drafts.  Every month I lose time because of this bug.  Please, please, please fix it.

Comment 4

[Ed Greenberg]

In the same vein, when you "edit a draft" in the drafts folder of an account not the default, the from address dropdown is reset to the default.  When you save, the message is saved in the drafts folder of the default address and deleted from the drafts folder of the original non-default address.

Comment 5

[Wayne Mery (:wsmwk)]

this seems really bad - nominating wanted.
workaround is to use template - does not happen with template, only draft

seen with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090515 Shredder/3.0b3pre

Comment 6

[Magnus Melin [:mkmelin]]

Would be good to fix for tb3.

Comment 8

[Mitra Ardron]

Adding my voice to the need to fix this . . I'm sure I'm not the only one to have a really bad data-loss experience with it ... Typical scenario

Need to write similar message to two people . 
Write message A - save it
Drafts -> select message -> Edit message as New
Save it 
Hit send 

Message A has disappeared - all the work that went into it lost, even worse, I probably miss-remember it as having been sent.  

Using Templates isn't an answer to this, this is a data-loss bug where the expected behavior doesn't happen, and the result is loss of data.

Comment 9

[Anje]

OS Vista
Thunderbird version 24.5.0
Request two options with a saved draft email.
1. Edit draft
This opens in a new Write message for editing the original draft and when I save it as a draft, it overwrites the orginal.
2. Edit as new message
This opens in a new Write message for editing, but when I save as draft, it does not overwrite the orignal instead it saves it as a new draft message.

currently, I have to either 
open draft using edit as a new message option and open a new Write message, then copy paste all the saved draft content into new Write message and then edit the section required - then save the new Write message as the second edited draft. note these two drafts have very similar content but go to different people. So I need both as separate drafts.
OR I have to open the original draft as edit as a new message, edit content and save the orginal draft as a template and save in that folder. You cannot move it from template to drafts as it still overwrites.  This is a workarounnd but not a satisfactory solution.

Comment 10

[romney.yeomanwalker]

I'm using Thunderbird 24.6.0 on Windows 7 Home Premium Service Pack 1 (64-bit).

Having saved a draft message, I expect that double-clicking it would open the draft to allow me to work on it further, while Edit as New should create a new message from the saved draft, which can also be saved as a new draft and subsequently sent completely independently of the original draft.

It seems that Edit as New doesn't currently work like this - and I think it should!

Comment 11

[Thomas D. (:thomas8)]

OMG! *#!#+§$%&!?

Please CC me to such bad bugs. It might give me a heart attack, but otherwise it's better to look the monster into the eye and face the truth. That monster which grew in full view of its official caretakers, and in fact they were always too few to ever gain full control and really "take care", clean, nourish, and grow the animal into something beautiful and domesticated. So as the beast became more ugly over time from systematic neglection, surprise - the crowds in their masses somehow never got warm with such a thing, whose inner beauty, strength and full potential was hard to see from a rough outside. Worse, so many specific appeals from the crowds how to domesticate the thing and how to increase the attraction just went with the wind unheard. So in something like a self-fulfilling prophecy, the top caretakers decided that well, if people are not interested in this creature as we offer it now, with that little care we're doing, why should we care? Let's release the beast into the wilderness, perhaps it'll find new friends and caretakers, whilst we can focus on the brave new world of mobile everything. So there we are now, a small circle of volunteer friends taking care of the beast, trying to heal the wounds of time, polishing and adding here and there, and shaking our heads over and over again over the unbelievable neglection this once beautiful creature had to suffer over time. Imagine the majesty and beauty this could have achieved, with an investment of more manpower, more care, more listening, more sincerity, more focus on UX, stringent bug management, and more direction...

Comment 12

[Mitra Ardron]

Email sent ...

Comment 13

[Thomas D. (:thomas8)]

Perhaps WADA can analyse what is required to fix this?

Comment 14

[WADA:World Anti-bad-Duping Agency]

(In reply to Thomas D. from comment #13)
> Perhaps WADA can analyse what is required to fix this?

It's pretty simple that "Edit As New of draft mail in Drafts folder" of  "Mozilla Mail&New.Thunderbird code" did do/does do same thing as "Edit Draft", isn't it?

I think following ; 
    "Edit As New" menu was added in addition to Edit Draft, Reply, Forward etc.
    "Draft Edit" routine does do same thing as "Edit Draft" when "Draft Edit" routine is invoked via "Edit As New" menu.
    (i.e. "Edit As New" menu is not for drfaft mail. "Edit As New" is for creation os new mail from existent mail.)

It's all up to Thunderbird developers.
  If "Edit As New" is meaningless in "Draft Edit" routine, when Drafts folder, disable "Edit As New" menu in Commnd Controler etc. 
  If  "Edit As New of draft mail" is mandatory & important feature, support "Edit As New of draft mail" appropriatelyy,
      in order to avoid user's confusion and misleding.and unwanted bug open.

Comment 15

[Thomas D. (:thomas8)]

(In reply to WADA from comment #14)
> (In reply to Thomas D. from comment #13)
> > Perhaps WADA can analyse what is required to fix this?
> 
> It's pretty simple that "Edit As New of draft mail in Drafts folder" of 
> "Mozilla Mail&New.Thunderbird code" did do/does do same thing as "Edit
> Draft", isn't it?

Maybe, although internally we call...
> 1869   composeMsgByType(Components.interfaces.nsIMsgCompType.Template);
...which is .Template so it should *not* usually behave like "Edit draft".
"Edit draft" and "Edit as new message" are two clearly distinct features.

Btw, "Edit draft" command (the most common action on drafts!) does not even exist in the menus, which looks like another bug, in terms of ux-consistency and probably accessability.

> It's all up to Thunderbird developers.

Which is now a team of volunteers including you and me...
Given that this already has wanted-thunderbird+ from :mkmelin (comment 6) and strong support from :wsmwk (comment 5), and my strong support (comment 11), plus several supportive user stories (again from known figures), I don't think we need to wait for anyone else to decide on the desired UX of this.

>   If  "Edit As New of draft mail" is mandatory & important feature, support
> "Edit As New of draft mail" appropriatelyy,
>       in order to avoid user's confusion and misleding.and unwanted bug open.

I can't see any reason (apart from manpower bottlenecks) against this feature, and we shouldn't disable commands and create exceptions without an actual design need.
User stories on this bug show that users like this feature and intuitively use it for their own workflows, which are just broken by this bug.
As described in comment 9, it makes perfect sense to start out from a base draft and then create clone copies of that for different recipients. Which avoids the mental and practical overhead of creating a dedicated template for that, which is probably neither intuitive nor desirable for the scenarios of this bug. Even if I start a draft and at some point along the way I want to try out re-arranging or modifying the draft while perserving a copy of the status quo, this looks like an intuitive way of creating a fresh copy of an existing draft.

So as far as the UX of this is concerned, in view of the above support, I'll just make the call and confirm this is wanted (by virtue of being one of TB's key UX contributors) :)

Comment 16

[Thomas D. (:thomas8)]

WADA, could you comment where along the code path we could change things (and how) so that it doesn't overwrite the original draft?

OK, here's the workflow for this:

http://mxr.mozilla.org/comm-central/search?string=MsgEditMessageAsNew%28%29

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#725
(and some other starting points in xul)
725     <menuitem id="mailContext-editAsNew"
726               label="&contextEditMsgAsNew.label;"
727               accesskey="&contextEditMsgAsNew.accesskey;"
728               oncommand="MsgEditMessageAsNew();"/>

<menuitem id="mailContext-editAsNew" label="Edit As New Message" accesskey="E" oncommand="MsgEditMessageAsNew();"/>

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1867
1867 function MsgEditMessageAsNew()
1868 {
	
1870 }

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1540
1540 function composeMsgByType(aCompType, aEvent) {
1541   // If we're the hidden window, then we're not going to have a gFolderDisplay
1542   // to work out existing folders, so just use null.
1543   let msgFolder = gFolderDisplay ? GetFirstSelectedMsgFolder() : null;
1544   let msgUris = gFolderDisplay ? gFolderDisplay.selectedMessageUris : null;
1545 
1546   if (aEvent && aEvent.shiftKey) {
1547     ComposeMessage(aCompType,
1548                    Components.interfaces.nsIMsgCompFormat.OppositeOfDefault,
1549                    msgFolder, msgUris);
1550   }
1551   else {
1552     ComposeMessage(aCompType, Components.interfaces.nsIMsgCompFormat.Default,
1553                    msgFolder, msgUris);
1554   }
1555 }

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#154
153 // type is a nsIMsgCompType and format is a nsIMsgCompFormat
154 function ComposeMessage(type, format, folder, messageArray)
155 {

[... too long to paste here, but if I read this correctly, we end up here:]

262           let hdrIdentity = getIdentityForHeader(hdr, type);
263           MailServices.compose.OpenComposeWindow(null, hdr, messageUri, type,
264                                                  format, hdrIdentity, msgWindow);
265         }
266       }

Then, correct me if I'm wrong, we get here:
http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#501
499 
500 NS_IMETHODIMP
501 nsMsgComposeService::OpenComposeWindow(const char *msgComposeWindowURL, nsIMsgDBHdr *origMsgHdr, const char *originalMsgURI,
502   MSG_ComposeType type, MSG_ComposeFormat format, nsIMsgIdentity * aIdentity, nsIMsgWindow *aMsgWindow)
503 {
504   nsresult rv;
505 
506   nsCOMPtr<nsIMsgIdentity> identity = aIdentity;
507   if (!identity)
508     GetDefaultIdentity(getter_AddRefs(identity));
509 
510   /* Actually, the only way to implement forward inline is to simulate a template message.
511      Maybe one day when we will have more time we can change that
512   */
513   if (type == nsIMsgCompType::ForwardInline || type == nsIMsgCompType::Draft || type == nsIMsgCompType::Template
514     || type == nsIMsgCompType::ReplyWithTemplate || type == nsIMsgCompType::Redirect)
515   {
516     nsAutoCString uriToOpen(originalMsgURI);
517     uriToOpen += (uriToOpen.FindChar('?') == kNotFound) ? '?' : '&';
518     uriToOpen.Append("fetchCompleteMessage=true");
519     if (type == nsIMsgCompType::Redirect)
520       uriToOpen.Append("&redirect=true");
521 
522     aMsgWindow->SetCharsetOverride(true);
523     return LoadDraftOrTemplate(uriToOpen, type == nsIMsgCompType::ForwardInline || type == nsIMsgCompType::Draft ?
524                                nsMimeOutput::nsMimeMessageDraftOrTemplate : nsMimeOutput::nsMimeMessageEditorTemplate,
525                                identity, originalMsgURI, origMsgHdr, type == nsIMsgCompType::ForwardInline,
526                                format == nsIMsgCompFormat::OppositeOfDefault, aMsgWindow);
527   }

http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgComposeService.cpp#1487
1487 /**
1488  * LoadDraftOrTemplate
1489  *   Helper routine used to run msgURI through libmime in order to fetch the contents for a
1490  *   draft or template.
1491  */
1492 nsresult
1493 nsMsgComposeService::LoadDraftOrTemplate(const nsACString& aMsgURI, nsMimeOutputType aOutType,
1494                                          nsIMsgIdentity * aIdentity, const char * aOriginalMsgURI,
1495                                          nsIMsgDBHdr * aOrigMsgHdr,
1496                                          bool aForwardInline,
1497                                          bool overrideComposeFormat,
1498                                          nsIMsgWindow *aMsgWindow)
1499 {
1500   return RunMessageThroughMimeDraft(aMsgURI, aOutType, aIdentity,
1501                                     aOriginalMsgURI, aOrigMsgHdr,
1502                                     aForwardInline, EmptyString(),
1503                                     overrideComposeFormat, aMsgWindow);
1504 }

1505 
1506 /**
1507  * Run the aMsgURI message through libmime. We set various attributes of the 
1508  * nsIMimeStreamConverter so mimedrft.cpp will know what to do with the message
1509  * when its done streaming. Usually that will be opening a compose window
1510  * with the contents of the message, but if forwardTo is non-empty, mimedrft.cpp
1511  * will forward the contents directly.
1512  *
1513  * @param aMsgURI URI to stream, which is the msgUri + any extra terms, e.g.,
1514  *                "redirect=true".
1515  * @param aOutType  nsMimeOutput::nsMimeMessageDraftOrTemplate or
1516  *                  nsMimeOutput::nsMimeMessageEditorTemplate
1517  * @param aIdentity identity to use for the new message
1518  * @param aOriginalMsgURI msgURI w/o any extra terms
1519  * @param aOrigMsgHdr nsIMsgDBHdr corresponding to aOriginalMsgURI
1520  * @param aForwardInline true if doing a forward inline
1521  * @param aForwardTo  e-mail address to forward msg to. This is used for
1522  *                     forward inline message filter actions.
1523  * @param aOverrideComposeFormat True if the user had shift key down when
1524                                  doing a command that opens the compose window,
1525  *                               which means we switch the compose window used
1526  *                               from the default.
1527  * @param aMsgWindow msgWindow to pass into DisplayMessage.
1528  */
1529 nsresult
1530 nsMsgComposeService::RunMessageThroughMimeDraft(
1531             const nsACString& aMsgURI, nsMimeOutputType aOutType,
1532             nsIMsgIdentity * aIdentity, const char * aOriginalMsgURI,
1533             nsIMsgDBHdr * aOrigMsgHdr,
1534             bool aForwardInline,
1535             const nsAString &aForwardTo,
1536             bool aOverrideComposeFormat,
1537             nsIMsgWindow *aMsgWindow)
1538 {
1539   nsCOMPtr <nsIMsgMessageService> messageService;
1540   nsresult rv = GetMessageServiceFromURI(aMsgURI, getter_AddRefs(messageService));
1541   NS_ENSURE_SUCCESS(rv, rv);
1542 
1543   // Create a mime parser (nsIMimeStreamConverter)to do the conversion.
1544   nsCOMPtr<nsIMimeStreamConverter> mimeConverter =
1545     do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv);
1546   NS_ENSURE_SUCCESS(rv, rv);
1547 
1548   mimeConverter->SetMimeOutputType(aOutType); // Set the type of output for libmime
1549   mimeConverter->SetForwardInline(aForwardInline);
1550   if (!aForwardTo.IsEmpty())
1551   {
1552     mimeConverter->SetForwardInlineFilter(true);
1553     mimeConverter->SetForwardToAddress(aForwardTo);
1554   }
1555   mimeConverter->SetOverrideComposeFormat(aOverrideComposeFormat);
1556   mimeConverter->SetIdentity(aIdentity);
1557   mimeConverter->SetOriginalMsgURI(aOriginalMsgURI);
1558   mimeConverter->SetOrigMsgHdr(aOrigMsgHdr);
1559 
1560   nsCOMPtr<nsIURI> url;
1561   bool fileUrl = StringBeginsWith(aMsgURI, NS_LITERAL_CSTRING("file:"));
1562   nsCString mailboxUri(aMsgURI);
1563   if (fileUrl)
1564   {
1565     // We loaded a .eml file from a file: url. Construct equivalent mailbox url.
1566     mailboxUri.Replace(0, 5, NS_LITERAL_CSTRING("mailbox:"));
1567     mailboxUri.Append(NS_LITERAL_CSTRING("&number=0"));
1568     // Need this to prevent nsMsgCompose::TagEmbeddedObjects from setting
1569     // inline images as moz-do-not-send.
1570     mimeConverter->SetOriginalMsgURI(mailboxUri.get());
1571   }
1572   if (fileUrl || PromiseFlatCString(aMsgURI).Find("&type=application/x-message-display") >= 0)
1573     rv = NS_NewURI(getter_AddRefs(url), mailboxUri);
1574   else
1575     rv = messageService->GetUrlForUri(PromiseFlatCString(aMsgURI).get(), getter_AddRefs(url), aMsgWindow);
1576   NS_ENSURE_SUCCESS(rv, rv);
[...]
1607   // Now, just plug the two together and get the hell out of the way!
1608   nsCOMPtr<nsIStreamListener> streamListener = do_QueryInterface(mimeConverter);
1609   return messageService->DisplayMessage(PromiseFlatCString(aMsgURI).get(), streamListener,
1610                                         aMsgWindow, nullptr, mailCharset.get(), nullptr);;
1611 }

So I think that's where we finally display the "new" msg composition window.

Comment 17

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #16)
Grrrr, when we finally get comment editing in bugzilla... :(
Accidentally cut instead of copied line 1869 when writing comment 15, so ftr, here it is:

> http://mxr.mozilla.org/comm-central/source/mail/base/content/
> mailWindowOverlay.js#1867
> 1867 function MsgEditMessageAsNew()
> 1868 {

1869   composeMsgByType(Components.interfaces.nsIMsgCompType.Template);
 	
> 1870 }

Comment 18

[WADA:World Anti-bad-Duping Agency]

(In reply to Thomas D. from comment #16)
> WADA, could you comment where along the code path we could change things 
> (and how) so that it doesn't overwrite the original draft?

homas D., please don't ask me any more, because I believe that "disable 'Edit As New' menu for Drafts" is sufficient and feasible, as I already stated in my previous comment #14.

Comment 19

[Mitra Ardron]

I agree with you Thomas D. "Edit as New" is deceptive. Its bad wording that causes data loss if you expect it to work in the same way as "Edit as New" does everywhere else. 

Sure - disabling the menu item is better than leaving it there. 
Changing its name to "Edit Draft" would also be acceptable elimination of the data-loss potential.

But **MUCH** better is what Thomas is suggesting, i.e. to make it actually edit a new copy just as happens everywhere else in TB.

Comment 20

[romney.yeomanwalker]

I sometimes need to send two similar messages at the same time. My approach is to write the first message and save it as a draft. Then use "Edit as New" to create a copy of the draft, make the necessary changes and save that as a draft, so that I have two messages ready to send at the same time.

Only I discovered the hard way that it doesn't work like that, and my second edited draft overwrites the original draft.

I would really like to see "Edit as New" implemented correctly.

If this cannot be done, then it should definitely be disabled/removed from the menus. I have lost quite a lot of edits this way in the past.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: do_not_overwrite_draft_message_when_editing_as_new.diff

For the reference, I am attaching a patch to fix this issue. I have not investigated much further though.
An xpcshell test is necessary.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix with a mozmill test

https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=e8c446fdeac7

Thomas and others, could you please confirm following binary can work fine as you expect?

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-e8c446fdeac7/

Comment 23

[WADA:World Anti-bad-Duping Agency]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22
> Fix with a mozmill test

"Double click of mail at Templates" is assigned to "Edit As New"(no "Edit" in Templates).
What is currently aassigned to "Double click of mail at Drafts"? 
If it's "Edit As New" currently, it should be chaned to "Edit" if the behaviour change of "Edit As New at Drafts" will be made.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

(In reply to WADA from comment #23)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #22
> > Fix with a mozmill test
> 
> "Double click of mail at Templates" is assigned to "Edit As New"(no "Edit"
> in Templates).
> What is currently aassigned to "Double click of mail at Drafts"? 
> If it's "Edit As New" currently, it should be chaned to "Edit" if the
> behaviour change of "Edit As New at Drafts" will be made.

The double click behavior is "Edit" on drafts folder with attachment 8530495 [details] [diff] [review].
attachment 8530495 [details] [diff] [review] does not change UI/UX at all, so I think the current behavior on drafts folder is "Edit".

I am not a person who decides UI/UX, so I can't judge which behavior is more proper in drafts folder though.

Comment 25

[Thomas D. (:thomas8)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> (In reply to WADA from comment #23)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #22
> > > Fix with a mozmill test
> > 
> > "Double click of mail at Templates" is assigned to "Edit As New"(no "Edit"
> > in Templates).
> > What is currently aassigned to "Double click of mail at Drafts"? 
> > If it's "Edit As New" currently, it should be chaned to "Edit" if the
> > behaviour change of "Edit As New at Drafts" will be made.
> 
> The double click behavior is "Edit" on drafts folder with attachment 8530495 [details] [diff] [review]
> [©] [details] [diff] [review].
> attachment 8530495 [details] [diff] [review] does not change UI/UX at
> all, so I think the current behavior on drafts folder is "Edit".
> 
> I am not a person who decides UI/UX, so I can't judge which behavior is more
> proper in drafts folder though.

As a decisive UX person, I confirm that the current behaviour of "double-click on draft = Edit (original) draft" is intentional and obviously the correct default action.
"Edit as new message" (clone the current draft msg to create and work on the copy) is a separate feature offered on context menu, the functionality of which will be fixed here.

"Edit draft" (the default action) is currently missing on context menu, and should be added (we also offer default actions on context menus of other things like inbox msgs, AB contact cards etc.). Hiro, can you add this command to your patch if it's not there yet, or you prefer a separate bug for that?

I'll test the trybuild a bit later when I have time. Pls needinfo me for such requests. Thanks!

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(In reply to Thomas D. from comment #25)
> "Edit draft" (the default action) is currently missing on context menu, and
> should be added (we also offer default actions on context menus of other
> things like inbox msgs, AB contact cards etc.).

Ah, I did not notice that.

> Hiro, can you add this command to your patch if it's not there yet, or you prefer a separate bug
> for that?

I'd prefer to open a new bug for that.

> I'll test the trybuild a bit later when I have time. Pls needinfo me for
> such requests. Thanks!

I am sorry for my lazy!

Comment 27

[WADA:World Anti-bad-Duping Agency]

(In reply to Thomas D. from comment #25)
> "Edit draft" (the default action) is currently missing on context menu, (snip)

"Edit" button is shown at Message Header Box when mail is mail in Drafts folder.
This is done by ;
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#3029
>  setDraftEditMessage: function() 
>       if (msgHdr.folder.isSpecialFolder(nsMsgFolderFlags.Drafts, true))
>           {
>                let buttons = [{ ..., ..., ..., 
>                     callback: function(aNotification, aButton) {
>                                     MsgComposeDraftMessage();
>                                     return true; // keep notification open
>                     }
>                }] ;
MsgComposeDraftMessage() is sibling of MsgEditMessageAsNew(), MsgForwardAsAttachment(event), MsgForwardAsInline(event) etc. which is defined in mailWindowOverlay.js.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1862

To do same thing at menu, following is needed.
1. Add "Edit Draft" to "Message" of main menu and Context menu of mail, as currently done on "Edit As New".
    Detailed definition at one of 2 is sufficient, if other watches primary menuitem item or toolbarbutton.
    "common command use in menuitem/toolbarbutton definition" is also possible.
2. If "command" is utilized, in Command Controller, enable/disable the menuitem according to context, status etc.,
    as currently done on Forward, Forward As, Edit As New etc.
3. When "Edit Draft" menu, it's maybe better hidden at main menu/context menu if folder is not Drafts,
    instead of simply disabling(gray out at menu).
    In this case, control in onmenupopup is additionlly needed.
> EditAsNew relevant definitions for menu
>   http://mxr.mozilla.org/comm-central/search?string=editasnew
> Edit As New in main menu
>   http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2687
> Edit As New in context menu
>   http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#725

I think following are different jobs and (b) is better done in separated bug. (b) is never fault of patch for this bug :-)
(a) "Edit As New" should be "Edit As New" at anywhere. "Edit As New" shouldn't be "Edit draft".
       Change "Edit As New of draft mail" from same one as "Edit draft" to same one as "Edit New of Template or ordinal mail".
(b) If "Edit As New" is "Edit As New" at anywhere as expected, we loose "Edit draft" via. menu.
      Add equivalence to "Edit button at Message Header Box of draft mail" in main menu and/or context menu.

Comment 28

[Thomas D. (:thomas8)]

(In reply to WADA from comment #27)

Thank you WADA for this helpful comment, with which I agree! :)

> To do same thing at menu, following is needed.
> 1. Add "Edit Draft" to "Message" of main menu and Context menu of mail, as
> currently done on "Edit As New".
>     Detailed definition at one of 2 is sufficient, if other watches primary
> menuitem item or toolbarbutton.
>     "common command use in menuitem/toolbarbutton definition" is also
> possible.

Yes, central command controller would be best, than let the UI observe that.

> 2. If "command" is utilized, in Command Controller, enable/disable the
> menuitem according to context, status etc.,
>     as currently done on Forward, Forward As, Edit As New etc.
> 3. When "Edit Draft" menu, it's maybe better hidden at main menu/context
> menu if folder is not Drafts,
>     instead of simply disabling(gray out at menu).

+1, hiding this command for non-draft folders lgtm.

>     In this case, control in onmenupopup is additionlly needed.

Yeah, most likely. I wonder if that could be streamlined using something like a "hidden" attribute on the command controller which is then auto-forwarded to UI elements observing that command?

> > EditAsNew relevant definitions for menu
> >   http://mxr.mozilla.org/comm-central/search?string=editasnew
> > Edit As New in main menu
> >   http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2687
> > Edit As New in context menu
> >   http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#725
> 
> I think following are different jobs and (b) is better done in separated
> bug. (b) is never fault of patch for this bug :-)
> (a) "Edit As New" should be "Edit As New" at anywhere. "Edit As New"
> shouldn't be "Edit draft".
>        Change "Edit As New of draft mail" from same one as "Edit draft" to
> same one as "Edit New of Template or ordinal mail".

Rephrase for clarity:
(a) "Edit As New" should mean/do "Edit as New" anywhere in the UI, because
    "Edit As New" cannot and should never mean "Edit this draft"
    Consequently, in this bug we are changing "Edit As New" for message in draft folder from current wrong behaviour ("Edit draft") to correct behaviour, which is same as "Edit As New Msg" of msg in Template folder or any other ordinary (non-draft) msg.

> (b) If "Edit As New" is "Edit As New" at anywhere as expected, we loose
> "Edit draft" via. menu.

(b) After this bug, when "Edit As New Msg" means/does "Edit as New Msg" anywhere in the UI as expected, we'll have lost "Edit draft" functionality via menu for draft msgs, which might be considered a regression of this bug. So I think before landing this bug, we should fix that:

>       Add equivalence to "Edit button at Message Header Box of draft mail"
> in main menu and/or context menu.

Iow (in a new bug) add "Edit draft" command to main menu's message menu and context menu in drafts folder (we currently *do* offer default actions in context menus, which imo makes sense, because it assists discovery and understanding, and also helps when you are tired, more control-oriented, or just insecure, and thus prefer explicit context menu actions over trusting that double click or Enter will do the right thing for you).

Comment 29

[Thomas D. (:thomas8)]

needinfo myself, see comment 22 (test try build)

Comment 30

[Thomas D. (:thomas8)]

Bug 1106412 - Menu items for "Edit draft" functionality missing after bug 321355 fixes "Edit as New Message" to do just that

(In reply to Thomas D. from comment #25)
> "Edit draft" (the default action) is currently missing on context menu, and
> should be added (we also offer default actions on context menus of other
> things like inbox msgs, AB contact cards etc.).

(In reply to WADA from comment #27)

> (b) ... Add equivalence to "Edit button at Message Header Box of draft mail"
> in main menu and/or context menu.

(In reply to Thomas D. from comment #28)

> Iow (in a new bug) add "Edit draft" command to main menu's message menu and
> context menu in drafts folder (we currently *do* offer default actions in
> context menus...

Comment 31

[Thomas D. (:thomas8)]

> needinfo myself, see comment 22 (test try build)

Comment 32

[WADA:World Anti-bad-Duping Agency]

(In reply to Thomas D. from comment #28)
> > 2. If "command" is utilized, in Command Controller, enable/disable the
> > menuitem according to context, status etc.,
> >     as currently done on Forward, Forward As, Edit As New etc.
> > 3. When "Edit Draft" menu, it's maybe better hidden at main menu/context
> > menu if folder is not Drafts,
> >     instead of simply disabling(gray out at menu).
> 
> +1, hiding this command for non-draft folders lgtm.
> 
> >     In this case, control in onmenupopup is additionlly needed.
> 
> Yeah, most likely. I wonder if that could be streamlined using something like a "hidden" attribute on the command controller 
> which is then auto-forwarded to UI elements observing that command?

Example of this kind of menu control : Context menu of message at Thread Pane -> "Tag".
   Because "Tag added to mail in Tb" is not solid element in Tb(user can add/delete/alter it any time),
   shown row in dropdown menu of "Context menu of message at Thread Pane" -> "Tag"
   is dynamically generated upon onmenupopup.
If required action is simply "Show or Hide a menuitem", needless to say, "display: visible/none of CSS" etc. can be freely used.

Comment 33

[Thomas D. (:thomas8)]

Thanks WADA! Ideally, such comments should now go into Bug 1106412 ;)

(In reply to WADA from comment #32)
> If required action is simply "Show or Hide a menuitem", needless to say,
> "display: visible/none of CSS" etc. can be freely used.

Comment 34

[WADA:World Anti-bad-Duping Agency]

Comment on attachment 8530495 [details] [diff] [review]
Fix with a mozmill test

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

It looks one liner patch.
>  -        fields->SetDraftId(mdd->url_name);
>      After it, msgComposeType = Redirect or Template is set, then CreateTheComposeWindow() is called.
By this change, I think "delete of original draft upon first draft save/send" won't be executed when "Edit draft".
Please read bug 1106412 comment #3.
If it's right, where/how should fields->SetDraftId(mdd->url_name) be set by whom when "Edit draft"?

Comment 35

[WADA:World Anti-bad-Duping Agency]

Sorry, removed line by patch was
https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#l1455
instead of 
https://hg.mozilla.org/comm-central/file/3f82c17bf9af/mailnews/mime/src/mimedrft.cpp#l1487
Sorry for my confusion.
If cmposition-type=Draft, draftID is still set, so "delete edited draft" is executed as expected upon first save/send.

Comment 36

[WADA:World Anti-bad-Duping Agency]

When "Edit As New of non-Drafts folder", gMsgCompose.compFields.draftID=original mail, gMsgCompose.deleteDraft=false was set, and original mail(in draftID) is not deleted upon "Save as draft".
However, when "Edit As New of a draft mail in Drafts folder", gMsgCompose.compFields.draftID=edited draft, window.deleteDraft=false was set, but edited draft=original=draftID was deleted upon save.
If draftID is set, and if Drafts folder, Tb always deletes draftID regardless of deleteDraft upon draft save/send?

Why "edited mail by Edit As New is deleted if Drafts" but "edited mail by Edit As New is not deleted if non-Drafts" is perhaps following :
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3674
>    After end of SaveAsDrft/SaveAsTemplate/QueueForLater/DeliverBackground,  RemoveCurrentDraftMessage() is called.
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3728
>   if draftID  is not set, RemoveCurrentDraftMessage does do nothing.
>   if draftID is set && folderFlags has nsMsgFolderFlags::Drafts, RemoveCurrentDraftMessage deletes draftID.
>   even if draftID  is set, if folderFlags doesn't have nsMsgFolderFlags::Drafts, RemoveCurrentDraftMessage does do nothing.
Small mismatch on "delete original or not" is seen in composer component modules.
    Someone deletes original if draftID is set && deleteDraft=true regardless of folder type,
   (I think this is concept/design in composer since initial, because deleteDraft is set only when composition-type=Draft)
    but other someone deletes original if draftID is set && folder is Drafts folder regardless of deleteDraft.
This mismatch maybe is difference between "process during mail composition" and "post save/send process".
   "post save/send process", RemoveCurrentDraftMessage(), looks mainly coded for IMAP.
   Because UID of newly saved draft mail can not known upon draft save execution(append) unless UIDPLUS is supported,
   UID of newly saved draft mail should be obtained by SEARCH command.
   And delete of old draft mail == "uid store UIDofOldDraft +Flags(\Deleted)" should be executed independently if imap.
But it maybe fault in OnStopCopy() and RemoveCurrentDraftMessage() : 
-  OnStopCopy() sets deleteDraft=true before first call of RemoveCurrentDraftMessage().
-  RemoveCurrentDraftMessage() desn't check gMsgCompose.deleteDraft.
-  RemoveCurrentDraftMessage() deletes message of draftID only when draftID is set && the draftID is in Drafts folder.
-  In RemoveCurrentDraftMessage(), if msgDBHdr for draftID is not found after save, 
    Tb assumes that it's IMAP and Drafts folder is not opened.
    However, "msgDBHdr for draftID is not found" can occur if following conditions are satisfied.
             Edit As New of mail in local mail folder
       && edited mail is deleted/Compacted or Offset of edited mail is changed by Compact
       &&  it's after first save.
    Is this a reason why phenomenon of "old draft is not deleted" on IMAP Drafts sometimes?
        If "append to IMAP Drafts for first draft save" is executed and mail data is accepted by server,
        and if above condition happens,
        and if IMAP Drafts folder access fails upon getting UID of newly saved draft mail,
        draftID may not be updated by UID of the newly saved draft mail.
        => After next draft save, "msgDBHdr for draftID is not found" occurs => "Delete previous draft" is skipped

Ikezoe-san, is my guess right?

Ikezoe-san, can you check change like next without removing mimedrft.cpp#l1455?
(A) Set deleteDraft=true after first RemoveCurrentDraftMessage call, in case of "deleteDraft=false for edited mail"
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3646
>     nsMsgComposeSendListener::OnStopCopy(nsresult aStatus)
> 3676       // We should only close the window if we are done. Things like templates
> 3677       // and drafts aren't done so their windows should stay open
> 3678       if (mDeliverMode == nsIMsgSend::nsMsgSaveAsDraft ||
> 3679           mDeliverMode == nsIMsgSend::nsMsgSaveAsTemplate)
> 3680       {
> 3681         msgCompose->NotifyStateListeners(nsIMsgComposeNotificationType::SaveInFolderDone, aStatus);
> 3682         // Remove the current draft msg when saving as draft/template is done.
> 3683  -      msgCompose->SetDeleteDraft(true); <== Shouldn't be set before first beforeRemoveCurrentDraftMessage call
> 3684         RemoveCurrentDraftMessage(msgCompose, true);
>          +      msgCompose->SetDeleteDraft(true);  <== Should be set after first beforeRemoveCurrentDraftMessage call
> 3685       }
> 3686       else
> 3687       {
> 3688         // Remove (possible) draft if we're in send later mode
> 3689         if (mDeliverMode == nsIMsgSend::nsMsgQueueForLater ||
> 3690             mDeliverMode == nsIMsgSend::nsMsgDeliverBackground)
> 3691         {
> 3692  -        msgCompose->SetDeleteDraft(true); <== Shouldn't be set before first beforeRemoveCurrentDraftMessage call
> 3693           RemoveCurrentDraftMessage(msgCompose, true);
>           +      msgCompose->SetDeleteDraft(true);  <== Should be set after first beforeRemoveCurrentDraftMessage call
> 3694         }
> 3695         msgCompose->CloseWindow(true);
> 3696       }
(B) Check deleteDraft in RemoveCurrentDraftMessage, according to design of deleteDraft
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3728
>     nsMsgComposeSendListener::RemoveCurrentDraftMessage(nsIMsgCompose *compObj, bool calledByCopy)
> http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3746
> +        bool deleteDraft=false;
>            // See http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#1383
> +        rv = compObj->GetDeleteDraft(getter_AddRefs(deleteDraft));
> +        NS_ASSERTION(NS_SUCCEEDED(rv), "RemoveCurrentDraftMessage can't get deleteDraft");
> +        if (NS_FAILED(rv))
> +        return rv;
> 3746   // Skip if no draft id (probably a new draft msg).
> 3747   if (NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty())
>      => if (NS_SUCCEEDED(rv) && !curDraftIdURL.IsEmpty()  && deleteDraft )
> );
> 3761         // only do this if it's a drafts or templates folder. <= Comment should be changed
> 3762         if (folderFlags & nsMsgFolderFlags::Drafts)
> 3763         { 
>                      => Delete of  message set in draftID is dione in this block. This is reason why problem hppens on Drafts only.
>                            After removing mimedrft.cpp#l1455, this check is not needed, but the is no need to remove this check.

composition-type=Template is "Never delete original(edited) mail" mode(i.e. original is never draft mail even in Drafts folder),
and composition-type=Draft is "Surely delete original(edited) mail" mode,
even if gMsgCompose.deleteDraft=false is always set when composition-type=Template
and even if gMsgCompose.deleteDraft=true is always set when composition-type=Draft.
If "original mail" is needed, it's always saved in gMsgCompose.originalMsgURI.
So, even if above change works and is correct action, I believe "removing mimedrft.cpp#l1455" is correct and mandatory action.

Ikezoe-san, do you think change of OnStopCopy() and RemoveCurrentDraftMessage() is needed?
Or "removing mimedrft.cpp#l1455" is sufficient, so such change is merely wasting of resources?

Comment 37

[WADA:World Anti-bad-Duping Agency]

Question about "OnStopCopy() /RemoveCurrentDraftMessage() by Send/Manul-Save and Auto-Save at same time".
Because Auto-Save is timer-pop event, it can happen even if Tb tries to serialize Send, Manul-Save, and Auto-Save.
IIRC, Tb tries to stop Auto-Save when Send is requested by setting flag in gMsgCompose.something. but it doesn't seem to work perfectly. So, "Auto-Save just after Send request", "Send just after Auto-Save is kicked" can occur. and non-deleted draft mail cn happen.
Ikezoe-san, is there easy way to resolve problem of "non-deleted draft mail due to Auto-Save" by small change of OnStopCopy() and/or RemoveCurrentDraftMessage() code?

Comment 38

[WADA:World Anti-bad-Duping Agency]

FYI.
I've opened Bug 1108441 for issue of Comment #36.
See Bug 638390 for problem in  "delete editing draft mail" which is relevant to AutoSave. AutoSave relevant issue should be processed in that bug instead of this bug. Sorry for excess comments to this bug.

Comment 39

[:aceman]

Thomas, can you summarize if there is anything to do in this bug? Does the patch work?

Comment 40

[Thomas D. (:thomas8)]

Comment on attachment 8517970 [details] [diff] [review]
do_not_overwrite_draft_message_when_editing_as_new.diff

Yes, it needs a determined developer to fix and land this :)
Probably it's ready to land as-is.
But I'm not entirely sure, too much hightech discussion here...
The desired UX is simple - "Edit as New" must do just that, namely to create and edit a duplicate copy (this bug), and for draft folders, we need a separate "Edit" command to edit the original draft (Bug 1106412).

I think this was wrongly obsoleted, because this one-liner seems to fix the main half of this bug per Wada's comment 34. The other patch attachment #8530495 [details] [diff] [review] is tests-only (probably the one-liner was forgotten).

It would be good to land this together with Bug 1106412 which has WIP patches, otherwise we're making things worse the other way round, esp. for accessibility-dependent users.

Comment 41

[Thomas D. (:thomas8)]

Per comments on bug 1106412, this should ideally involve creating cleaner code with central <command> which the other UI elements can then listen to / observe. But perhaps for this bug it is not necessary or we can do it later in another bug. For bug 110641 it's probably necessary because we're adding a missing command (I think).

Comment 42

[Jorg K (CEST = GMT+2)]

Looks like Hiro is not working on this any more. Aceman, could you take over?
I tried the code patch and it works: Use an existing draft, edit it, save. Now creates a new draft.
I will attach a patch with a proper header.

Could you take care of the test, please ;-)

Comment 43

[Jorg K (CEST = GMT+2)]

Comment on attachment 8517970 [details] [diff] [review]
do_not_overwrite_draft_message_when_editing_as_new.diff

This is not needed since it is contained in the other patch. It works though. Sorry, I won't attach anything since the other patch contains everything.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: Fix with a mozmill test (refreshed)

OK, I've refreshed the patch and I've run the two tests:
mozmake SOLO_TEST=composition/test-drafts.js mozmill-one (added new test)
mozmake SOLO_TEST=composition/test-charset-edit.js mozmill-one (needed to change since we need to "Edit draft" instead of "Edit as new message".

Looks good to me. Since it's not my own work, I can give it f+. Let's get it landed after 1.5 years of rotting. After all, it has six votes and is deemed critical.

Thanks to Thomas for the wake-up call.

Comment 45

[:aceman]

Comment on attachment 8726810 [details] [diff] [review]
Fix with a mozmill test (refreshed)

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

So this seems to work and I r+ the mozmill test.

I find it strange that something in mime decides the outcome of this operation. I'd expect the frontend code to say if it wants edit the draft or make a copy. So I'd like jcranmer to review the mime part. I don't know why the removed line was even there and if it does not break something else now.

::: mail/test/mozmill/composition/test-charset-edit.js
@@ +114,5 @@
>  
>    // Edit the original message. Charset should be windows-1252 now.
>    msg = select_click_row(0);
>    plan_for_new_window("msgcompose");
> +  mc.click(mc.eid("msgNotificationBar", {tagName: "button", label: "Edit"}));

When you use the notification bar, you need to wait for it until it slides in so you can click it. Without it, it may randomly fail (and it does for me locally).

Put something like 'wait_for_notification_to_show(mc, "msgNotificationBar", "draftMsgContent");' before the plan_for_new_window(). You can see it in test_open_draft_again().

@@ +155,5 @@
>  
>    // Edit the original message. Charset should be UTF-8 now.
>    msg = select_click_row(0);
>    plan_for_new_window("msgcompose");
> +  mc.click(mc.eid("msgNotificationBar", {tagName: "button", label: "Edit"}));

Same here.

::: mail/test/mozmill/composition/test-drafts.js
@@ +160,5 @@
>    assert_format_value("format_both", Ci.nsIMsgCompSendFormat.Both);
>  
>    close_compose_window(cwc);
>  
>    press_delete(mc); // clean up the created draft

Please modify the draft in this test (test_open_draft_again) and then at the end check there is only one draft in the folder. That should be the difference to the new test below when you open the draft via the menu (not the Edit button).

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: Fix with a mozmill test (v2)

Aceman, thanks for the r+, can you please check again.

Joshua, please review the
-  fields->SetDraftId(mdd->url_name);

Looking for GetDraftId, there are five calls in nsMsgCompose.cpp. Most have a comment like "if no draft ID, it's a new draft".

Comment 47

[Jorg K (CEST = GMT+2)]

Comment on attachment 8726937 [details] [diff] [review]
Fix with a mozmill test (v2)

Magnus, are you able to review the mime change so we don't have to wait for Joshua. Maybe we can even land this before the next branch date.

Comment 48

[:aceman]

Comment on attachment 8726937 [details] [diff] [review]
Fix with a mozmill test (v2)

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

r+ for the test with the fix below.

::: mail/test/mozmill/composition/test-charset-edit.js
@@ +115,5 @@
>    // Edit the original message. Charset should be windows-1252 now.
>    msg = select_click_row(0);
> +
> +  // Wait for the notification with the Edit button.
> +  wait_for_notification_to_show(mc, "msgNotificationBar", "draftMsgContent");

The test fails as the function is not defined. You need to import notificationbox-helpers in MODULE_REQUIRES.

Comment 49

[Jorg K (CEST = GMT+2)]

Attachment: Fix with a mozmill test (v2a).

Sorry, I ran test-drafts.js again manually, but not test-charset-edit.js, grrr ;-(
Now it works.

Carrying forward Aceman's r+ for the test and requesting review of the mime change
-  fields->SetDraftId(mdd->url_name);
via the superreview field.

Comment 50

[Magnus Melin [:mkmelin]]

Comment on attachment 8727043 [details] [diff] [review]
Fix with a mozmill test (v2a).

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

LGTM, r=mkmelin

Comment 51

[Jorg K (CEST = GMT+2)]

Oh, please put r=aceman,mkmelin.

Comment 52

[Jorg K (CEST = GMT+2)]

Comment on attachment 8727043 [details] [diff] [review]
Fix with a mozmill test (v2a).

[Approval Request Comment]
Regression caused by (bug #): No regression.
User impact if declined: Will work badly as before, see below.
Testing completed (on c-c, etc.): Manual and with Mozmill test.
Risk to taking this patch (and alternatives if risky):
Not risky. This has been a long-standing issue and annoyance deemed critical from 2005 with six votes. It's a one line code change.

Comment 53

[Kent James (:rkent)]

approval-comm-release is for Seamonkey builds. We will be using approval-comm-esr45 for further uplifts to Thunderbird.

Comment 54

[Jorg K (CEST = GMT+2)]

Comment on attachment 8727043 [details] [diff] [review]
Fix with a mozmill test (v2a).

I know. Sorry, I didn't see the flag. (Will fix the other bugs too.)

Comment 55

[Kent James (:rkent)]

http://hg.mozilla.org/releases/comm-aurora/rev/37f8814f5abd

Comment 56

[Jorg K (CEST = GMT+2)]

I'll land it on C-C myself using a transplant from Aurora.

Comment 57

[Kent James (:rkent)]

Comment on attachment 8727043 [details] [diff] [review]
Fix with a mozmill test (v2a).

http://hg.mozilla.org/releases/comm-beta/rev/e7e79f9aaf79
http://hg.mozilla.org/releases/comm-esr45/rev/60c360c6b4aa

Comment 58

[Kent James (:rkent)]

I had to adapt the mozmill test (which is not something I really do well) to uplift this, since another patch that also modified test-drafts.js conflicted. Now that test is failing (see treeherder on comm-esr45 or comm-beta). Could you investigate, and see the issue is my test adaptation or something else?

Comment 59

[:aceman]

Can you see if maybe bug 1106412 landed first? It should land after the bug here.
Or maybe that bug 1202165 is not on the branches.
Awyway, why are you removing the loop with cwins2 ? That wasn't in the original patch. And the failure mentions that variable.

Comment 60

[Kent James (:rkent)]

Bug 1202165 was not in the branches, and is marked as WONTFIX for the branches.

Can you prepare an adapted mozmill test that we can land? I'm not a mozmill kind of guy.

Comment 61

[:aceman]

I think you need to put this block back, which you removed in the landed patch:

-  let cwins2 = 0;
-  let e2 = Services.wm.getEnumerator("msgcompose");
-  while (e2.hasMoreElements()) {
-    e2.getNext();
-    cwins2++;
-  }

Comment 62

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/fb34bc19b35f - Backed out wrong merge
https://hg.mozilla.org/releases/comm-beta/rev/3a386f9a75b5 - landed again.

Please uplift to ESR45.

Aceman is right, I put that back. Sorry, you were writing comments while I was doing the work and not watching bugmail.

Comment 63

[Kent James (:rkent)]

http://hg.mozilla.org/releases/comm-esr45/rev/d6f4c7a7ce1f
http://hg.mozilla.org/releases/comm-esr45/rev/48f502911bac

Thanks for the help!

Comment 64

[Jorg K (CEST = GMT+2)]

The beta looks a lot greener than the ESR45, but I guess that's life.

Comment 65

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/1a06d4cf1ad8