Closed Bug 1680795 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: Backend, task)

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
86 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: tom, Assigned: tom)

Details

Attachments

(3 files, 3 obsolete files)

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.

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

Attachment #9191380 - Flags: review?(benc)
Attached patch debug-before-patch.patch (obsolete) — Splinter Review

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

Attached patch debug-after-patch.patch (obsolete) — Splinter Review

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).

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!

Rebased after bug 1612239.

Attachment #9191383 - Attachment is obsolete: true
Attached patch debug-after-patch.patch (obsolete) — Splinter Review

Rebased after bug 1612239.

Attachment #9191385 - Attachment is obsolete: true

Corrected wrong debug output.

Attachment #9191600 - Attachment is obsolete: true
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)
Attachment #9191380 - Flags: review?(benc)
Attachment #9191380 - Flags: review+
Attachment #9191380 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9191380 [details] [diff] [review] 1680795-nsDelAttachListener.patch Review of attachment 9191380 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, will run clang-format before landing.
Attachment #9191380 - Flags: feedback?(mkmelin+mozilla) → feedback+
Assignee: nobody → tom
Status: UNCONFIRMED → ASSIGNED
Type: enhancement → task
Ever confirmed: true
Target Milestone: --- → 86 Branch

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.

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/57839111adf1
Simplify code in nsDelAttachListener and correct comments. r=benc

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: