1202165 - Delivery Format not preserved for saved drafts (Auto-Detect|Plaintext|HTML|Both)

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Thomas D. (:thomas8)]

STR

1) compose msg with HTML formatting which Auto-Detect misfeature considers discardable, e.g. attachment 633907 [details]

2) To prevent Auto-Detect's unpredictable destruction upon sending, explicitly pick another Delivery Format like HTML from Options > Delivery Format.

3) Save and close the composition

4) Reopen draft and verify Delivery Format

Actual result:

- User-defined Delivery Format is NOT preserved in the draft; upon reopening saved draft, always falls back to "Auto-Detect" and the subsequent dataloss of formatting associated therewith.
- User unnecessarily forced to verify and re-set desired Delivery Format (HTML)

Expected result:
- Delivery Format is an important message-specific setting, which must be preserved across closing and reopening draft

This might be a step on the way to Bug 136502 (to have a per-account user-configurable default setting for Delivery Format).

Comment 1

[Thomas D. (:thomas8)]

Hints for implementation

I imagine this to work the same way like receipt, attachment reminder, etc. for which we also save flags inside the draft:

> X-Mozilla-Draft-Info: internal/draft; vcard=0; receipt=0; DSN=0; uuencode=0;
> attachmentreminder=0

So we could have someting like:
> X-Mozilla-Draft-Info: deliveryformat=Auto|Text|HTML|TextAndHTML

Involved files e.g.

http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompFields.cpp#396

> 396 NS_IMETHODIMP nsMsgCompFields::GetAttachmentReminder(bool *_retval)
> 397 {
> 398   *_retval = m_attachmentReminder;
> 399   return NS_OK;
> 400 }
> 401 
> 402 NS_IMETHODIMP nsMsgCompFields::SetAttachmentReminder(bool value)
> 403 {
> 404   m_attachmentReminder = value;
> 405   return NS_OK;
> 406 }

We'd need similar methods for our flag here, but with string not bool (I'd much prefer strings over cryptic numbers).

http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompFields.cpp#75

> 75   m_attachmentReminder = false;

...and more files not yet listed here.

Comment 2

[Thomas D. (:thomas8)]

Bug 889315 has an overview over the conglomerate of delivery format ux failures.

Comment 3

[:aceman]

Suyash, I think you did the preserving of the attachment reminder in draft, can you do the same here?

Comment 4

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #3)
> Suyash, I think you did the preserving of the attachment reminder in draft,
> can you do the same here?

Will try to do. Thanks :)

Comment 5

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Comment 6

[:aceman]

Comment on attachment 8663459 [details] [diff] [review]
Patch v1

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

Thanks, I haven't yet run with this but it looks nice. I have some comments.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2998,5 @@
> +{
> +  var toolbar = document.getElementById("FormatToolbar");
> +  var format_menubar = document.getElementById("formatMenu");
> +  var insert_menubar = document.getElementById("insertMenu");
> +  var show_menuitem = document.getElementById("menu_showFormatToolbar");

Use lets.

@@ +3016,5 @@
> +    aDeliveryFormat == nsIMsgCompSendFormat.PlainText);
> +  document.getElementById("format_html").setAttribute("checked",
> +    aDeliveryFormat == nsIMsgCompSendFormat.HTML);
> +  document.getElementById("format_both").setAttribute("checked",
> +    aDeliveryFormat == nsIMsgCompSendFormat.Both);

Maybe a switch would be more readable, as in the OutputFormatMenuSelect function. Or I would make 2 arrays for the ID and nsIMsgCompSendFormat values at the matching indexes and use this to map between the 2:)

::: mailnews/compose/public/nsIMsgCompFields.idl
@@ +51,5 @@
>    /// Status of manually-activated attachment reminder.
>    attribute boolean attachmentReminder;
> +  /// Delivery format for the mail being composed
> +  /// (auto-4, text-1, html-2, text and html-3).
> +  attribute long deliveryFormat;

Maybe try = instead of - .

::: mailnews/compose/src/nsMsgCompFields.cpp
@@ +72,5 @@
>    m_bodyIsAsciiOnly = false;
>    m_forceMsgEncoding = false;
>    m_needToCheckCharset = true;
>    m_attachmentReminder = false;
> +  m_deliveryFormat = 4;

Can you access the nsIMsgCompSendFormat constant by name here?

::: mailnews/mime/src/mimedrft.cpp
@@ +1282,5 @@
>        else
>          fields->SetAttachmentReminder(false);
>        PR_FREEIF(parm);
> +      parm = MimeHeaders_get_parameter(draftInfo, "deliveryformat", NULL, NULL);
> +      if (parm) {

What happens if the parameter is not in the saved draft (think age old message saved before this patch)? Will the UI be initialized to the proper default value? Will it get the default of 4 from the CompFields?

@@ +1285,5 @@
> +      parm = MimeHeaders_get_parameter(draftInfo, "deliveryformat", NULL, NULL);
> +      if (parm) {
> +        int deliveryFormat = 4;
> +        sscanf(parm, "%d", &deliveryFormat);
> +        fields->SetDeliveryFormat((int32_t)deliveryFormat);

Does the deliveryFormat variable need to be int or can it be int32_t directly? Also, can you use the constant by name instead of 4?

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.5

Made the suggested changes, thanks.

Comment 8

[:aceman]

Comment on attachment 8663910 [details] [diff] [review]
Patch v1.5

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

I have now run with this and it works great. The format value is preserved in the draft and upon reopening the draft the UI is set up properly based on the format value.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3017,5 @@
> +                      nsIMsgCompSendFormat.PlainText,
> +                      nsIMsgCompSendFormat.HTML,
> +                      nsIMsgCompSendFormat.Both ];
> +
> +  for (let format in deliveryFormats) {

I imagined you could use deliveryFormats.indexOf(aDeliveryFormat) here, no for loop.

@@ +3040,5 @@
>        {
>          case "format_auto":  gSendFormat = nsIMsgCompSendFormat.AskUser;     break;
>          case "format_plain": gSendFormat = nsIMsgCompSendFormat.PlainText;   break;
>          case "format_html":  gSendFormat = nsIMsgCompSendFormat.HTML;        break;
>          case "format_both":  gSendFormat = nsIMsgCompSendFormat.Both;        break;

I'd thought you could use the 2 arrays for this block now (using indexOf again). I'm not sure why we even need to use the 'id' here, why value attribute would be not enough containing the nsIMsgCompSendFormat constant directly. We can file that as a new bug.

::: mailnews/compose/src/nsMsgCompFields.cpp
@@ +409,5 @@
>  
> +NS_IMETHODIMP nsMsgCompFields::SetDeliveryFormat(int32_t value)
> +{
> +  m_deliveryFormat = value;
> +  return NS_OK;

I wonder where would be a good place to check if the 'value' is withing range. A 'malicious draft' could contain deliveryformat=12312312 and then we have an unknown value. I hope the mailnews reviewer can say if it should be here, or at the spot where you read in the draft (in mimedrft.cpp).

Comment 9

[Jorg K (CEST = GMT+2)]

No progress here? f+ and now, what's next??

Comment 10

[Suyash Agarwal (:sshagarwal)]

(In reply to Jorg K (GMT+1) [currently frustrated by waiting for reviews/feedback] from comment #9)
> No progress here? f+ and now, what's next??

Sorry, give me this Sunday.

Comment 11

[Jorg K (CEST = GMT+2)]

Aceman, do you think we can finish off this bug one day. I wanted it in TB 45, but we well and truly missed that.

Comment 12

[:aceman]

Yes, I talked to Suyash recently and he is alive and will look at this in these days.

Actually I think we can collect some reviews now. We experimented with solutions to my comment 8 but none was very elegant. So maybe it can stay as is for now. I said already that we can polish that later in a new bug.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8663910 [details] [diff] [review]
Patch v1.5

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3024,5 @@
> +              .setAttribute("checked", "true");
> +    else
> +      document.getElementById(deliveryFormats[format])
> +              .removeAttribute("checked");
> +  }

I don't understand why you need to remove the checkmark. This is configured as a proper radio button, setting one entry to checked will clear the checkmark on any other entry.
So I'd put a break after the setting and remove the else branch.

Or what's wrong with Aceman's suggestion:
let index = sendFormats.indexOf(aDeliveryFormat);
document.getElementById(deliveryFormats[index]).setAttribute("checked", "true");

Or a case statement as below.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: Patch v1.6

I prefer a simple case statement:

function SelectDeliveryFormatMenuOption(aDeliveryFormat)
{
  let id;
  switch (aDeliveryFormat)
  {
    case nsIMsgCompSendFormat.AskUser:   id = "format_auto";  break;
    case nsIMsgCompSendFormat.PlainText: id = "format_plain"; break;
    case nsIMsgCompSendFormat.HTML:      id = "format_html";  break;
    case nsIMsgCompSendFormat.Both:      id = "format_both";  break;
  }
  document.getElementById(id).setAttribute("checked", "true");
}

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: Issue

Hi jorgk, this is what happens if I am not unchecking the checked ones. I know it's radio and it's supposed to behave that way. For some reason, I think I am unable to reach the required element. Will try to do it.

Thanks.

Comment 16

[Jorg K (CEST = GMT+2)]

Hmm, I tested my variation of your patch before attaching it. I don't see two radio buttons checked at the same time.

Comment 17

[Suyash Agarwal (:sshagarwal)]

(In reply to Jorg K (GMT+1) from comment #16)
> Hmm, I tested my variation of your patch before attaching it. I don't see
> two radio buttons checked at the same time.

This is easily reproducible for me. I just chose a delivery format, saved as draft, restarted TB and voila! we have 2 checks.

Comment 18

[Jorg K (CEST = GMT+2)]

OK, with my patch applied I created a new message, set the format to "plain+rich", saved the draft. Looked at the content, saw: deliveryformat=3. Closed down TB. Started it again. Edited the draft, looked at the delivery format: Plain+rich is selected, nothing else. Technically the radio button cannot select two different items unless they are in different groups. I'm on Windows. Do I need to start my Linux box to check it? Or are you on Mac?

Sure, in the XUL definition the auto option is checked, but the code sets something else and that should clear the initial selection. Hard to understand. Aceman, can you please try my (superseded) patch?

Comment 19

[Jorg K (CEST = GMT+2)]

I know for a fact that the "Format > Font" menu only clears the checkmark if it doesn't set another one.
If it sets on, the loop breaks:
https://dxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#894
So you should be able to select a font starting with C and then one with A and the one with C shouldn't get cleared.

Comment 20

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

This works for me.

Thanks.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8711516 [details] [diff] [review]
Patch v2

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

I've already looked at it and tested it last night. It works and looks good, so I'm giving this an r+.
I'm still curious to know why this
- <menuitem type="radio" name="output_format" ... id="format_auto" checked="true"/>
+ <menuitem type="radio" name="output_format" ... id="format_auto"/>
was necessary.

Comment 22

[:aceman]

Comment on attachment 8711516 [details] [diff] [review]
Patch v2

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

Looks good to me now, thanks.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2970,5 @@
> +  let toolbar = document.getElementById("FormatToolbar");
> +  let format_menubar = document.getElementById("formatMenu");
> +  let insert_menubar = document.getElementById("insertMenu");
> +  let show_menuitem = document.getElementById("menu_showFormatToolbar");
> +  gHideMenus = (gSendFormat == nsIMsgCompSendFormat.PlainText);

Should this gSendFormat be aDeliveryFormat ? Otherwise you do not use the passed in argument anywhere.

@@ +3004,3 @@
>  function OutputFormatMenuSelect(target)
>  {
> +  var currentSendFormat = gSendFormat;

Use let.

@@ +3009,1 @@
>      var msgCompFields = gMsgCompose.compFields;

Use let.

@@ +3009,3 @@
>      var msgCompFields = gMsgCompose.compFields;
> +    if (msgCompFields) {
> +      switch (target.getAttribute('id')) {

Double quotes.

::: mailnews/compose/src/nsMsgCompFields.cpp
@@ +422,5 @@
> +
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP nsMsgCompFields::GetDeliveryFormat(int32_t *_retval)

Please check if the argument is not null, NS_ENSURE_ARG_POINTER(_retval)

Comment 23

[:aceman]

(In reply to Jorg K (GMT+1) from comment #21)
> I've already looked at it and tested it last night. It works and looks good,
> so I'm giving this an r+.
> I'm still curious to know why this
> - <menuitem type="radio" name="output_format" ... id="format_auto"
> checked="true"/>
> + <menuitem type="radio" name="output_format" ... id="format_auto"/>
> was necessary.

I'd be OK with this change. If the menuitem would be the authoritative single point determining the current value, then yes, initialize it to "auto". But when we actually use the gSendFormat variable to keep the state, let's not show the UI something else while gSendFormat has a different value (the variable is first declared without a value. It is then initialized in ComposeStartup, where also the menuitem is checked.

I wonder if we then should also set up the UI here where gSendFormat is also set:
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#124.

Comment 24

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #17)
> (In reply to Jorg K (GMT+1) from comment #16)
> > Hmm, I tested my variation of your patch before attaching it. I don't see
> > two radio buttons checked at the same time.
> 
> This is easily reproducible for me. I just chose a delivery format, saved as
> draft, restarted TB and voila! we have 2 checks.

I'll test the latest patch to see if this doesn't happen. Maybe it is OS X specific?

Comment 25

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.1

Made the suggested changes.

Comment 26

[Jorg K (CEST = GMT+2)]

Thanks. I'm happy to see that this is finally moving forward. I don't think we need Joshua's review here, since the mailnews changes are minimal. I'd be happy to uplift this so we can get it into TB 45 beta and release.

Comment 27

[:aceman]

(In reply to :aceman from comment #23)
> I wonder if we then should also set up the UI here where gSendFormat is also
> set:
> http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/
> MsgComposeCommands.js#124.

What about this?

Comment 28

[Jorg K (CEST = GMT+2)]

IMHO setting it in ComposeStartup() is sufficient. I don't see that you could update the UI in InitializeGlobalVariables(). I believe that the currently proposed patch is fine.

Comment 29

[:aceman]

OK.
I have also tested it now and I do not see the duplicate checkmark (on linux). It actually a real "radio" widget, with circles and dots, not checkmarks.

Comment 30

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8711637 [details] [diff] [review]
Patch v2.1

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

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +349,5 @@
> +    draftInfo.AppendLiteral("; ");
> +    int32_t deliveryFormat;
> +    fields->GetDeliveryFormat(&deliveryFormat);
> +    draftInfo.AppendLiteral("deliveryformat=");
> +    draftInfo.AppendInt(deliveryFormat);

If you're changing *anything* in this method, you need to update test_messageHeaders.js to include the new headers being added, particularly for all the different possible values. I'd be surprised if this passed tests at all, in fact.

Comment 31

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.1 + test

Tests pass now. I've also added a case where we change the deliveryFormat.

Thanks.

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 8711760 [details] [diff] [review]
Patch v2.1 + test

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

(Don't think my review is needed here)

Comment 33

[:aceman]

The try run seems fine with xpcshell:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=826e7a07950b

Just my added mozmill test fails, again due to not being able to click after SaveAsDraft, as in the identity tests (bug 1238264).

Comment 34

[Jorg K (CEST = GMT+2)]

I foolishly set tracking-thunderbird45 to + in comment #26 not keeping in mind the interface change. So is this important enough to invest extra work required for the interface change? Maybe not.

Comment 35

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+1) from comment #14)
> Created attachment 8711485 [details] [diff] [review]
> Patch v1.6
> 
> I prefer a simple case statement:
> 
> function SelectDeliveryFormatMenuOption(aDeliveryFormat)
> {
>   let id;
>   switch (aDeliveryFormat)
>   {
>     case nsIMsgCompSendFormat.AskUser:   id = "format_auto";  break;

Just my 2 cents, but from all I remember .AskUser actually effects literally that: user will be asked HTML mail question dialogue and can choose a format from there. Which does not seem the same as format_auto because it's not automatical. So I suspect at least the wording needs improvement (to avoid future confusions), or maybe we're not correct in behaviour about this case.

>     case nsIMsgCompSendFormat.PlainText: id = "format_plain"; break;
>     case nsIMsgCompSendFormat.HTML:      id = "format_html";  break;
>     case nsIMsgCompSendFormat.Both:      id = "format_both";  break;
>   }
>   document.getElementById(id).setAttribute("checked", "true");
> }

Comment 36

[:aceman]

Comment on attachment 8711760 [details] [diff] [review]
Patch v2.1 + test

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

I think this is done now. The xpcshells pass (seen on try). jcranmer, you can update your review now.
I'll upload the mozmill test as a separate attachment now.

::: mailnews/compose/test/unit/test_messageHeaders.js
@@ +193,5 @@
>      "X-Mozilla-Draft-Info": "internal/draft; " +
> +      "vcard=1; receipt=1; DSN=1; uuencode=0; attachmentreminder=1; deliveryformat=4",
> +  });
> +
> +  fields.deliveryFormat = 3;

Use Components.interfaces.nsIMsgCompSendFormat.Both here instead of 3.

Comment 37

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #36)
> I'll upload the mozmill test as a separate attachment now.
"Now" being when exactly ;-) And does it have a problem as per comment #33?

Comment 38

[:aceman]

Attachment: mozmill test

I have solved the problem from comment 33, at least I hope so, but the try server is so broken that I can't prove/test it. If you can check it locally, then you could review it. There are different paths for OS X and the other platforms.

Comment 39

[:aceman]

Both tests pass at least on linux: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c6fcb22d0a0c

The mozmill one passed on linux since the beginning, but not on the other platforms.

Comment 40

[Jorg K (CEST = GMT+2)]

Comment on attachment 8713981 [details] [diff] [review]
mozmill test

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

I ran this on Windows and it worked just fine. However, we know that running things manually and on the try servers can (sadly) give different results.
The r+ is only given under the condition that you provide a green try run for Mac and Windows. I have no possibility to check the Mac stuff without seeing a try run. How did you implement it? Did Suyash run it for you?

Two ignorant questions:
How do these mozmill tests work? They run every top level function in the file whose name starts with test?
Why is assert_format_value() embedded in test_save_composition_properties() and not at the same level as setupComposeWin()?

If you provide a new patch, would you mind using r=jorgk instead of JorgK. Sorry about the confusion. All approvals I've done so far I've done with a=jorgk.

::: mail/test/mozmill/composition/test-drafts.js
@@ +90,5 @@
>    press_delete(mc); // clean up after ourselves
>  }
>  
> +/**
> + * Tests that needed settings of the compose window are stored in a draft message.

Nit: Actually, you're only checking the delivery format, so please change this comment. In the future I can imagine storing other things, like for example the chosen dictionary language. The attachment reminder is also tested elsewhere.

@@ +92,5 @@
>  
> +/**
> + * Tests that needed settings of the compose window are stored in a draft message.
> + */
> +function test_save_composition_properties() {

Same here. Or are we planning to add more checking here later? For the time being that could be called test_save_delivery_format.

Comment 41

[:aceman]

(In reply to Jorg K (GMT+1) from comment #40)
> I ran this on Windows and it worked just fine. However, we know that running
> things manually and on the try servers can (sadly) give different results.
> The r+ is only given under the condition that you provide a green try run
> for Mac and Windows.
Yes, it seems Windows is up again, so here is new try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5cf962f0aeab

> I have no possibility to check the Mac stuff without
> seeing a try run. How did you implement it? Did Suyash run it for you?
I implemented and tested the OS X code locally, because it also works on other platforms. I just don't want to use it on other platforms as it is not the correct way to do it. I just do it on OS X as there mozmill can't operate the main menu.

> Two ignorant questions:
> How do these mozmill tests work? They run every top level function in the
> file whose name starts with test?
Yes.

> Why is assert_format_value() embedded in test_save_composition_properties()
> and not at the same level as setupComposeWin()?
Because it is only used in that single test :)
I plan to move setupComposeWin to a shared module as it is duplicated co that also other tests use.

> If you provide a new patch, would you mind using r=jorgk instead of JorgK.
> Sorry about the confusion. All approvals I've done so far I've done with
> a=jorgk.
Ok.

> ::: mail/test/mozmill/composition/test-drafts.js
> @@ +90,5 @@
> >    press_delete(mc); // clean up after ourselves
> >  }
> >  
> > +/**
> > + * Tests that needed settings of the compose window are stored in a draft message.
> 
> Nit: Actually, you're only checking the delivery format, so please change
> this comment. In the future I can imagine storing other things, like for
> example the chosen dictionary language. The attachment reminder is also
> tested elsewhere.
Yeah, I made it too universal for possible future use ;)

> @@ +92,5 @@
> >  
> > +/**
> > + * Tests that needed settings of the compose window are stored in a draft message.
> > + */
> > +function test_save_composition_properties() {
> 
> Same here. Or are we planning to add more checking here later? For the time
> being that could be called test_save_delivery_format.
OK:)

Comment 42

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5cf962f0aeab
worked. What about Mac, that sadly failed to build.

Given that this test seems to work, can you fix bug 1238264 too ;-)

Comment 43

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.2

Made the suggested change in the test.

Comment 44

[Jorg K (CEST = GMT+2)]

The way this is going, it doesn't make sense to track it. The bug needs an interface change, so it's too late for TB 45 unless someone wants to do the extra work.

Comment 45

[Kent James (:rkent)]

(In reply to Jorg K (GMT+1) from comment #44)
> The way this is going, it doesn't make sense to track it. The bug needs an
> interface change, so it's too late for TB 45 unless someone wants to do the
> extra work.

The interface change adds a new attribute. This can be ported to TB45 if desired by adding a new 45-specific interface that only includes the new attribute, then QI'ing the pointer to that interface when you need to operate on the attribute.

Comment 46

[:aceman]

We do not even have a mailnews review here :(

Anyway I do not know how to do that interface QI game so unless somebody does, it will not be done.

Comment 47

[Kent James (:rkent)]

It's not hard, I can do the porting.

I'm not saying that we SHOULD do it for this bug, my point was that a simple interface change does not preclude successfully adapting the bug for a post-freeze release landing.

Comment 48

[Jorg K (CEST = GMT+2)]

Kent already explained it here: Bug 597369 comment #60. IMHO it's not worth doing.

Comment 49

[Kent James (:rkent)]

Comment on attachment 8714103 [details] [diff] [review]
Patch v2.2

Stealing this review to get it moving. Looked it all over, LGTM, jcranmer comments addressed, so r+

Comment 50

[Kent James (:rkent)]

fix: http://hg.mozilla.org/comm-central/rev/91460cf3f516
test: http://hg.mozilla.org/comm-central/rev/87800d242806

Comment 51

[Jorg K (CEST = GMT+2)]

Thanks a lot, Kent, but you were a little to eager here.

The test has r+ but there were some nits to be fixed before this should have landed, see comment #40.

Comment 52

[:aceman]

Correct and I didn't upload the fixed version yet as the build servers had broken Windows builds etc. I'll make a new try run right now.

Comment 53

[Kent James (:rkent)]

OK, sorry about that over eagerness. I thought they were addressed in comment 43, but I suppose that was for the other patch.

Comment 54

[:aceman]

Attachment: mozmill test v1.1

Here is the fixed mozmill test. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7c8cfd9be232 

Please back out the old version and check in this one when appropriate.

Comment 55

[Jorg K (CEST = GMT+2)]

(Sorry about the spelling mistake in comment #51: too eager)

Backout:
https://hg.mozilla.org/comm-central/rev/a26c5cd42b0c

So landed are:
fix:  https://hg.mozilla.org/comm-central/rev/91460cf3f516
test: https://hg.mozilla.org/comm-central/rev/296187f1ff37

DONTBUILD since this only changed comment and a function name.

Comment 56

[Jorg K (CEST = GMT+2)]

Looks like TB 45 and TB 46 are "wontfix", due to the interface change and due to the regression in bug 1249693.