Simplify code and fix comments in class nsDelAttachListener in nsMessenger.cpp
Categories
(MailNews Core :: Backend, task)
Tracking
(thunderbird_esr78 wontfix)
| Tracking | Status | |
|---|---|---|
| thunderbird_esr78 | --- | wontfix |
People
(Reporter: tom, Assigned: tom)
Details
Attachments
(3 files, 3 obsolete files)
|
6.42 KB,
patch
|
benc
:
review+
mkmelin
:
feedback+
|
Details | Diff | Splinter Review |
|
6.73 KB,
patch
|
Details | Diff | Splinter Review | |
|
5.60 KB,
patch
|
Details | Diff | Splinter Review |
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.
This patch just adds debug so you can see how it worked before the changes we're suggesting.
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•4 years ago
|
||
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.
Rebased after bug 1612239.
Corrected wrong debug output.
|
|
||
Comment 8•4 years ago
|
||
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•4 years ago
|
||
Comment on attachment 9191380 [details] [diff] [review] 1680795-nsDelAttachListener.patch Review of attachment 9191380 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, will run clang-format before landing.
|
|
||
Updated•4 years ago
|
|
Assignee | |
Comment 10•4 years ago
|
||
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®exp=true
Certainly if you prefer, we can add the braces.
|
||
Comment 11•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/57839111adf1
Simplify code in nsDelAttachListener and correct comments. r=benc
Description
•