186731 - various cleanup in nsPop3Service.cpp ("pop service NewURI should rely on + concatenation" no longer valid)

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, enhancement)

Description

[timeless]

the concatenation operator is supposed to be better than sequential appends.

Comment 1

[timeless]

Attachment: callqueryinterface, concatenation

Comment 2

[timeless]

Attachment: + fix tabs, correct PR_...free, early returns,

Comment 3

[:aceman]

This was never reviewed. Is it still relevant?

Comment 4

[David :Bienvenu]

(In reply to :aceman from comment #3)
> This was never reviewed. Is it still relevant?

It's extremely bit-rotted, and I think moving to frozen linkages might make this not doable.

Comment 5

[Mark Banner (:standard8)]

Yeah I can't remember from the frozen linkages aspect, maybe Neil can.

Comment 6

[[e-mail]]

Indeed, nsStringAPI doesn't support operator+().

Comment 7

[:aceman]

What about the rest of the changes?

Comment 8

[[e-mail]]

Comment on attachment 111146 [details] [diff] [review]
+ fix tabs, correct PR_...free, early returns,

>-  if (!((const char *)popHost)) 
>+  if (popHost.IsEmpty()) 
Already landed.

>       ? PR_smprintf("pop3://%s@%s:%d", (const char *)escapedUsername, (const char *)popHost, popPort)
>       : PR_smprintf("pop3://%s@%s:%d/?check", (const char *)escapedUsername, (const char *)popHost, popPort);
>     rv = BuildPop3Url(urlSpec, aInbox, aPopServer, aUrlListener, getter_AddRefs(url), aMsgWindow);
>-    PR_Free(urlSpec);
>+    PR_smprintf_free(urlSpec);
Useful.

>-  rv = pop3Url->QueryInterface(NS_GET_IID(nsIURI), (void **) aUrl);
>+  rv = CallQueryInterface(pop3Url, aUrl);
Good.

>-    aScheme = "pop3";
>+  aScheme = NS_LITERAL_CSTRING("pop3");
But .AssignLiteral would be better still.

>-    rv = protocol->QueryInterface(NS_GET_IID(nsIChannel), (void **) _retval);
>+  rv = CallQueryInterface(protocol, _retval);
Good.

Comment 9

[:aceman]

Thanks.

So I assume the talk in comment 6 was about this part:
-    nsCAutoString popSpec("pop://");
-    popSpec += escapedUsername;
-    popSpec += "@";
-    popSpec += hostname;
-    popSpec += ":";
+  nsCAutoString popSpec;
+  popSpec = NS_LITERAL_CSTRING("pop://")
+            + nsDependentCString(escapedUsername)
+            + NS_LITERAL_CSTRING("@")
+            + nsDependentCString(hostname)
+            + NS_LITERAL_CSTRING(":");
     popSpec.AppendInt(port);
-    popSpec += "?";
+  popSpec += NS_LITERAL_CSTRING("?");

I'll drop that but refresh the other changes.

Comment 10

[:aceman]

Attachment: patch

Comment 11

[[e-mail]]

Comment on attachment 591240 [details] [diff] [review]
patch

I don't see any technical errors in these changes although I can't vouch for the correctness of the style changes so punting the review over to bienvenu.

>+  nsPop3Protocol *protocol = new nsPop3Protocol(aURI);
>+  NS_ENSURE_TRUE(protocol, NS_ERROR_OUT_OF_MEMORY);
>+
>+  NS_ADDREF(protocol);
>+  rv = protocol->Initialize(aURI);
>+  if (NS_FAILED(rv))
>   {
>+    NS_RELEASE(protocol);
>+    return rv;
>   }
>+  protocol->SetUsername(realUserName.get());
>+
>+  rv = CallQueryInterface(protocol, _retval);
>+  NS_RELEASE(protocol);
> 
>   return rv;
> }
We can actually improve further on this using nsRefPtr<nsPop3Protocol> protocol which allows us to remove the NS_ADDREF and NS_RELEASE lines.

Comment 12

[:aceman]

Sorry, I can't do that, do not understand those ns pointers yet.
But if you can write it I can include it in the patch. Or you'd need to do in another bug.

Comment 13

[Mark Banner (:standard8)]

You basically need to do exactly what Neil said ;-) :

Replace nsPop3Protocol *protocol with nsRefPtr<nsPop3Protocol> protocol, and then drop all the NS_ADDREF and NS_RELEASE lines that work on protocol.

Comment 14

[:aceman]

Comment on attachment 591240 [details] [diff] [review]
patch

Oh, "break and then tweak until it compiles somehow"-method again :)
Will do.

Comment 15

[:aceman]

Attachment: patch v2

Comment 16

[David :Bienvenu]

looks good, thx, a few nits:

+
+  rv = CallQueryInterface(protocol, _retval);
 
   return rv;

this can be just return CallQueryInterface...

I think using kNotFound instead of -1 makes Neil unhappy w.r.t. frozen linkages - please correct me if I'm wrong, Neil.


please fix the space after nsCOMPtr...

+    nsCOMPtr <nsIMsgMailNewsUrl> url = do_QueryInterface(aUrlToRun);
+    if (url)
+      AlertServerBusy(url);
+    rv = NS_ERROR_FAILURE;

Comment 17

[[e-mail]]

kNotFound should be fine, we #define it to -1 in nsMsgUtils.h for you.

Comment 18

[David :Bienvenu]

Comment on attachment 592191 [details] [diff] [review]
patch v2

r=me, modulo the kNotFound comment

Comment 19

[:aceman]

Attachment: patch v3

Fixes nits and keeps r=dbienvenu.

Comment 20

[Mark Banner (:standard8)]

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