186728 - smtp service NewURI should use callqueryinterface

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, enhancement)

Description

[timeless]

from reading the netwerk impls, it looks like this could use CallQueryInterface.

Comment 1

[timeless]

Attachment: callqueryinterface

Comment 2

[timeless]

Attachment: and do_CreateInstance and do_QueryElementAt ...

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 111141 [details] [diff] [review]
and do_CreateInstance and do_QueryElementAt ...

>Index: mozilla/mailnews/compose/src/nsSmtpService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mozilla/mailnews/compose/src/nsSmtpService.cpp,v
>retrieving revision 1.117
>diff -u -r1.117 mozilla/mailnews/compose/src/nsSmtpService.cpp
>--- mozilla/mailnews/compose/src/nsSmtpService.cpp
>+++ mozilla/mailnews/compose/src/nsSmtpService.cpp
>@@ -294,7 +294,7 @@
>             if (aStatusFeedback)
>                 url->SetStatusFeedback(aStatusFeedback);
>         }
>-        rv = smtpUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) aUrl);
>+        rv = CallQueryInterface(smtpUrl, aUrl);
>     }
> 
>     return rv;
>@@ -321,7 +321,7 @@
> 
>         NS_ADDREF(smtpProtocol);
>         rv = smtpProtocol->LoadUrl(aUrl, aConsumer); // protocol will get destroyed when url is completed...
>-        smtpProtocol->QueryInterface(NS_GET_IID(nsIRequest), (void **) aRequest);
>+        CallQueryInterface(smtpProtocol, aRequest);
>         NS_RELEASE(smtpProtocol);
>     }
> 
>@@ -584,12 +584,10 @@
>   // get a new smtp url 
> 
>   nsresult rv = NS_OK;
>-	nsCOMPtr <nsIURI> mailtoUrl;
>+  nsCOMPtr <nsIURI> mailtoUrl = do_CreateInstance(kCMailtoUrlCID);
> 
>-	rv = nsComponentManager::CreateInstance(kCMailtoUrlCID, NULL, NS_GET_IID(nsIURI), getter_AddRefs(mailtoUrl));
>-
>-	if (NS_SUCCEEDED(rv))
>-	{
>+  if (NS_SUCCEEDED(rv))
>+  {

rv will always be NS_OK here, because do_CreateInstance was only called with
one param.   How about calling do_CreateInstance with two params, and, while
you're at it, switch from CID to ContractID.

Everything else looks good, however.

Note that you still need to a have mailnews module owner review this, so I'd
suggest pinging sspitzer for super-review instead of darin.

Comment 4

[Wayne Mery (:wsmwk)]

timeless, is this still needed?
(resetting QA)

Comment 5

[Phil Ringnalda (:philor)]

Yes, though I think between them darin and Standard8 have gotten everything except the smtpProtocol hunk over the years.

Comment 6

[Mark Banner (:standard8)]

Attachment: Fix nsSmtpProtocol instance

Phil wasn't quite right, there were two instances left. Let's just fix them and close out this bug.

Comment 7

[[e-mail]]

Comment on attachment 367573 [details] [diff] [review]
Fix nsSmtpProtocol instance

> nsresult NS_MsgLoadSmtpUrl(nsIURI * aUrl, nsISupports * aConsumer, nsIRequest ** aRequest)
> {
>-    // for now, assume the url is an smtp url and load it....
>-    nsCOMPtr <nsISmtpUrl> smtpUrl;
>-    nsSmtpProtocol *smtpProtocol = nsnull;
>-    nsresult rv = NS_OK;
>+  if (!aUrl)
>+    return NS_OK; 
>-    if (!aUrl)
>-        return rv;
Eww, not writing an outparam on success? I guess we never pass null ;-)

>+  nsSmtpProtocol* smtpProtocol = new nsSmtpProtocol(aUrl);
Use nsRefPtr<nsSmtpProtocol> to avoid manual addref/release, or use nsCOMPtr<nsIRequest> and .forget instead of CallQueryInterface:
nsCOMPtr<nsIRequest> request(smtpProtocol);
...
request.forget(aRequest);

>+  nsresult rv = smtpProtocol->LoadUrl(aUrl, aConsumer);
>+  CallQueryInterface(smtpProtocol, aRequest);
We should only do this when LoadUrl succeeds, otherwise we'll leak...

>+  return CallQueryInterface(mailtoUrl, _retval);
mailtoUrl.forget(_retval);

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> >+  nsSmtpProtocol* smtpProtocol = new nsSmtpProtocol(aUrl);
> Use nsRefPtr<nsSmtpProtocol> to avoid manual addref/release, or use
> nsCOMPtr<nsIRequest> and .forget instead of CallQueryInterface:
> nsCOMPtr<nsIRequest> request(smtpProtocol);
> ...
> request.forget(aRequest);

LoadUrl is a function on nsSmtpProtocol, so its the nsRefPtr option here.

Comment 9

[Mark Banner (:standard8)]

Attachment: Patch for checkin

This is the one I'm checking in.

Comment 10

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/ec7a662e202c