Closed Bug 1472170 Opened 4 years ago Closed 3 years ago
.prototype .description proposal
The Symbol.prototype.description proposal was moved to stage 3 in the May meeting . Test262 tests are already present , but we first need to update our local test262 copy to have them available. I'm setting the "good-first-bug" label, because the proposal seems to be relatively easy to implement (only a single accessor property needs to be added to Symbol.prototype).  https://github.com/tc39/tc39-notes/blob/master/es9/2018-05/may-22.md#symbolprototypedescription-for-stage-3  https://github.com/tc39/test262/tree/master/test/built-ins/Symbol/prototype/description
Symbol.prototype methods are declared here  and then implemented in the corresponding cpp-file js/src/builtin/Symbol.cpp. The implementation is added to the Symbol.prototype object by adding it to the (currently empty) |SymbolObject::properties| field . For example the DataView.prototype.buffer accessor property shows how to use |JSPropertySpec|s [3,4].  https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/Symbol.h#50-55  https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/Symbol.cpp#33-35  https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/DataViewObject.cpp#836-849  https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/DataViewObject.cpp#963
This seems like an easy change. The tests seem to be currently available in the local test262 copy. From what I understand, JS_PSG("description", toString, 0) needs to be added as a SymbolObject property in Symbol.cpp. Am I understanding correctly that this is all that needs to be done? If so, I can submit the patch.
Yes, that's almost all that's needed here. Except we can't reuse the existing |SymbolObject::toString| method, but instead need to create a new |SymbolObject::description| method (and its companion method |SymbolObject::description_impl|), because |toString| returns the 'descriptive-string' . For example for |Symbol("my-string-description")|, |SymbolObject::toString| returns "Symbol(my-string-description)", whereas the new 'description' getter is supposed to just return "my-string-description". And when the Symbol was created without a description |Symbol()|, the 'description' getter needs to return |undefined|.  https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/js/src/builtin/Symbol.cpp#192
Ok, I'll fix it up. Thanks for the helpful comments and pointers!
I fixed Symbol.cpp and updated the tests. I wasn't sure if it should all be in one patch or separate for the code and for the tests. Let me know if you see anything else that needs to be fixed or if I should separate it into two patches.
Attachment #9004024 - Attachment description: Modifies Symbol.h, Symbol.cpp to add description property and update tests → Modifies Symbol.h, Symbol.cpp to add description property and update tests.
Comment on attachment 9004024 [details] [diff] [review] Modifies Symbol.h, Symbol.cpp to add description property and update tests. Review of attachment 9004024 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to omersid from comment #7) > I fixed Symbol.cpp and updated the tests. I wasn't sure if it should all be > in one patch or separate for the code and for the tests. Let me know if you > see anything else that needs to be fixed or if I should separate it into two > patches. It's okay to keep all changes in a single patch, because the test changes are relatively small, so in my opinion it's not necessary to split them off. There are only two small issues which need to be fixed and the commit message needs to be updated to include the reviewer , i.e. "r=anba" needs to added. I've already r+'ed the patch, because the two remaining issues are only small nits which don't affect the overall review. And I've also pushed the patch to the try-server  to ensure all tests pass when the new property is added to `Symbol.prototype`. If the try-server push comes back green and the updated patch was uploaded, the patch can be marked as ready to be checked into the Firefox source repository.  https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3b4ef01b74a1eed429ecd8199029910bf4245f9 ::: js/src/builtin/Symbol.cpp @@ +249,5 @@ > + args.rval().setUndefined(); > + return true; > +} > + > +bool Please remove the trailing whitespace after 'bool'. ::: js/src/builtin/Symbol.h @@ +53,5 @@ > static MOZ_MUST_USE bool valueOf_impl(JSContext* cx, const CallArgs& args); > static MOZ_MUST_USE bool valueOf(JSContext* cx, unsigned argc, Value* vp); > static MOZ_MUST_USE bool toPrimitive(JSContext* cx, unsigned argc, Value* vp); > > + // Properties defined on Symbol.protoype. Nit: Typo in 'protoype' -> 'prototype'
Attachment #9004024 - Flags: review?(andrebargull) → review+
Assignee: nobody → omersid
Status: NEW → ASSIGNED
My bad for not noticing those things the last time through- they have been fixed.
Comment on attachment 9004451 [details] [diff] [review] Modifies Symbol.h, Symbol.cpp to add description property and update tests. Review of attachment 9004451 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Thank you! :-)
Attachment #9004451 - Flags: review?(andrebargull) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/709590a1a24f Added description as a property for Symbol and updated tests262. r=anba
You need to log in before you can comment on or make changes to this bug.