Define @@toStringTag on Promise

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

({dev-doc-complete})

unspecified
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

3 years ago
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

Updated

3 years ago
Assignee: nobody → evilpies
Assignee

Comment 1

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

Comment 2

3 years ago
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)
Assignee

Comment 3

3 years ago
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)
Assignee

Comment 4

3 years ago
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]:
-----------------------------------------------------------------

r=me
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.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.
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+

Comment 9

3 years ago
Pushed by evilpies@gmail.com:
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
Assignee

Comment 10

3 years ago
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.