Remove nsAdopting[C]String

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

54 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

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

Updated

a year ago
Depends on: 1384835
(Assignee)

Updated

a year ago
Assignee: nobody → n.nethercote
(Assignee)

Comment 1

a year ago
Created attachment 8890713 [details] [diff] [review]
(part 1) - Remove remaining uses of nsAdoptingString
Attachment #8890713 - Flags: review?(erahm)
(Assignee)

Comment 2

a year ago
Created attachment 8890714 [details] [diff] [review]
(part 2) - Remove remaining uses of nsAdoptingCString
Attachment #8890714 - Flags: review?(erahm)
(Assignee)

Comment 3

a year ago
Created attachment 8890715 [details] [diff] [review]
(part 3) - Remove nsAdopting[C]String
Attachment #8890715 - Flags: review?(erahm)
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+
(Assignee)

Comment 7

a year ago
Created attachment 8891152 [details] [diff] [review]
(part 4) - Improve comments for Adopt() and getter_Copies()

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)
(Assignee)

Comment 8

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

Comment 9

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

Comment 11

a year ago
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.
You need to log in before you can comment on or make changes to this bug.