Closed Bug 1393228 Opened 2 years ago Closed 2 years ago

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

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: jorgk, Assigned: jorgk)

Details

Attachments

(2 files)

Almost all callers have nsCStrings anyway.
Summary: Change nsMsgNewURL(nsIURI** , const char *) to nsMsgNewURL(nsIURI** , const nsCString) → Change nsMsgNewURL(nsIURI** , const char *) to nsMsgNewURL(nsIURI** , const nsCString&)
I saw all those .get() accessors when fixing bustage in nsMsgSend.cpp.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8900482 - Flags: review?(acelists)
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?
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)).
However, the Substring().Equals() is what everyone else does:
http://searchfox.org/mozilla-central/search?q=substring.*equals&case=false&regexp=true&path=
Yes, with those options it is not that bad :)
Attachment #8900482 - Flags: review?(acelists) → review+
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.
Attachment #8900482 - Flags: feedback?(n.nethercote)
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
(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()?
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.
To clarify: The patch compiles, changing it to nsACString does not.
> 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 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.
Attachment #8900482 - Flags: feedback?(n.nethercote)
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.
if (aSpec.Find("://") == kNotFound && !StringBeginsWith(aSpec, NS_LITERAL_CSTRING("data:")))
works.
Comment on attachment 8900665 [details] [diff] [review]
1393228-nsMsgNewURL.patch (v1a).

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

Compiles ;)
Attachment #8900665 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/88afdfc38ae6
Change nsMsgNewURL(nsIURI**, const char *) to nsMsgNewURL(nsIURI**, const nsCString&). r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.