1976378 - Stop using nsIMsgMailNewsUrl in message header view

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, task)

Description

[Geoff Lankow (:darktrojan)]

Exchange message URLs aren't nsIMsgMailNewsUrls, so there are errors reported for every message opened, because msgHdrView code uses nsIMsgMailNewsUrl for some things. In this bug I'm going to unpick that, which also involves adding headers in nsIMimeHeaders form to nsIMailChannel, as some things use that form rather than the names/values arrays.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1976378 - Stop using nsIMsgMailNewsUrl in message header view. r=#thunderbird-reviewers

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1976378 - Ignore message loading notifications if they're not for the current message. r=#thunderbird-reviewers

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7c9eb9d408d6
Stop using nsIMsgMailNewsUrl in message header view. r=mkmelin
https://hg.mozilla.org/comm-central/rev/e8008b9941d8
Ignore message loading notifications if they're not for the current message. r=mkmelin

Comment 4

[Kai Engert [:KaiE:]]

Geoff, is it possible to take the "ignore notifications" patch for esr140 (to fix bug 1892876)?

Should it work by itself, or does it depend on the other patch in this bug? Are there any logical dependencies?

(The patch applies cleanly on 140.)

Comment 5

[Kai Engert [:KaiE:]]

I've tested and it fixes bug 1892876 for me.

Please advise which patches we should uplift to 140.

Comment 6

[Geoff Lankow (:darktrojan)]

Comment on attachment 9499375 [details]
Bug 1976378 - Ignore message loading notifications if they're not for the current message. r=#thunderbird-reviewers

Uplift Approval Request

• Please state case for uplift consideration and ensure bug severity is set: Fixes bug 1892876 – I can't comment on the severity of that bug but Kai can. Unlikely to affect much else outside of tests.
• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Daily?: Yes
• Has the fix been verified in Beta?: No
• Needs manual test from QA?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• Does the fix cause any migrations to be skipped?: Yes
• String changes made/needed:

Comment 7

[Geoff Lankow (:darktrojan)]

Yes, we can uplift this, but I want it to spend a reasonable time (at least two weeks) on beta to shake out any possible regressions. It affects every message that loads, so I want to be sure I've not done something silly.

Comment 8

[Kai Engert [:KaiE:]]

Geoff, after testing, do you recommend uplifting both patches to ESR, or just the one notification patch?

Comment 9

[Kai Engert [:KaiE:]]

(In reply to Geoff Lankow (:darktrojan) from comment #6)

• Please state case for uplift consideration and ensure bug severity is set: Fixes bug 1892876 – I can't comment on the severity of that bug but Kai can. Unlikely to affect much else outside of tests.

I think the severity is pretty high.

Showing attachments and status information in the context of the wrong email can be very misleading.

Comment 10

[Geoff Lankow (:darktrojan)]

Just the one. The other patch aimed at future development.

Comment 11

[Corey Bryant]

(In reply to Geoff Lankow (:darktrojan) from comment #7)

Yes, we can uplift this, but I want it to spend a reasonable time (at least two weeks) on beta to shake out any possible regressions. It affects every message that loads, so I want to be sure I've not done something silly.

This would be a good comment to put in the uplift template.

It makes me think we should not uplift this to beta because beta will become our stable monthly release 141.0 next week. Seems like it makes more sense to just let this ride the train.

Comment 12

[Geoff Lankow (:darktrojan)]

Comment on attachment 9499375 [details]
Bug 1976378 - Ignore message loading notifications if they're not for the current message. r=#thunderbird-reviewers

Ah yeah, I didn't think about that. TBH I hadn't really considered the time frame until writing the comment about it. Never mind, this can wait a little bit longer.

Comment 13

[Kai Engert [:KaiE:]]

This is now in Beta. Let's target an uplift to 140 end of August.

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 9499375 [details]
Bug 1976378 - Ignore message loading notifications if they're not for the current message. r=#thunderbird-reviewers

Uplift Approval Request

• Please state case for uplift consideration and ensure bug severity is set: Displaying wrong information to the user
• User impact if declined:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Daily?: Yes
• Has the fix been verified in Beta?: Yes
• Needs manual test from QA?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I could think of a small remaining risk that we'd stop displaying certain information on screen.
• Does the fix cause any migrations to be skipped?: No
• String changes made/needed:

Comment 16

[Corey Bryant]

Comment on attachment 9499375 [details]
Bug 1976378 - Ignore message loading notifications if they're not for the current message. r=#thunderbird-reviewers

[Triage Comment]
Approved for esr140

Please fill in all uplift template fields in the future.

Comment 17

[Stephen King (:sking)]

Thunderbird 140.3.1:
https://hg.mozilla.org/releases/comm-esr140/rev/e9e5eac0e81b723f6b14caf5b4f934b3076b0e25