Closed
Bug 681219
Opened 13 years ago
Closed 11 years ago
make method name of nsISmtpService decent
Categories
(MailNews Core :: Networking: SMTP, defect)
MailNews Core
Networking: SMTP
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 22.0
People
(Reporter: hiro, Assigned: hiro)
Details
(Keywords: addon-compat)
Attachments
(1 file, 4 obsolete files)
29.30 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
Some methods of nsISmtpService have "Smtp" in its name and some other methods does not have "Stmp". I think "Smtp" word is redundant there since the interface has already "Smtp" word. So I'd propose that new methods which has no "Smtp" is added.
Comment 1•13 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0) > Created attachment 555063 [details] [diff] [review] > > So I'd propose that new methods which has no "Smtp" is added. Any reasons you didn't ask for reviews ?
Comment 2•13 years ago
|
||
Comment on attachment 555063 [details] [diff] [review] Proposed change > /** > * Return the SMTP server that is associated with an identity. > */ >+ void GetServerByIdentity(in nsIMsgIdentity aSenderIdentity, >+ out nsISmtpServer aServer); >+ /** >+ * @status DEPRECATED -- use GetServerByIdentity instead. >+ */ > void GetSmtpServerByIdentity(in nsIMsgIdentity aSenderIdentity, > out nsISmtpServer aServer); I'm not sure I really see the point in keeping the old method around for a while, extensions should be able to switch fairly easily. If you really want to do that, then you should mark it with [deprecated] as iirc idl supports that now.
This is a change in an .idl. Shouldn't there be corresponding changes in some .cpp file? Is :hiro working on this?
Version: unspecified → Trunk
Assignee | ||
Comment 4•12 years ago
|
||
Yes, I'm still working on this.
Assignee | ||
Comment 5•12 years ago
|
||
Remove 'DEPRECATED' from previous patch. I pushed this patch to try server to check omission. It seems to be good. http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=70aa19669932 http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=43f1eb900f43
Attachment #555063 -
Attachment is obsolete: true
Attachment #592640 -
Flags: review?(mbanner)
Comment 6•12 years ago
|
||
Comment on attachment 592640 [details] [diff] [review] Revised patch Review of attachment 592640 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, generally looks good, but I'd just like to re-check it after the change requested against the idl file before I give r+. ::: mailnews/compose/public/nsISmtpService.idl @@ +109,5 @@ > > /** > * Return the SMTP server that is associated with an identity. > */ > + void GetServerByIdentity(in nsIMsgIdentity aSenderIdentity, I didn't notice this before, but we should start this function lower-case as is the general standard, so getServerByIdentity. This file will also need a uuid change. Also, would be nice if you add a bit of documentation about the parameters (but not required for r+). ::: mailnews/compose/src/nsSmtpService.cpp @@ +753,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsSmtpService::GetServerByIdentity(nsIMsgIdentity *aSenderIdentity, nsISmtpServer **aSmtpServer) nit, lets wrap that second parameter onto a new line whilst we're here. ::: mailnews/import/eudora/src/nsEudoraMac.cpp @@ +942,5 @@ > return; > } > nsCOMPtr<nsISmtpServer> smtpServer; > > + rv = smtpService->CreateServer( getter_AddRefs( smtpServer)); nit: please drop the space after the open brackets. ::: mailnews/import/eudora/src/nsEudoraWin32.cpp @@ +1007,5 @@ > return; > } > nsCOMPtr<nsISmtpServer> smtpServer; > > + rv = smtpService->CreateServer( getter_AddRefs( smtpServer)); nit: please drop the space after the open brackets. ::: mailnews/import/oexpress/nsOESettings.cpp @@ +854,5 @@ > } > else { > nsCOMPtr<nsISmtpServer> smtpServer; > PRInt32 port; > + rv = smtpService->CreateServer(getter_AddRefs( smtpServer)); nit: please drop the space after the open bracket. ::: mailnews/import/outlook/src/nsOutlookSettings.cpp @@ +537,5 @@ > IMPORT_LOG1( "SMTP server already exists: %s\n", pServer); > return; > } > nsCOMPtr<nsISmtpServer> smtpServer; > + rv = smtpService->CreateServer( getter_AddRefs( smtpServer)); nit: please drop the space after the open brackets.
Attachment #592640 -
Flags: review?(mbanner) → review-
Assignee | ||
Comment 7•12 years ago
|
||
* Add a trivial documentation of parameters of getServerByIdentity * Remove space nits.
Attachment #592640 -
Attachment is obsolete: true
Attachment #599383 -
Flags: review?(mbanner)
Assignee | ||
Comment 8•12 years ago
|
||
Comment on attachment 599383 [details] [diff] [review] Revised patch Forgot to change the uuid.
Attachment #599383 -
Flags: review?(mbanner)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #599383 -
Attachment is obsolete: true
Attachment #599395 -
Flags: review?(mbanner)
Comment 10•12 years ago
|
||
Comment on attachment 599395 [details] [diff] [review] Change the uuid too Review of attachment 599395 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks.
Attachment #599395 -
Flags: review?(mbanner) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 11•12 years ago
|
||
Comment on attachment 599395 [details] [diff] [review] Change the uuid too Oh sorry, technically this is an interface change and hence requires sr.
Attachment #599395 -
Flags: superreview?(dbienvenu)
Updated•12 years ago
|
Keywords: checkin-needed
Comment 12•12 years ago
|
||
Comment on attachment 599395 [details] [diff] [review] Change the uuid too interface changes look reasonable
Attachment #599395 -
Flags: superreview?(dbienvenu) → superreview+
Comment 13•11 years ago
|
||
Is there anything to be done here? It has all the required reviews. checkin-needed?
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
This needs rebasing. It doesn't apply to comm-central at all.
Keywords: checkin-needed
Comment 15•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #14) > This needs rebasing. It doesn't apply to comm-central at all. Aceman could you do that too ?
Whiteboard: patchlove
Comment 16•11 years ago
|
||
Yeah.
Comment 17•11 years ago
|
||
Attachment #599395 -
Attachment is obsolete: true
Attachment #724071 -
Flags: review+
Keywords: addon-compat,
checkin-needed
Whiteboard: patchlove
Comment 18•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/fab9e5145cd4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
You need to log in
before you can comment on or make changes to this bug.
Description
•