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

RESOLVED FIXED in Thunderbird 57.0

Status

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

unspecified
Thunderbird 57.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Almost all callers have nsCStrings anyway.
(Assignee)

Updated

a year ago
Summary: Change nsMsgNewURL(nsIURI** , const char *) to nsMsgNewURL(nsIURI** , const nsCString) → Change nsMsgNewURL(nsIURI** , const char *) to nsMsgNewURL(nsIURI** , const nsCString&)
(Assignee)

Comment 1

a year ago
Created attachment 8900482 [details] [diff] [review]
1393228-nsMsgNewURL.patch (v1).

I saw all those .get() accessors when fixing bustage in nsMsgSend.cpp.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8900482 - Flags: review?(acelists)

Comment 2

a year ago
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?
(Assignee)

Comment 3

a year ago
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)).
(Assignee)

Comment 4

a year ago
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

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

Updated

a year ago
Attachment #8900482 - Flags: review?(acelists) → review+
(Assignee)

Comment 6

a year ago
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
(Assignee)

Comment 8

a year ago
(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

a year ago
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.
(Assignee)

Comment 10

a year ago
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.
(Assignee)

Comment 12

a year ago
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)

Comment 13

a year ago
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.
(Assignee)

Comment 14

a year ago
Created attachment 8900665 [details] [diff] [review]
1393228-nsMsgNewURL.patch (v1a).

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

Comment 15

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

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

Compiles ;)
Attachment #8900665 - Flags: review+

Comment 16

a year ago
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
Last Resolved: a year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.