Closed Bug 1430766 Opened 7 years ago Closed 6 years ago

Remove unused code from msgMapiHook.cpp

Categories

(MailNews Core :: Simple MAPI, defect)

Product:
MailNews Core
Component:
Simple MAPI
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: just, Assigned: jorgk-bmo)

Details

Attachments

(2 files, 2 obsolete files)

Bug_1430766-removed_unused_code_from_msgMapiHook.patch (v2)
(deleted), patch
Details | Diff | Splinter Review
Bug_1430766-removed_unused_code_from_msgMapiHook.patch (v3b) - less drastic
(deleted), patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20171128222554 Steps to reproduce: checked code line 330ff Actual results: saw some code after the "return" command. Expected results: expected no code after "return" command.
proposed patch - I did not try to build with it yet
Attachment #8942897 - Flags: review?
Comment on attachment 8942897 [details] [diff] [review] Bug_1430766-removed_unused_code_from_msgMapiHook.patch Hi again, you need to pick a reviewer, otherwise no one pays attention. Aceman, what do you make of this funny code? Incredible. Did they mean rv = pMsgCompose->SendMsg(...); Banner/Bienvenu touched that code and didn't notice anything?? https://hg.mozilla.org/comm-central/rev/ee9a4fdb8c27#l1.30
Attachment #8942897 - Flags: review? → review?(acelists)
Comment on attachment 8942897 [details] [diff] [review] Bug_1430766-removed_unused_code_from_msgMapiHook.patch Review of attachment 8942897 [details] [diff] [review]: ----------------------------------------------------------------- I can't judge what this code does, as I do not have any MAPI. The return is there since the initial import into hg in https://hg.mozilla.org/comm-central/rev/e4f4569d451a. There is a comment above: /** initialize nsIMsgCompose, Send the message, wait for send completion response **/ So it seem the code below pMsgCompose->SendMsg() you are removing does just that. Maybe it should rather be rv = pMsgCompose->SendMsg(...) ?
Attachment #8942897 - Flags: review?(acelists) → review-
Assignee: nobody → just
Severity: normal → minor
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

At least then the comment mentioned by :aceman should be edited as well?

Mike, what do you make of this nonsense? Spinning the event loop and blocking the app is never a good idea. Should we take the patch and remove the dead code ... and fix the comment?

Flags: needinfo?(mikekaganski)
Comment on attachment 8942897 [details] [diff] [review] Bug_1430766-removed_unused_code_from_msgMapiHook.patch OK, whatever the story was here, there is no discussion that this is dead (and dangerous code). So we should removed it and fix the comment.
Attachment #8942897 - Flags: review- → review+
Comment on attachment 8942897 [details] [diff] [review] Bug_1430766-removed_unused_code_from_msgMapiHook.patch Actually, looking at this, this removes the only call site of `IsDone()` and therefore the necessity of the entire send listener nonsense which is totally dead code in nsIMsgComposeParams.idl and nsMsgComposeParams.cpp. I'll do a more drastic patch.
Flags: needinfo?(mikekaganski)
Attachment #8942897 - Flags: review+

I'm hijacking this bug.

Aceman, you'll hate this, but you can be my reviewer ;-) - You'll get support from some people providing feedback.

Mike, can you test this patch and see that it doesn't have negative side effects.

Kai, the PR_C* calls are NSPR, yes? Why on earth were those necessary? The net effect of the code is that m_done was being set that no one ever looked at anyway, right? Am I missing something?

There is plenty of send listener code in nsMsgCompose.cpp, but this appears to be the only use of send listener code for nsIMsgComposeParams.

A less drastic patch would be to leave that unused functionality in place, in case some add-on uses it. I'll add that, too.

Assignee: just → jorgk
Attachment #8942897 - Attachment is obsolete: true
Attachment #9037844 - Flags: review?(acelists)
Attachment #9037844 - Flags: feedback?(mikekaganski)
Attachment #9037844 - Flags: feedback?(kaie)

OK, this only changes MAPI code. As a peer I have the right to pick a non-peer reviewer ;-) - This can be Mike since no "mainstream" TB code is affected.

Kai, could you also take a glance at this please to check the removal of the NSPR stuff.

Attachment #9037845 - Flags: review?(mikekaganski)
Attachment #9037845 - Flags: feedback?(kaie)

Oops, sorry, forgot to fix that comment we talked about. Sorry about the noise. Maybe we don't go with (v2), so I didn't update that.

Attachment #9037845 - Attachment is obsolete: true
Attachment #9037845 - Flags: review?(mikekaganski)
Attachment #9037845 - Flags: feedback?(kaie)
Attachment #9037846 - Flags: review?(mikekaganski)
Attachment #9037846 - Flags: feedback?(kaie)
Attachment #9037844 - Flags: review?(acelists)
Attachment #9037844 - Flags: feedback?(mikekaganski)
Attachment #9037844 - Flags: feedback?(kaie)
Attachment #9037846 - Flags: review?(mikekaganski)
Attachment #9037846 - Flags: feedback?(kaie)

OK, the funny code got introduced in bug 108275 attachment 147428 [details] [diff] [review]. They really shot themselves in the foot by adding code that never ran.

Further research shows that the send listener will be used in bug 547027 attachment 791889 [details] [diff] [review], so let's all head over there and get that patch landed :-)

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

Attachment

General

Creator:
Created:
Updated:
Size: