834028 - Wrong parameter name for fEAlert method in nsIImapServerSink interface

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[rsx11m]

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

Comment 1

[rsx11m]

Attachment: 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.

Comment 2

[rsx11m]

Oh, and I didn't rev the UUID given that the type of the parameter is unchanged.

Comment 3

[rsx11m]

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?

Comment 4

[rsx11m]

Attachment: Fix all occurences

Let's make this consistent, also fixes the FEAlertFromServer parameter.

Comment 5

[[e-mail]]

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.

Comment 6

[rsx11m]

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.

Comment 7

[rsx11m]

(In reply to [e-mail] 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

[[e-mail]]

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.

Comment 9

[rsx11m]

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.

Comment 10

[[e-mail]]

(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 ;-)

Comment 11

[Magnus Melin [:mkmelin]]

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.

Comment 12

[[e-mail]]

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

OK, so as this was just an oversight by mkmelin, then r=me.

Comment 13

[rsx11m]

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.

Comment 14

[rsx11m]

Push for trunk/comm-central, please.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/7301f1cf35e0