681219 - make method name of nsISmtpService decent

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

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

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

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]

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)]

Yes, I'm still working on this.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: 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)]

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)]

Attachment: Revised patch

* Add a trivial documentation of parameters of getServerByIdentity
* Remove space nits.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 599383 [details] [diff] [review]
Revised patch

Forgot to change the uuid.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Change the uuid too

Comment 10

[Mark Banner (:standard8)]

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)]

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]

Comment on attachment 599395 [details] [diff] [review]
Change the uuid too

interface changes look reasonable

Comment 13

[:aceman]

Is there anything to be done here? It has all the required reviews. checkin-needed?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing. It doesn't apply to comm-central at all.

Comment 15

[Ludovic Hirlimann [:Usul]]

(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]

Yeah.

Comment 17

[:aceman]

Attachment: unbitrotted patch

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/fab9e5145cd4