make method name of nsISmtpService decent

RESOLVED FIXED in Thunderbird 22.0

Status

MailNews Core
Networking: SMTP
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

({addon-compat})

Trunk
Thunderbird 22.0
addon-compat

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(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 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

5 years ago
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

5 years ago
Yes, I'm still working on this.
(Assignee)

Comment 5

5 years ago
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
Attachment #555063 - Attachment is obsolete: true
Attachment #592640 - Flags: review?(mbanner)
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-

Updated

5 years ago
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 7

5 years ago
Created attachment 599383 [details] [diff] [review]
Revised patch

* 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

5 years ago
Comment on attachment 599383 [details] [diff] [review]
Revised patch

Forgot to change the uuid.
Attachment #599383 - Flags: review?(mbanner)
(Assignee)

Comment 9

5 years ago
Created attachment 599395 [details] [diff] [review]
Change the uuid too
Attachment #599383 - Attachment is obsolete: true
Attachment #599395 - Flags: review?(mbanner)
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

5 years ago
Keywords: checkin-needed
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)
Keywords: checkin-needed

Comment 12

5 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

4 years ago
Is there anything to be done here? It has all the required reviews. checkin-needed?
Keywords: checkin-needed
This needs rebasing. It doesn't apply to comm-central at all.
Keywords: checkin-needed
(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

4 years ago
Yeah.

Comment 17

4 years ago
Created attachment 724071 [details] [diff] [review]
unbitrotted patch
Attachment #599395 - Attachment is obsolete: true
Attachment #724071 - Flags: review+

Updated

4 years ago
Keywords: addon-compat, checkin-needed
Whiteboard: patchlove
https://hg.mozilla.org/comm-central/rev/fab9e5145cd4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.