Bug 205516 – [Bindings] Add @@toStringTag to our iterator prototype object

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Add @@ toStringTag to our iterator prototype object, as per:
- https://heycam.github.io/webidl/#es-iterator-prototype-object

[Chris Dumez]

Created attachment 386245 [details]
Patch

[Darin Adler]

Comment on attachment 386245 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386245&action=review

> Source/WebCore/bindings/js/JSDOMIterator.h:267
> +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(vm, info()->className), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);

I guess this is the same pattern that prototypes in the JavaScript runtime follow, so I guess it’s appropriately efficient.

Do we want to expose the actual values from className? No need to be indirect or hide that as an implementation detail?

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 386245 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386245&action=review
> 
> > Source/WebCore/bindings/js/JSDOMIterator.h:267
> > +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(vm, info()->className), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);
> 
> I guess this is the same pattern that prototypes in the JavaScript runtime
> follow, so I guess it’s appropriately efficient.
> 
> Do we want to expose the actual values from className? No need to be
> indirect or hide that as an implementation detail?

Using info()->className here is important to pass the tests. info()->className looks like "URLSearchParams Iterator" and is set in our generated bindings.

The spec says:
"""
The class string of an iterator prototype object for a given interface is the result of concatenating the identifier of the interface and the string " Iterator".
"""

[Chris Dumez]

Created attachment 386249 [details]
Patch

[Darin Adler]

Comment on attachment 386249 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386249&action=review

> Source/WebCore/bindings/js/JSDOMIterator.h:268
> +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(vm, info()->className), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);

JSC::PropertyAttribute perhaps?

[Chris Dumez]

Created attachment 386259 [details]
Patch

[Darin Adler]

Comment on attachment 386259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386259&action=review

> Source/WebCore/bindings/js/JSDOMIterator.h:268
> +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(vm, info()->className), JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);

Might also have to be JSC::jsString -- not really sure.

[WebKit Commit Bot]

Comment on attachment 386259 [details]
Patch

Clearing flags on attachment: 386259

Committed r253855: <https://trac.webkit.org/changeset/253855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58129948>

[Darin Adler]

Comment on attachment 386259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386259&action=review

>> Source/WebCore/bindings/js/JSDOMIterator.h:268
>> +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(vm, info()->className), JSC::PropertyAttribute::DontEnum | JSC::PropertyAttribute::ReadOnly);
> 
> Might also have to be JSC::jsString -- not really sure.

I figured out why it does not. The type of the first argument is JSC::VM&, and so argument-dependent lookup kicks in to search the JSC namespace for the function name.

[Darin Adler]

(In reply to Chris Dumez from comment #3)
> (In reply to Darin Adler from comment #2)
> > > Source/WebCore/bindings/js/JSDOMIterator.h:267
> > > +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol, jsString(vm, info()->className), PropertyAttribute::DontEnum | PropertyAttribute::ReadOnly);
> > 
> > I guess this is the same pattern that prototypes in the JavaScript runtime
> > follow, so I guess it’s appropriately efficient.
> > 
> > Do we want to expose the actual values from className? No need to be
> > indirect or hide that as an implementation detail?
> 
> Using info()->className here is important to pass the tests.
> info()->className looks like "URLSearchParams Iterator" and is set in our
> generated bindings.
> 
> The spec says:
> """
> The class string of an iterator prototype object for a given interface is
> the result of concatenating the identifier of the interface and the string "
> Iterator".
> """

Yes, I understand we need that name. What I wondered is whether it was OK to require that it be exactly equal to the className value from the info structure.

The four examples I found that have a className that is different from the toStringTagSymbol value are:

    AsyncGeneratorFunctionPrototype: className="AsyncGenerator", toStringTag="AsyncGeneratorFunction"
    JSDataViewPrototype: className="DataViewPrototype", toStringTag="DataView"
    JSModuleNamespaceObject: className="ModuleNamespaceObject", toStringTag="Module"
    JSPromisePrototype: className="PromisePrototype", toStringTag="Promise"

I was just wondering why this happens sometimes and why it shouldn't happen here. Maybe all four of those are just bugs.