1393228 - Change nsMsgNewURL(nsIURI** , const char *) to nsMsgNewURL(nsIURI** , const nsCString&)

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Almost all callers have nsCStrings anyway.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1393228-nsMsgNewURL.patch (v1).

I saw all those .get() accessors when fixing bustage in nsMsgSend.cpp.

Comment 2

[:aceman]

Comment on attachment 8900482 [details] [diff] [review]
1393228-nsMsgNewURL.patch (v1).

Review of attachment 8900482 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks.

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +1540,5 @@
>      return NS_ERROR_NULL_POINTER;
>    nsCOMPtr<nsIIOService> pNetService =
>      mozilla::services::GetIOService();
>    NS_ENSURE_TRUE(pNetService, NS_ERROR_UNEXPECTED);
> +  if (aSpec.Find("://") == kNotFound && !Substring(aSpec, 0, 5).Equals("data:"))

Can you check if aSpec.EqualsASCII("data:", 5) would work here?

Comment 3

[Jorg K (CEST = GMT+2)]

That compiles, but it's not what we want:
http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/xpcom/string/nsTSubstring.cpp#824

bool
nsTStringRepr_CharT::EqualsASCII(const char* aData, size_type aLen) const
{
  return mLength == aLen &&
         char_traits::compareASCII(mData, aData, aLen) == 0;
}

But agreed, the Substring().Equals() is ugly, how about:
  if (aSpec.Find("://") == kNotFound && aSpec.Find("data:") != 0)

Sadly there is not StartsWith(). Or we leave strncmp(aSpec.BeginReading(), "data:", 5)).

Comment 4

[Jorg K (CEST = GMT+2)]

However, the Substring().Equals() is what everyone else does:
http://searchfox.org/mozilla-central/search?q=substring.*equals&case=false&regexp=true&path=

Comment 5

[:aceman]

Yes, with those options it is not that bad :)

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8900482 [details] [diff] [review]
1393228-nsMsgNewURL.patch (v1).

Thanks for the review. When I filed the bug I had only seen the many .get() accessors in nsMsgSend.cpp.

Now we need to this Substring().Equals() which isn't so nice. So does this patch have any real advantage or should be just make this WONTFIX?

Nicholas, you do string things, could you give me your opinion on whether to land this patch or not.

Comment 7

[Nicholas Nethercote [inactive]]

The get() removal is nice and the Substring() usage doesn't seem any worse to me than strncmp(). So I think landing is fine.

BTW, nsACString is generally preferred over nsCString for parameters. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Appendix_A_-_What_class_to_use_when

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> BTW, nsACString is generally preferred over nsCString for parameters.
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/
> Internal_strings#Appendix_A_-_What_class_to_use_when

Well, ...
0:10.28 c:/mozilla-source/comm-central/mailnews/compose/src/nsMsgCompUtils.cpp(1544): error C2039: 'Find': is not a member of 'nsACString'

So what do I do with that? Add PromiseFlatCString()?

Comment 9

[:aceman]

I'm also for landing that is why I gave r+ together with the Substring().Equals().
Sorry I didn't compile it as my tree doesn't compile at some other place right now.

Comment 10

[Jorg K (CEST = GMT+2)]

To clarify: The patch compiles, changing it to nsACString does not.

Comment 11

[Nicholas Nethercote [inactive]]

> So what do I do with that? Add PromiseFlatCString()?

That should do it. Or you could just keep it as nsCString, probably doesn't matter that much either way.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8900482 [details] [diff] [review]
1393228-nsMsgNewURL.patch (v1).

Thanks.

Having grown up with "C", I think strncmp(aSpec, "data:", 5) would be (much) faster than !Substring(aSpec, 0, 5).Equals("data:") since the latter is already two function calls with object creation, etc. So adding yet another function call would just be overkill, IMHO.

Comment 13

[:aceman]

But doesn't strncmp work with the raw char* pointer, which nsCString wants to encapsulate and hide away?
So let's not touch the raw pointers when we can avoid it.

Can you see if StringBeginsWith() works on nsCString? Maybe it is only for nsString.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1393228-nsMsgNewURL.patch (v1a).

if (aSpec.Find("://") == kNotFound && !StringBeginsWith(aSpec, NS_LITERAL_CSTRING("data:")))
works.

Comment 15

[:aceman]

Comment on attachment 8900665 [details] [diff] [review]
1393228-nsMsgNewURL.patch (v1a).

Review of attachment 8900665 [details] [diff] [review]:
-----------------------------------------------------------------

Compiles ;)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/88afdfc38ae6
Change nsMsgNewURL(nsIURI**, const char *) to nsMsgNewURL(nsIURI**, const nsCString&). r=aceman