Closed Bug 1299321 Opened 6 years ago Closed 6 years ago

Define @@toStringTag on Promise


(Core :: JavaScript: Standard Library, defect)

Not set



Tracking Status
firefox51 --- fixed


(Reporter: evilpie, Assigned: evilpie)



(Keywords: dev-doc-complete)


(4 files)

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.
Assignee: nobody → evilpies
I hacked up some ClassSpec and Xray stuff for string values that seems to work good enough for this.
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.
Attachment #8787223 - Flags: review?(jwalden+bmo)
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 ...
Attachment #8787239 - Flags: review?(peterv)
This is quite succinct now \o/
Attachment #8787240 - Flags: review?(till)
Comment on attachment 8787240 [details] [diff] [review]
Add @@toStringTag to Promise

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

Attachment #8787240 - Flags: review?(till) → review+
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 Promise]", "second toString result was incorrect");

r=me with something like that.  Certainly you should not need any async stuff in this test.
Attachment #8787241 - Flags: review?(bzbarsky) → review+
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*.
Attachment #8787223 - Flags: review?(jwalden+bmo) → review+
Attachment #8787239 - Flags: review?(peterv) → review+
Pushed by
Implement string values for JSPropertySpec. r=Waldo
Make DOM/Xray handle string values in JSPropertySpec. r=peterv
Add @@toStringTag to Promise. r=till
DOM test changes for Promise @@toStringTag. r=bz
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?
You need to log in before you can comment on or make changes to this bug.