Closed Bug 1384834 Opened 7 years ago Closed 7 years ago

Remove nsAdopting[C]String

Categories

(Core :: XPCOM, enhancement)

54 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

We want to remove nsXPIDL[C]String (bug 1377356). This requires also removing
nsAdopting[C]String, which is a subclass of nsXPIDL[C]String. Fortunately,
there aren't that many uses of nsAdopting[C]String, and those uses aren't hard
to get rid of.

Note that even once we get rid of nsAdopting[C]String we will still have the
Adopt() method and the getter_Copies() function, both of which are very useful.

Overall, this will reduce the amount of code. Some of the places that used
nsAdopting[C]String will become slightly less concise, but this is outweighed
by the removal of the class itself.
Depends on: 1384835
Assignee: nobody → n.nethercote
Comment on attachment 8890713 [details] [diff] [review]
(part 1) - Remove remaining uses of nsAdoptingString

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

::: netwerk/streamconv/converters/nsIndexedToHTML.cpp
@@ +857,5 @@
>  nsIndexedToHTML::OnInformationAvailable(nsIRequest *aRequest,
>                                          nsISupports *aCtxt,
>                                          const nsAString& aInfo) {
>      nsAutoCString pushBuffer;
> +    char16_t* str = nsEscapeHTML2(PromiseFlatString(aInfo).get());

We should really file a bug to get rid of this function / convert it to using nsString as a follow up.

If you feel like it we can remove the PromiseFlatString call, it's not required, ie:

nsEscapeHTML2(aInfo.BeginReading(), aInfo.Length());
Attachment #8890713 - Flags: review?(erahm) → review+
Comment on attachment 8890714 [details] [diff] [review]
(part 2) - Remove remaining uses of nsAdoptingCString

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

::: netwerk/dns/nsDNSService2.cpp
@@ +628,5 @@
>          // Disable prefetching either by explicit preference or if a manual proxy is configured
>          mDisablePrefetch = disablePrefetch || (proxyType == nsIProtocolProxyService::PROXYCONFIG_MANUAL);
>  
>          mLocalDomains.Clear();
> +        if (!localDomains.IsVoid()) {

I don't think IsVoid will ever be false, we probably just want IsEmpty()

::: netwerk/streamconv/converters/nsIndexedToHTML.cpp
@@ +556,5 @@
>          // dealing with a resource URI.
>          if (!isResource) {
>              buffer.AppendLiteral("<base href=\"");
> +            nsCString htmlEscapedUri;
> +            htmlEscapedUri.Adopt(nsEscapeHTML(baseUri.get()));

We should file follow up to convert nsEscapeHTML to just use nsACStrings.
Attachment #8890714 - Flags: review?(erahm) → feedback+
Comment on attachment 8890715 [details] [diff] [review]
(part 3) - Remove nsAdopting[C]String

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

\o/
Attachment #8890715 - Flags: review?(erahm) → review+
While I'm here, let me add the comments that I wish had been present when I
started this bug.
Attachment #8891152 - Flags: review?(erahm)
> I don't think IsVoid will ever be false, we probably just want IsEmpty()

Do you mean "IsVoid will ever be true"?

Here's the relevant line:

> prefs->GetCharPref(kPrefDnsLocalDomains, getter_Copies(localDomains));

It's pretty hairy what goes on under the covers. getter_Copies() creates an nsTGetterCopies_CharT, which has an mData field that defaults to nullptr. If GetCharPref() fails, then that mData field will never be overwritten. When the temporary nsTGetterCopies_CharT is destroyed its destructor calls Adopt(mData). And when Adopt() receives a nullptr, it sets the string to Void.

So I'm confident the change as written replicates the previous behaviour, which is what I want.
> We should file follow up to convert nsEscapeHTML to just use nsACStrings.

I filed bug 1385172.
Attachment #8891152 - Flags: review?(erahm) → review+
(In reply to Nicholas Nethercote [:njn] from comment #8)
> > I don't think IsVoid will ever be false, we probably just want IsEmpty()
> 
> Do you mean "IsVoid will ever be true"?

Yeah, sorry I flipped that.

> Here's the relevant line:
> 
> > prefs->GetCharPref(kPrefDnsLocalDomains, getter_Copies(localDomains));
> 
> It's pretty hairy what goes on under the covers. getter_Copies() creates an
> nsTGetterCopies_CharT, which has an mData field that defaults to nullptr. If
> GetCharPref() fails, then that mData field will never be overwritten. When
> the temporary nsTGetterCopies_CharT is destroyed its destructor calls
> Adopt(mData). And when Adopt() receives a nullptr, it sets the string to
> Void.
> 
> So I'm confident the change as written replicates the previous behaviour,
> which is what I want.

Yeah that makes sense, In general `IsVoid `is misused when `IsEmpty` is the intended logical behavior. Scanning a few usages of `IsVoid` this definitely seems to be the case, that or we should really be using |Maybe| to make it clear the string can be 'non-existent' vs 'empty'. Anyhow I'll file a follow up to discuss if getting rid of `IsVoid` would make sense.
Attachment #8890714 - Flags: review+
Julian looked a bit into removing IsVoid. It's kind of a swamp. A Void string is a weird superposition of "no string" and "empty string" and it's not used consistently. And some of the uses are related to XPIDL, which makes them hard to change. As for Maybe<>, often you'd want |Maybe<const nsACString&>| parameters but that doesn't work.
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.