Last Comment Bug 681219 - make method name of nsISmtpService decent
: make method name of nsISmtpService decent
Status: RESOLVED FIXED
: addon-compat
Product: MailNews Core
Classification: Components
Component: Networking: SMTP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 22.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-23 03:27 PDT by Hiroyuki Ikezoe (:hiro)
Modified: 2013-03-12 12:30 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed change (2.46 KB, patch)
2011-08-23 03:27 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
Revised patch (26.38 KB, patch)
2012-01-30 02:11 PST, Hiroyuki Ikezoe (:hiro)
standard8: review-
Details | Diff | Splinter Review
Revised patch (26.58 KB, patch)
2012-02-21 15:11 PST, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
Change the uuid too (27.34 KB, patch)
2012-02-21 15:47 PST, Hiroyuki Ikezoe (:hiro)
standard8: review+
mozilla: superreview+
Details | Diff | Splinter Review
unbitrotted patch (29.30 KB, patch)
2013-03-12 11:54 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description Hiroyuki Ikezoe (:hiro) 2011-08-23 03:27:04 PDT
Created attachment 555063 [details] [diff] [review]
Proposed change

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 Ludovic Hirlimann [:Usul] 2011-08-26 07:49:10 PDT
(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 Mark Banner (:standard8) 2011-08-26 11:21:25 PDT
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.
Comment 3 :aceman 2012-01-23 09:03:08 PST
This is a change in an .idl. Shouldn't there be corresponding changes in some .cpp file?

Is :hiro working on this?
Comment 4 Hiroyuki Ikezoe (:hiro) 2012-01-23 19:36:36 PST
Yes, I'm still working on this.
Comment 5 Hiroyuki Ikezoe (:hiro) 2012-01-30 02:11:53 PST
Created attachment 592640 [details] [diff] [review]
Revised patch

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
Comment 6 Mark Banner (:standard8) 2012-02-21 02:52:38 PST
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.
Comment 7 Hiroyuki Ikezoe (:hiro) 2012-02-21 15:11:16 PST
Created attachment 599383 [details] [diff] [review]
Revised patch

* Add a trivial documentation of parameters of getServerByIdentity
* Remove space nits.
Comment 8 Hiroyuki Ikezoe (:hiro) 2012-02-21 15:40:22 PST
Comment on attachment 599383 [details] [diff] [review]
Revised patch

Forgot to change the uuid.
Comment 9 Hiroyuki Ikezoe (:hiro) 2012-02-21 15:47:09 PST
Created attachment 599395 [details] [diff] [review]
Change the uuid too
Comment 10 Mark Banner (:standard8) 2012-02-23 14:20:41 PST
Comment on attachment 599395 [details] [diff] [review]
Change the uuid too

Review of attachment 599395 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
Comment 11 Mark Banner (:standard8) 2012-02-24 02:36:08 PST
Comment on attachment 599395 [details] [diff] [review]
Change the uuid too

Oh sorry, technically this is an interface change and hence requires sr.
Comment 12 David :Bienvenu 2012-02-24 08:55:41 PST
Comment on attachment 599395 [details] [diff] [review]
Change the uuid too

interface changes look reasonable
Comment 13 :aceman 2013-03-03 04:48:44 PST
Is there anything to be done here? It has all the required reviews. checkin-needed?
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-03-12 05:42:23 PDT
This needs rebasing. It doesn't apply to comm-central at all.
Comment 15 Ludovic Hirlimann [:Usul] 2013-03-12 05:45:36 PDT
(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 ?
Comment 16 :aceman 2013-03-12 06:15:22 PDT
Yeah.
Comment 17 :aceman 2013-03-12 11:54:56 PDT
Created attachment 724071 [details] [diff] [review]
unbitrotted patch
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-03-12 12:30:15 PDT
https://hg.mozilla.org/comm-central/rev/fab9e5145cd4

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