1489047 - Remove DOMString from XPIDL

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

XPIDL has both AString and DOMString. The only difference between the two is that, when you transfer an `undefined` value from JS to C++:

- if the C++ value is an AString it will become a Void nsString;

- if the C++ value is a DOMString it will become an nsString with the value "undefined".

The latter is apparently how WebIDL works. I suspect XPIDL only has DOMString because for a while we were trying to make XPIDL emulate WebIDL.

Anyway, I'm pretty sure DOMString can now be removed from XPIDL.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Change almost all DOMString occurrences in XPIDL files to AString

Because they have almost identical semantics.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Remove C++ support for, and testing of, the XPIDL DOMString type

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Remove support for DOMString from the XPIDL compiler

Comment 4

[Nicholas Nethercote [inactive]]

FWIW, here's what bz said about this in an email exchange I had with him a while back.

> The DOMString behavior was the one we wanted for various DOM APIs.
> The AString behavior is the one that various chrome APIs had.
> 
> I wonder whether we can just kill DOMString and replace all its uses
> with AString, now that web-exposed stuff should all be using webidl bindings anyway.

Nika, please forward these reviews onto someone else, if there is someone more appropriate.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9006793 [details] [diff] [review]
Change almost all DOMString occurrences in XPIDL files to AString

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

I think I'm a reasonably appropriate reviewer for this. 

My hesitation about this is that it feels like perhaps we should try to go the other direction -> removing AString in favor of DOMString. Moving XPIDL to have more similar semantics to WebIDL will be really nice for the future to avoid surprises due to the two IDLs acting differently.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9006795 [details] [diff] [review]
Remove support for DOMString from the XPIDL compiler

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

r=me if we decide to take this approach

::: xpcom/typelib/xpt/tools/xpt.py
@@ +280,2 @@
>          'char_ptr',
> +        '__DOMString__',    # Was 'DOMString', but no longer used

Fun fact: xpt.py still only exists in the tree due to packaging tests using it. We never actually create a XPT file.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9006794 [details] [diff] [review]
Remove C++ support for, and testing of, the XPIDL DOMString type

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

r=me if we decide to take this approach

Comment 8

[Nicholas Nethercote [inactive]]

> My hesitation about this is that it feels like perhaps we should try to
> go the other direction -> removing AString in favor of DOMString.

I will argue against this :)

XPIDL and WebIDL are sufficiently different that making them more similar in one small way doesn't gain much, IMO. But it would change XPIDL semantics that have been in place for a long time.

More practically, here's a rough count of all the different string kinds we have in XPIDL files, as judged by some careful grepping. (Careful because you need to avoid WebIDL files and uses within comments... these numbers probably aren't perfect, but should be close.)

- DOMString 253
- AString 891
- ACString 579
- AUTFString 402
- wstring 199
- string 415

Removing AString would be a larger change than removing DOMString. And then for consistency we'd probably want to change all the other types to have the same "undefined becomes 'undefined'" behaviour, which would be a larger semantics change.

Going in the other direction would also raise the question of what to do about bug 1489010 -- abandon it?

Nika, what do you think?

Comment 9

[Nicholas Nethercote [inactive]]

> Fun fact: xpt.py still only exists in the tree due to packaging tests using
> it. We never actually create a XPT file.

Well spotted! With glandium's help, I filed bug 1489340 to remove it.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> > My hesitation about this is that it feels like perhaps we should try to
> > go the other direction -> removing AString in favor of DOMString.
> 
> I will argue against this :)
> 
> XPIDL and WebIDL are sufficiently different that making them more similar in
> one small way doesn't gain much, IMO. But it would change XPIDL semantics
> that have been in place for a long time.

True. In the long term it would be nice to slowly unify the two type systems so it's easier to bride chrome code between the two, but making decisions in the short term based on that may not be the best idea. 

> More practically, here's a rough count of all the different string kinds we
> have in XPIDL files, as judged by some careful grepping. (Careful because
> you need to avoid WebIDL files and uses within comments... these numbers
> probably aren't perfect, but should be close.)
> 
> - DOMString 253
> - AString 891
> - ACString 579
> - AUTFString 402
> - wstring 199
> - string 415
> 
> Removing AString would be a larger change than removing DOMString. And then
> for consistency we'd probably want to change all the other types to have the
> same "undefined becomes 'undefined'" behaviour, which would be a larger
> semantics change.
> 
> Going in the other direction would also raise the question of what to do
> about bug 1489010 -- abandon it?
> 
> Nika, what do you think?

I think I'm fine with that in the end, it's going to make a smaller patch. I would be curious to know what the test suite looks like with AString using the same behaviour as DOMString, though. If we can switch either way without breakage, I feel like consistent behaviour is preferable.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So right now we have the following behaviors in the tree:

  AString: undefined becomes void, null becomes void.

  XPIDL DOMString: undefined becomes "undefined", null becomes void.

  WebIDL DOMString: undefined becomes "undefined", null becomes "null" unless
                    [TreatNullAs=EmptyString] is used.  Also, "DOMString?" can
                    be used to get the XPCOM "AString" behavior.

so we already have different behavior between XPIDL DOMString and WebIDL DOMString.  In terms of reducing the total number of possible behaviors it would almost make sense to make XPIDL DOMString behave like AString.  That sort of fits into the pattern of other things (like interface types) being implicitly nullable in XPIDL, maybe.

In general, regression risk is much higher, imo, when moving from the AString behavior to the XPIDL DOMString behavior than vice versa....  And there is additional regression risk if we try to change XPIDL DOMString to match (non-nullable) WebIDL DOMString.  It's not entirely clear to me to what extent those risks are worth the behavior alignment.

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #11)

> AString: undefined becomes void, null becomes void.
> 
> XPIDL DOMString: undefined becomes "undefined", null becomes void.
> 
> WebIDL DOMString: undefined becomes "undefined", null becomes "null" unless
>                   [TreatNullAs=EmptyString] is used.  Also, "DOMString?" can
>                   be used to get the XPCOM "AString" behavior.

Thanks for the extra info. XPIDL DOMString is an awkward mix! All the more reason to get rid of it.


> it would almost make sense to make XPIDL DOMString behave like AString.
> [...]
> if we try to change XPIDL DOMString to match (non-nullable) WebIDL DOMString.

I want to (a) eliminate XPIDL DOMString, (b) convert existing XPIDL DOMString uses to AString, and (c) leave XPIDL AString's behaviour unchanged. That's the smallest change that eliminates XPIDL DOMString: it touches the fewest lines, and it doesn't change the semantics of remaining XPIDL types. (I have no interest in aligning XPIDL and WebIDL behaviour, that feels unimportant to me, especially given that the name "DOMString" will no longer be shared between languages.)

In comment 10 Nika is (IIUC) suggesting we do (a) and (b), but for (c) instead change AString's behaviour to match DOMString... presumably (non-nullable) WebIDL DOMString rather than XPIDL DOMString, given that consistency between XPIDL and WebIDL is the rationale. And presumably we'd change ACString/AUTF8String/wstring/string as well, this time for consistency within XPIDL. These changes would affect many more interfaces and -- comment 5 shows that ACString/AUTF8String/wstring/string are used about 10x as often as DOMString in existing XPIDL files. And as you say, that semantics change likely has a higher chance of causing regressions.

Comment 13

[Nicholas Nethercote [inactive]]

Nika, bz: any more thoughts in light of comment 12? It would be nice to simplify XPIDL.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fwiw, I think I am OK with the (a), (b), (c) plan from comment 12....

Comment 15

[Nika Layzell [:nika] (ni? for response)]

I think I'm OK with that too - It's unfortunate IMO that we have such different semantics, but we shouldn't try to take too large of a step at once :-)

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 9006793 [details] [diff] [review]
Change almost all DOMString occurrences in XPIDL files to AString

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

Re-requesting review.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31941d904a7c
Change almost all DOMString occurrences in XPIDL files to AString. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/079e92879710
Remove C++ support for, and testing of, the XPIDL DOMString type. r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/77dfc06a7c78
Remove support for DOMString from the XPIDL compiler. r=nika

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/31941d904a7c
https://hg.mozilla.org/mozilla-central/rev/079e92879710
https://hg.mozilla.org/mozilla-central/rev/77dfc06a7c78