1680795 - Simplify code and fix comments in class nsDelAttachListener in nsMessenger.cpp

Status: RESOLVED FIXED

(MailNews Core :: Backend, task)

Description

[Tom]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

While looking at the code in class nsDelAttachListener in nsMessenger.cpp for bug 1676718, we noticed some room for improvement. We'll attach our patch soon.

Comment 1

[Tom]

Attachment: 1680795-nsDelAttachListener.patch

Here is our patch, we removed some dead code, simplified code and corrected/removed comments.

Comment 2

[Tom]

Attachment: debug-before-patch.patch

This patch just adds debug so you can see how it worked before the changes we're suggesting.

Comment 3

[Tom]

Attachment: debug-after-patch.patch

This patch also adds debug so you can see how it is working with the changes we're suggesting (to be applied after 1680795-nsDelAttachListener.patch).

Comment 4

[Ben Campbell]

The patch looks pretty sensible to me in isolation, but I need to spend a little time to dig about and understand it in context. I'll do that and give it a proper review over the next day or two, so apologies for the delay!

Comment 5

[Tom]

Attachment: debug-before-patch.patch

Rebased after bug 1612239.

Comment 6

[Tom]

Attachment: debug-after-patch.patch

Rebased after bug 1612239.

Comment 7

[Tom]

Attachment: debug-after-patch.patch

Corrected wrong debug output.

Comment 8

[Ben Campbell]

Comment on attachment 9191380 [details] [diff] [review]
1680795-nsDelAttachListener.patch

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

Sorry about the delay, but it looks good to me - always so nice seeing things simplified, corrected and clarified!

try run looks clean:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=67e53a6a37792183821057a028dd7359590ba2a6

Raising a feedback flag for a quick second opinion from Magnus: have I missed anything obvious? I still don't really know my way around nsMessenger.

::: mailnews/base/src/nsMessenger.cpp
@@ +2358,5 @@
>  
>  NS_IMETHODIMP
>  nsDelAttachListener::OnStopRunningUrl(nsIURI* aUrl, nsresult aExitCode) {
>    nsresult rv = NS_OK;
> +  if (mOriginalMessage && m_state == eUpdatingFolder) rv = DeleteOriginalMessage();

linting nit: move the "rv = ..". onto it's own line.
(I prefer using surrounding braces even for one-line conditionals, but I'm pretty sure the coding standard is happy either way. So whatever your preference is, is fine)

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9191380 [details] [diff] [review]
1680795-nsDelAttachListener.patch

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

LGTM, will run clang-format before landing.

Comment 10

[Tom]

Thank you for the review. Did you check the original behaviour with the "debug-before-patch.patch" to see how confused it is? It sets eUpdatingFolder even for local folders and only this line
https://searchfox.org/comm-central/rev/b97629284ff9f582b11ab6b7a983356730ddb850/mailnews/base/src/nsMessenger.cpp#2332
prevents an endless recursion since DeleteOriginalMessage() is potentially called again via the DeleteMessages() OnStopCopy() callback. The patch doesn't change any behaviour, it only improves the clarity of the code.

As for the nit, there are many other "bad" examples here:
https://searchfox.org/comm-central/search?q=if+%5C%28.*%5C%29.*+%3D+.*&path=.cpp&case=false&regexp=true
Certainly if you prefer, we can add the braces.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/57839111adf1
Simplify code in nsDelAttachListener and correct comments. r=benc