André Bargull [:anba] Reporter
	Description • 6 years ago

The Symbol.prototype.description proposal was moved to stage 3 in the May meeting [1]. Test262 tests are already present [2], 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). 


[1] https://github.com/tc39/tc39-notes/blob/master/es9/2018-05/may-22.md#symbolprototypedescription-for-stage-3
[2] https://github.com/tc39/test262/tree/master/test/built-ins/Symbol/prototype/description

	André Bargull [:anba] Reporter
	Comment 1 • 6 years ago

Symbol.prototype methods are declared here [1] 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 [2]. For example the DataView.prototype.buffer accessor property shows how to use |JSPropertySpec|s [3,4].


[1] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/Symbol.h#50-55
[2] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/Symbol.cpp#33-35
[3] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/DataViewObject.cpp#836-849
[4] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/builtin/DataViewObject.cpp#963

	omersid Assignee
	Comment 2 • 6 years ago

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.

	André Bargull [:anba] Reporter
	Comment 3 • 6 years ago

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' [1]. 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|.

[1] https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/js/src/builtin/Symbol.cpp#192

	André Bargull [:anba] Reporter
	Comment 5 • 6 years ago

Comment on attachment 9003842 [details] [diff] [review]
Modifies Symbol.h, Symbol.cpp to add description property

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

Looks almost complete! \o/

There only some minor style nits which need to be fixed and the return value when no description is present needs to be changed to `undefined`. When both issues are corrected, please upload a new patch and re-request review.


Actually there's another thing which needs to be done for this bug, namely we want to enable the tests from the ECMAScript conformance test suite <https://github.com/tc39/test262/> for `Symbol.prototype.description`. The tests are already present in the source tree, but they're currently disabled. To enable them this line [1] in the test262 update script needs to be removed and then the update script needs to be executed with:

  ~/hg/mozilla-inbound/js/src/tests$ ./test262-update.py --revision=ab436c465106be86719c4849c9cedecd7b570ff9

where ab436c465106be86719c4849c9cedecd7b570ff9 is the currently checked out revision [2]. (Or if you don't want to run the update script or if there are any problems when executing the script, it's also possible to manually edit the test files: In this directory [3], remove the line "// |reftest| skip -- Symbol.prototype.description is not supported" [4] in each file where it is present.)

[1] https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/js/src/tests/test262-update.py#33
[2] https://searchfox.org/mozilla-central/source/js/src/tests/test262/GIT-INFO#1
[3] https://searchfox.org/mozilla-central/source/js/src/tests/test262/built-ins/Symbol/prototype/description
[4] https://searchfox.org/mozilla-central/rev/c45b9755593ce6cc91f558b21e544f5165752de7/js/src/tests/test262/built-ins/Symbol/prototype/description/description-symboldescriptivestring.js#1

::: js/src/builtin/Symbol.cpp
@@ +234,5 @@
>  
> +bool
> +SymbolObject::descriptionGetter_impl(JSContext* cx, const CallArgs& args)
> +{
> +    //Get symbol object pointer   

Nit: Please add a space character after `//`, end the sentence with a full stop `.`, and remove the trailing whitespace.

@@ +248,5 @@
> +    if(str){
> +        if(!sb.append(str))
> +            return false;    
> +    }else{
> +        if(!sb.append("undefined"))

According to the spec proposal, the `[[Description]]` field of the symbol should be return directly, which means in SpiderMonkey when `sym->description()` is the null-pointer, we need to return the JavaScript `undefined` value instead of the string value `"undefined"`.

And you can also directly return |sym->description()| instead of creating a StringBuffer, so these lines can be reduced to:

```
// Return the symbol's description if present, otherwise return undefined.
if (JSString* str = sym->description())
    args.rval().setString(str);
else
    args.rval().setUndefined();

return true;
```

@@ +259,5 @@
> +    args.rval().setString(str);
> +    return true;
> +}
> +
> +bool 

Nit: Please remove the trailing whitespace here.

	omersid Assignee
	Comment 6 • 6 years ago

Ok, I'll fix it up. Thanks for the helpful comments and pointers!

	omersid Assignee
	Comment 7 • 6 years ago

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.

	André Bargull [:anba] Reporter
	Comment 8 • 6 years ago

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 [1], 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 [2] 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. 

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
[2] 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'

	omersid Assignee
	Comment 9 • 6 years ago

My bad for not noticing those things the last time through- they have been fixed.

	André Bargull [:anba] Reporter
	Comment 10 • 6 years ago

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! :-)

	André Bargull [:anba] Reporter
	Comment 11 • 6 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a5810aece11aba325bd0ccd374bdd14790d3adb

	Pulsebot
	Comment 12 • 6 years ago

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/709590a1a24f
Added description as a property for Symbol and updated tests262. r=anba

	Andrei Ciure[:aciure]
	Comment 13 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/709590a1a24f

	Florian Scholz (Open Web Docs)
	Comment 14 • 6 years ago

Developer release notes: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#JavaScript
New reference page: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/description
Compat data: https://github.com/mdn/browser-compat-data/pull/2693
Example: https://github.com/mdn/interactive-examples/pull/1126