Last Comment Bug 834028 - Wrong parameter name for fEAlert method in nsIImapServerSink interface
: Wrong parameter name for fEAlert method in nsIImapServerSink interface
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 21.0
Assigned To: rsx11m
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-23 15:09 PST by rsx11m
Modified: 2013-02-04 17:31 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix typo in name (1.27 KB, patch)
2013-01-23 15:13 PST, rsx11m
neil: review+
Details | Diff | Review
Fix all occurences (3.59 KB, patch)
2013-01-23 17:59 PST, rsx11m
neil: review+
Details | Diff | Review

Description rsx11m 2013-01-23 15:09:37 PST
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 2013-01-23 15:13:30 PST
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.
Comment 2 rsx11m 2013-01-23 15:14:22 PST
Oh, and I didn't rev the UUID given that the type of the parameter is unchanged.
Comment 3 rsx11m 2013-01-23 16:38:41 PST
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 2013-01-23 17:59:18 PST
Created attachment 705681 [details] [diff] [review]
Fix all occurences

Let's make this consistent, also fixes the FEAlertFromServer parameter.
Comment 5 neil@parkwaycc.co.uk 2013-01-24 04:02:39 PST
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 2013-01-24 05:48:36 PST
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 2013-02-04 06:25:22 PST
(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 neil@parkwaycc.co.uk 2013-02-04 08:22:29 PST
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 2013-02-04 08:49:41 PST
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 neil@parkwaycc.co.uk 2013-02-04 08:57:33 PST
(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 2013-02-04 11:42:31 PST
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 neil@parkwaycc.co.uk 2013-02-04 11:48:10 PST
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 2013-02-04 13:57:42 PST
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 2013-02-04 13:59:28 PST
Push for trunk/comm-central, please.
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-02-04 17:31:10 PST
https://hg.mozilla.org/comm-central/rev/7301f1cf35e0

Note You need to log in before you can comment on or make changes to this bug.