The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Networking: POP
--
enhancement
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: timeless, Assigned: aceman)

Tracking

Trunk
Thunderbird 13.0

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

15 years ago
the concatenation operator is supposed to be better than sequential appends.
(Reporter)

Comment 1

14 years ago
Created attachment 110932 [details] [diff] [review]
callqueryinterface, concatenation
(Reporter)

Comment 2

14 years ago
Created attachment 111146 [details] [diff] [review]
+ fix tabs, correct PR_...free, early returns,
Attachment #110932 - Attachment is obsolete: true
Product: MailNews → Core
Product: Core → MailNews Core
QA Contact: esther → networking.pop
(Assignee)

Comment 3

5 years ago
This was never reviewed. Is it still relevant?

Comment 4

5 years ago
(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.
Yeah I can't remember from the frozen linkages aspect, maybe Neil can.

Comment 6

5 years ago
Indeed, nsStringAPI doesn't support operator+().
(Assignee)

Comment 7

5 years ago
What about the rest of the changes?

Comment 8

5 years ago
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.
Attachment #111146 - Attachment description: + fix tabs, correct PR_...free, early returns, → + fix tabs, correct PR_...free, early returns,
(Assignee)

Comment 9

5 years ago
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.
Assignee: timeless → acelists
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Hardware: x86 → All
Summary: pop service NewURI should rely on + concatenation → various cleanup in nsPop3Service.cpp ("pop service NewURI should rely on + concatenation" no longer valid)
(Assignee)

Comment 10

5 years ago
Created attachment 591240 [details] [diff] [review]
patch
Attachment #111146 - Attachment is obsolete: true
Attachment #591240 - Flags: review?(neil)
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.
Attachment #591240 - Flags: review?(neil) → review?(dbienvenu)
(Assignee)

Comment 12

5 years ago
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.
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.
(Assignee)

Comment 14

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

Oh, "break and then tweak until it compiles somehow"-method again :)
Will do.
Attachment #591240 - Flags: review?(dbienvenu)
(Assignee)

Comment 15

5 years ago
Created attachment 592191 [details] [diff] [review]
patch v2
Attachment #591240 - Attachment is obsolete: true
Attachment #592191 - Flags: review?(dbienvenu)

Comment 16

5 years ago
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;
kNotFound should be fine, we #define it to -1 in nsMsgUtils.h for you.

Comment 18

5 years ago
Comment on attachment 592191 [details] [diff] [review]
patch v2

r=me, modulo the kNotFound comment
Attachment #592191 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 594766 [details] [diff] [review]
patch v3

Fixes nits and keeps r=dbienvenu.
Attachment #592191 - Attachment is obsolete: true
Attachment #594766 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/99d57b8e0bef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
You need to log in before you can comment on or make changes to this bug.