Closed
Bug 1384834
Opened 7 years ago
Closed 7 years ago
Remove nsAdopting[C]String
Categories
(Core :: XPCOM, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
erahm
:
review+
erahm
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
Assignee: nobody → n.nethercote
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8890713 -
Flags: review?(erahm)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8890714 -
Flags: review?(erahm)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8890715 -
Flags: review?(erahm)
Comment 4•7 years ago
|
||
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 5•7 years ago
|
||
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 6•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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•7 years ago
|
||
> We should file follow up to convert nsEscapeHTML to just use nsACStrings.
I filed bug 1385172.
Updated•7 years ago
|
Attachment #8891152 -
Flags: review?(erahm) → review+
Comment 10•7 years ago
|
||
(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.
Updated•7 years ago
|
Attachment #8890714 -
Flags: review+
Assignee | ||
Comment 11•7 years 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.
Assignee | ||
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd41537bcdd1b492583c553ec0191d2fd70689c7
Bug 1384834 (part 1) - Remove remaining uses of nsAdoptingString. r=erahm.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f258975b80a9cef336962987cdf33113a8ddb2
Bug 1384834 (part 2) - Remove remaining uses of nsAdoptingCString. r=erahm.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b45d52b78e2757f26559033bdb476e22c35159
Bug 1384834 (part 3) - Remove nsAdopting[C]String. r=erahm.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e8c5e46a872611ddbe8703980212d5daf2984f
Bug 1384834 (part 4) - Improve comments for Adopt() and getter_Copies(). r=erahm.
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd41537bcdd1
https://hg.mozilla.org/mozilla-central/rev/d1f258975b80
https://hg.mozilla.org/mozilla-central/rev/f3b45d52b78e
https://hg.mozilla.org/mozilla-central/rev/d6e8c5e46a87
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Blocks: StringCleaning
Updated•4 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•