1299321 - Define @@toStringTag on Promise

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Tom S. (please needinfo tschuster)]

I wanted to do this, but it's surprisingly tricky. We purely define Promises with that ClassSpec setup, especially so that it works across compartments, however we currently don't have a way to define symbol properties with basic string values.

Comment 1

[Tom S. (please needinfo tschuster)]

I hacked up some ClassSpec and Xray stuff for string values that seems to work good enough for this.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: Implement string values for JSPropertySpec

This adds the option to have string values instead of just getters/setters in your JSPropertySpec. Pretty straight forward, if we want to support more types we probably have to increase the size of flags and stop using JSPROP_INTERNAL_USE_BIT.

Comment 3

[Tom S. (please needinfo tschuster)]

Attachment: Make DOM/Xray handle string values in JSPropertySpec

We have to change codegen a tiny bit, because of the new union and struct changes. The Xray code is basically a re-implementation of JS_DefineProperties ...

Comment 4

[Tom S. (please needinfo tschuster)]

Attachment: Add @@toStringTag to Promise

This is quite succinct now \o/

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: DOM changes and test for Promise @@toStringTag

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 8787240 [details] [diff] [review]
Add @@toStringTag to Promise

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

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8787241 [details] [diff] [review]
DOM changes and test for Promise @@toStringTag

>+  var p = new win.Promise(function(resolve, reject) { resolve(win.Promise.resolve(5)); });

I don't understand why you need all this complexity.  All you want is to check the result of toString() on an Xrayed promise, right?  How about this:

  is(win.Promise.prototype[Symbol.toStringTag], "Promise", "@@toStringTag was incorrect");
  var p = win.Promise.resolve();
  is(String(p), "[object Promise]", "String() result was incorrect");
  is(p.toString(), "[object Promise]", "toString result was incorrect");
  is(Object.prototype.toString.call(p), "[object Promise]", "second toString result was incorrect");
  nextTest();

r=me with something like that.  Certainly you should not need any async stuff in this test.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8787223 [details] [diff] [review]
Implement string values for JSPropertySpec

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

The horror...the horror...

::: js/src/jsapi.h
@@ +2009,5 @@
> +
> +
> +#define JS_PS_ACCESSOR_SPEC(name, getter, setter, flags, extraFlags) \
> +    { name, uint8_t(JS_CHECK_ACCESSOR_FLAGS(flags) | extraFlags), \
> +      { {  getter, setter  } } }

Move this above JS_PSG somewhere so that it's not being "used" "before" it's defined.  Preprocessing might not work like that, sure, but readers' minds do.

@@ +2012,5 @@
> +    { name, uint8_t(JS_CHECK_ACCESSOR_FLAGS(flags) | extraFlags), \
> +      { {  getter, setter  } } }
> +#define JS_PS_STRINGVALUE_SPEC(name, value, flags) \
> +    { name, uint8_t(flags | JSPROP_INTERNAL_USE_BIT), \
> +      { { STRINGVALUE_WRAPPER(value), JSNATIVE_WRAPPER(nullptr) } } }

And this up above JS_STRING*.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f45cd867c98
Implement string values for JSPropertySpec. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc2f0343294
Make DOM/Xray handle string values in JSPropertySpec. r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/f57b7dc26ed5
Add @@toStringTag to Promise. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/863f9f32c140
DOM test changes for Promise @@toStringTag. r=bz

Comment 10

[Tom S. (please needinfo tschuster)]

Thanks everyone for the awesome review times. This should be enough to cover everything that is trivially observable related to @@toStringTag. We still have to remove the fallback code (bug 1277799), however that requires some decisions and an implementation on the DOM side. XPConnect might need to start defining @@toString as well?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f45cd867c98
https://hg.mozilla.org/mozilla-central/rev/ddc2f0343294
https://hg.mozilla.org/mozilla-central/rev/f57b7dc26ed5
https://hg.mozilla.org/mozilla-central/rev/863f9f32c140

Comment 12

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/toStringTag