Last Comment Bug 186731 - various cleanup in nsPop3Service.cpp ("pop service NewURI should rely on + concatenation" no longer valid)
: various cleanup in nsPop3Service.cpp ("pop service NewURI should rely on + co...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: POP (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
Depends on:
Blocks: 186719
  Show dependency treegraph
 
Reported: 2002-12-24 23:29 PST by timeless
Modified: 2012-02-13 06:26 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
callqueryinterface, concatenation (1.56 KB, patch)
2003-01-08 01:11 PST, timeless
no flags Details | Diff | Splinter Review
+ fix tabs, correct PR_...free, early returns, (10.17 KB, patch)
2003-01-09 22:12 PST, timeless
no flags Details | Diff | Splinter Review
patch (20.71 KB, patch)
2012-01-24 13:09 PST, :aceman
no flags Details | Diff | Splinter Review
patch v2 (20.59 KB, patch)
2012-01-27 11:35 PST, :aceman
mozilla: review+
Details | Diff | Splinter Review
patch v3 (22.28 KB, patch)
2012-02-06 11:59 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description timeless 2002-12-24 23:29:30 PST
the concatenation operator is supposed to be better than sequential appends.
Comment 1 timeless 2003-01-08 01:11:52 PST
Created attachment 110932 [details] [diff] [review]
callqueryinterface, concatenation
Comment 2 timeless 2003-01-09 22:12:31 PST
Created attachment 111146 [details] [diff] [review]
+ fix tabs, correct PR_...free, early returns,
Comment 3 :aceman 2012-01-23 08:41:56 PST
This was never reviewed. Is it still relevant?
Comment 4 David :Bienvenu 2012-01-23 09:56:02 PST
(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) 2012-01-23 10:06:30 PST
Yeah I can't remember from the frozen linkages aspect, maybe Neil can.
Comment 6 neil@parkwaycc.co.uk 2012-01-23 16:16:07 PST
Indeed, nsStringAPI doesn't support operator+().
Comment 7 :aceman 2012-01-23 23:48:16 PST
What about the rest of the changes?
Comment 8 neil@parkwaycc.co.uk 2012-01-24 01:52:00 PST
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 2012-01-24 03:04:16 PST
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 2012-01-24 13:09:15 PST
Created attachment 591240 [details] [diff] [review]
patch
Comment 11 neil@parkwaycc.co.uk 2012-01-25 13:59:09 PST
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 2012-01-25 14:15:52 PST
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) 2012-01-27 00:53:21 PST
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 2012-01-27 02:43:18 PST
Comment on attachment 591240 [details] [diff] [review]
patch

Oh, "break and then tweak until it compiles somehow"-method again :)
Will do.
Comment 15 :aceman 2012-01-27 11:35:39 PST
Created attachment 592191 [details] [diff] [review]
patch v2
Comment 16 David :Bienvenu 2012-02-06 09:05:48 PST
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 neil@parkwaycc.co.uk 2012-02-06 09:07:44 PST
kNotFound should be fine, we #define it to -1 in nsMsgUtils.h for you.
Comment 18 David :Bienvenu 2012-02-06 09:09:07 PST
Comment on attachment 592191 [details] [diff] [review]
patch v2

r=me, modulo the kNotFound comment
Comment 19 :aceman 2012-02-06 11:59:22 PST
Created attachment 594766 [details] [diff] [review]
patch v3

Fixes nits and keeps r=dbienvenu.
Comment 20 Mark Banner (:standard8) 2012-02-13 06:26:00 PST
Checked in: http://hg.mozilla.org/comm-central/rev/99d57b8e0bef

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