The default bug view has changed. See this FAQ.

Wrong parameter name for fEAlert method in nsIImapServerSink interface

RESOLVED FIXED in Thunderbird 21.0

Status

MailNews Core
Networking: IMAP
--
trivial
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rsx11m, Assigned: rsx11m)

Tracking

Trunk
Thunderbird 21.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.59 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
I just saw this by coincidence: There is a typo in the fEAlert() parameter definition for the alert string, which is "aAlertSring" rather than correct "aAlertString" (introduced with bug 137489 renaming that parameter). That's likely non-consequential, but since I ran into it, no reason not to fix it. ;-)
(Assignee)

Comment 1

4 years ago
Created attachment 705600 [details] [diff] [review]
Fix typo in name

These are the two only occurrences of that string. I don't know how to test this as I'm unable to trigger a message, but it compiles and runs without issues.
Attachment #705600 - Flags: review?(neil)
(Assignee)

Comment 2

4 years ago
Oh, and I didn't rev the UUID given that the type of the parameter is unchanged.
(Assignee)

Comment 3

4 years ago
http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#1791 defines the implementation as

nsImapIncomingServer::FEAlert(const nsAString& aString, nsIMsgMailNewsUrl *aUrl

Shouldn't this be "const nsAString& aAlertString" as well?
(Assignee)

Comment 4

4 years ago
Created attachment 705681 [details] [diff] [review]
Fix all occurences

Let's make this consistent, also fixes the FEAlertFromServer parameter.
Attachment #705600 - Attachment is obsolete: true
Attachment #705600 - Flags: review?(neil)
Attachment #705681 - Flags: review?(neil)

Comment 5

4 years ago
Comment on attachment 705600 [details] [diff] [review]
Fix typo in name

I'm going to + this one because I'm not sure the other one is worthwhile.
Attachment #705600 - Attachment is obsolete: false
Attachment #705600 - Flags: review+

Updated

4 years ago
Attachment #705681 - Flags: feedback?(mbanner)
(Assignee)

Comment 6

4 years ago
Ok, let's wait for Mark. I figured that it's better to make the argument lists of the implementations match the prototypes for these cases, to avoid any ambiguity.
(Assignee)

Comment 7

4 years ago
(In reply to neil@parkwaycc.co.uk from comment #5)
> ... I'm not sure the other one is worthwhile.

While Mark seems to be busy hunting down the various bustages, can you elaborate a bit more where you see any problems with the "long version" of the patch?

Maybe it wasn't worth the effort to also fix the implementations, but now I've done it and so it's readily available. Having the arguments match the prototypes helps reading the code when you see it the first time a lot, and we only talk about changing a parameter's name. Thus, I don't see any risk involved.

Comment 8

4 years ago
The first is a typo change, after all if you're searching for both "Alert" and "String" in the same line you'll miss this. The second is more subtle as the word "Alert" is already present on the same line as well as throughout the body of the function and its purpose is therefore more obvious.
(Assignee)

Comment 9

4 years ago
Well, that sure is a point, but then you could argue that the interface definition should consistently use "aString" only as well for fEAlert() and fEAlertFromServer(). The main issue is consistency in the parameter naming between interface and implementation rather than the name itself.

It was my impression that bug 137489 renamed the interface argument on purpose from the generic "aString" to more specific ones, thus I'd tend to correct the implementation rather than reverting that change other than correcting the typo.
(In reply to rsx11m from comment #9)
> It was my impression that bug 137489 renamed the interface argument on
> purpose from the generic "aString" to more specific ones, thus I'd tend to
> correct the implementation rather than reverting that change other than
> correcting the typo.

Well, let's ask the author ;-)
Flags: needinfo?(mkmelin+mozilla)

Comment 11

4 years ago
Yeah, that's correct. I don't think aString is a good name for anything (but the new ones coule perhaps have been better too). Sorry about the typo.
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 705681 [details] [diff] [review]
Fix all occurences

OK, so as this was just an oversight by mkmelin, then r=me.
Attachment #705681 - Flags: review?(neil) → review+
(Assignee)

Comment 13

4 years ago
Comment on attachment 705681 [details] [diff] [review]
Fix all occurences

Thanks, I'm cancelling the feedback request for Mark given the r+ for this one.
Attachment #705681 - Flags: feedback?(mbanner)
(Assignee)

Updated

4 years ago
Attachment #705600 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Push for trunk/comm-central, please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
https://hg.mozilla.org/comm-central/rev/7301f1cf35e0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → Thunderbird 21.0
You need to log in before you can comment on or make changes to this bug.