186729 - mailbox service NewURI should use callqueryinterface

Status: RESOLVED FIXED

(MailNews Core :: Networking, enhancement)

Description

[timeless]

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

Comment 1

[timeless]

Attachment: callqueryinterface

Comment 2

[timeless]

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

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

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

requesting review on behalf of timeless

Comment 4

[timeless]

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

whatever conflicts may exist are resolved in my tree at home. i'm traveling
until mid january, please review based on the changes in the patch, i'll
address any concerns before i commit.

Comment 5

[David :Bienvenu]

Attachment: salvage unbitrotted parts of patch

From my reading of the code, we need to check both the source and dest of CallQueryInterface parameters, because it only does null checks in debug mode

Comment 6

[[e-mail]]

Comment on attachment 321265 [details] [diff] [review]
salvage unbitrotted parts of patch

>-  if (NS_SUCCEEDED(rv) && mailboxurl)
>+  if (NS_SUCCEEDED(rv) && aURL)
I think you still want to null-check mailboxurl rather than aURL - aURL is contracted not to be null (although I think QueryInterface checks anyway).

>-    rv = mailboxurl->QueryInterface(NS_GET_IID(nsIURI), (void **) aURL);
>+    CallQueryInterface(mailboxurl, aURL);
Isn't this supposed to have rv = ?

Comment 7

[David :Bienvenu]

Attachment: patch addressing comments

you're right about rv, and needing to check mailboxurl. But I think QueryInterface doesn't check the output parameter (it asserts, but I think in release mode it would crash), so I added an NS_ENSURE_ARG_POINTER

Comment 8

[[e-mail]]

Comment on attachment 321516 [details] [diff] [review]
patch addressing comments

>+    nsCAutoString temp = NS_LITERAL_CSTRING("mailbox://") + buf;
I don't think this is external API safe... might want to do this separately?

>+  if (mailboxurl && aURL)
>   if (aURL && mailboxurl)
>+  if (aURL && mailboxurl)
Some consistency would look nice ;-)

Comment 9

[David :Bienvenu]

Attachment: patch for checkin

I went ahead and tweaked the string stuff so that it should work with external linkage, and made the if aURL and mailboxurl checks consistent. Carrying forward the review flags...