1401813 - Rename Null[C]String() as Void[C]String()

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

XPCOM's string API doesn't have the notion of a "null string". But it does have
the notion of a "void string" (or "voided string"), and that's what these
functions are returning. So the names should reflect that.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Rename Null[C]String() as Void[C]String()

Comment 2

[Eric Rahm [:erahm]]

Comment on attachment 8910559 [details] [diff] [review]
Rename Null[C]String() as Void[C]String()

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

Seems reasonable, might want to send an email to dev-platform telling them where `NullString` went.

Comment 3

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10cfc563ec9dc126f4667d1d6952fbd2a703ab9a
Bug 1401813 - Rename Null[C]String() as Void[C]String(). r=erahm.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I explicitly added Null[C]String() since that is how and why the void-ness was added to string APIs, to be able to express null in DOM APIs, which deal with strings or null values.

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/10cfc563ec9d

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'm sort of with comment 4 here.  A bunch of the changes in this patch are to places where the intent really is to express "null" on the JS side.  XPCOM strings don't have such a concept, but DOM strings definitely do; see dom/base/nsDOMString.h

At the very least, this affects things like the changes to Element::SetAttrAndNotify/UnsetAttr and nsDOMMutationRecord where the string is really representing a nullable string type to be passed to JS.  The new code is much more confusing, since nothing actually clearly documents that the internal implementation details of VoidString() means "null" in JS terms.

Now we could try to switch to a different type for "nullable string" and stop using void strings to represent null.  I'd be OK with that, but that needs to actually happen.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I think I'd rather just have Null[C]Strings and changed Set/IsVoid to Set/IsNull.

Comment 8

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

I'm against having the string classes try to claim they support null.

The problem with supporting "null" as a concept is that people have expectations about what null means, for example, that it implies making the string empty, and that various other operations on a string (which ones exactly?) such as appending data to it might make the string no longer be null.

scc initially suggested the void flag be called "purple" rather than "void" to suggest that this was just a marker that the DOM code could use to indicate that it's transferring an extra bit of data through the string classes, but that the string classes don't actually know anything about that bit.  "void" is a bit of a compromise -- it doesn't convey the clear semantics that null does.

But I think actually making the string API claim to support null would mean that people would have various expectations such as that appending data to a string would make it non-null, and maybe other things (e.g., should a null string, concatenated with another null string, also be a null string?).  Supporting these things would add unneeded complexity to the string code -- complexity that's not needed for the DOM bindings to support null strings -- merely to match the expectations conveyed by an inappropriate name.

Thus I prefer naming the API in a way that makes it clearer that this is a bit that the string code doesn't know about or interpret.

I'd also note that if I'd been asked to review bug 711841 at the time, I would have provided this feedback then.

Comment 9

[Nicholas Nethercote [inactive]]

My opinions aren't super strong here, so I'm happy to defer to whatever everyone else agrees on.

Comment 10

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> My opinions aren't super strong here, so I'm happy to defer to whatever
> everyone else agrees on.

We discussed a bit on dev-platform, it sounded like reverting the changes to the DOM APIs and adding a 'using nsNullString = nsVoidString;' or something along those lines would be good enough for now.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> We discussed a bit on dev-platform, it sounded like reverting the changes to the DOM APIs and adding a 'using > nsNullString = nsVoidString;' or something along those lines would be good enough for now.

Did that ever happen?

Comment 12

[Eric Rahm [:erahm]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> > We discussed a bit on dev-platform, it sounded like reverting the changes to the DOM APIs and adding a 'using > nsNullString = nsVoidString;' or something along those lines would be good enough for now.
> 
> Did that ever happen?

No, I'll file a followup now. FTR the conversation on dev-platform: https://groups.google.com/d/msg/mozilla.dev.platform/e9n9Gnv-FwU/1g3KZWf_DwAJ