1390428 - Remove nsXPIDLCString

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

Let's remove nsXPIDLCString.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove many nsXPIDLCString local variables

These are all easy cases where an nsXPIDLCString local variable is set via
getter_Copies() and then is only used in ways that nsCStrings can also be used
(i.e. no null checks or implicit conversions to |char*|).

In every case the patch trivially replaces the nsXPIDLCString with an
nsCString. (Also, there are a couple of unused nsXPIDLCString variables that
the patch simply removes.)

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove more nsXPIDLCString local variables

These are all easy cases where an nsXPIDLCString local variable is set via
getter_Copies() and then is used in ways that rely on the implicit conversion
to |char*|. The patch uses get() and EqualsLiteral() calls to replace the
implicit conversions.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove nsXPIDLCString use in nsHttpHandler

Most of these fields are not using any nsXPIDLCString-specific features.
The exceptions are mUserAgentOverride and mDefaultSocketType, which require a
little more care.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove still more nsXPIDLCString local variables

These are all easy cases where an nsXPIDLCString local variable is set via
getter_Copies() and then is null checked. The patch uses IsVoid() to replace
the null checks (and get() and EqualsLiteral() calls to replace any implicit
conversions).

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove more nsXPIDLCString uses

These are all simple cases, with similarities to previous patches in this
series.

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8897359 [details] [diff] [review]
(part 5) - Remove more nsXPIDLCString uses

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

r+ with minor nits addressed

::: xpfe/components/directory/nsDirectoryViewer.cpp
@@ +741,2 @@
>    GetDestination(r, uri);
> +  return uri.get() && !strncmp(uri.get(), kFTPProtocol, sizeof(kFTPProtocol) - 1) &&

a) Should this be `!uri.IsVoid()`? I thought we always used empty string internally, but I'm might be recalling incorrectly
b) We could just use `StringBeginsWith(uri, NS_LITERAL_CSTRING(kFTPProtocol)` now

@@ +936,5 @@
>        if (source) {
>          httpIndex->GetDestination(source, uri);
>        }
>  
> +      if (!uri.get()) {

Same question about IsVoid

Comment 7

[Nicholas Nethercote [inactive]]

> > +  return uri.get() && !strncmp(uri.get(), kFTPProtocol, sizeof(kFTPProtocol) - 1) &&
> 
> a) Should this be `!uri.IsVoid()`? I thought we always used empty string
> internally, but I'm might be recalling incorrectly
> b) We could just use `StringBeginsWith(uri,
> NS_LITERAL_CSTRING(kFTPProtocol)` now

Yes and yes! Good catches, thank you.

> > +      if (!uri.get()) {
> 
> Same question about IsVoid

Correct here, too.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/822d7f1bc602f84332b13e258738aa3b95700691
Bug 1390428 (part 1) - Remove many nsXPIDLCString local variables. r=erahm.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Remove nsXPIDLCString class members

These are all straightforward except for InternalLoadEvent::mTypeHint, which
requires a bit of care to preserve existing behaviour.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Remove a tricky nsXPIDLCString variable

The existing comment was very helpful here.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Remove nsXPIDLCString use in NullCString()

This change makes NullCString() work the same way as NullString().

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Remove nsXPIDLCString

This is straightforward, with only two notable things.

- `#include "nsXPIDLString.h" is replaced with `#include "nsString.h"`
  throughout, because all nsXPIDLString.h did was include nsString.h. The
  exception is for files which already include nsString.h, in which case the
  patch just removes the nsXPIDLString.h inclusion.

- The patch removes the |xpidl_string| gtest, but improves the |voided| test to
  cover some of its ground, e.g. testing Adopt(nullptr).

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/822d7f1bc602

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf0e8609abb4b978070dd0cbfcab6de7b2cda73
Bug 1390428 (part 2) - Remove more nsXPIDLCString local variables. r=erahm.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Nicholas Nethercote [:njn] from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> fbf0e8609abb4b978070dd0cbfcab6de7b2cda73
> Bug 1390428 (part 2) - Remove more nsXPIDLCString local variables. r=erahm.

Backed out for various Windows clipboard test failures (Win7/Win8, all flavors).
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2571908d00cad44be3fc9dcc14426060b8b0a77

https://treeherder.mozilla.org/logviewer.html#?job_id=124096377&repo=mozilla-inbound

Comment 16

[Eric Rahm [:erahm]]

Comment on attachment 8898148 [details] [diff] [review]
(part 9) - Remove nsXPIDLCString

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

\o/

Comment 17

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8e481fd0e16fba17ab6786a3f5212ab814c0e9
Bug 1390428 (part 2, attempt 2) - Remove more nsXPIDLCString local variables. r=erahm.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cc8e481fd0e1

Comment 19

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/009055acb0b72b06f64570bd4c91eb6742975307
Bug 1390428 (part 3) - Remove nsXPIDLCString use in nsHttpHandler. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0db0af85307fc2458bf255661cc72bd6f6ff97ab
Bug 1390428 (part 4) - Remove still more nsXPIDLCString local variables. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6cd458e0634bc42e4dd8f1bf5dcc53fe701dcde
Bug 1390428 (part 5) - Remove more nsXPIDLCString uses. r=erahm.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/009055acb0b7
https://hg.mozilla.org/mozilla-central/rev/0db0af85307f
https://hg.mozilla.org/mozilla-central/rev/c6cd458e0634

Comment 21

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d792b3bb3d98254f2ba3ea23d0bfeb3151c99aa5
Bug 1390428 (part 6) - Remove nsXPIDLCString class members. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeab1b5748c568d30b7909a7eae2b8fceb3956cf
Bug 1390428 (part 7) - Remove a tricky nsXPIDLCString variable. r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/63a6f7bcca059cffac8cd9e4320cb1b107b8dbda
Bug 1390428 (part 8) - Remove nsXPIDLCString use in NullCString(). r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e420a10be0c7b2037fab085aa7e25633b6242a
Bug 1390428 (part 9) - Remove nsXPIDLCString. r=erahm.

Comment 22

[Nicholas Nethercote [inactive]]

I updated https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings to remove all mentions of nsXPIDL[C]String.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should this still have the 'leave-open' keyword?

Comment 24

[Nicholas Nethercote [inactive]]

> Should this still have the 'leave-open' keyword?

I had already removed it. But thank you for paying attention :)

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d792b3bb3d98
https://hg.mozilla.org/mozilla-central/rev/eeab1b5748c5
https://hg.mozilla.org/mozilla-central/rev/63a6f7bcca05
https://hg.mozilla.org/mozilla-central/rev/e3e420a10be0

Comment 26

[Nicholas Nethercote [inactive]]

Attachment: Remove unused nsXPIDLString.h inclusion

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8900559 [details] [diff] [review]
Remove unused nsXPIDLString.h inclusion

Thanks.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/56c3a9e0cbda
Remove unused nsXPIDLString.h inclusion. r=jorgk