[webkit-reviews] review granted: [Bug 190094] Make crossOriginObject.then undefined for promises : [Attachment 351126] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 190094: Make crossOriginObject.then undefined for promises
https://bugs.webkit.org/show_bug.cgi?id=190094

Attachment 351126: Patch

https://bugs.webkit.org/attachment.cgi?id=351126&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 351126
  --> https://bugs.webkit.org/attachment.cgi?id=351126
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:177
> +    if (propertyName == vm.propertyNames->builtinNames().thenPublicName() ||
propertyName == vm.propertyNames->toStringTagSymbol || propertyName ==
vm.propertyNames->hasInstanceSymbol || propertyName ==
vm.propertyNames->isConcatSpreadableSymbol) {

I would like it if we had a named function for this check. It would have two
benefits: 1) Could put *vm.propertyNames into a local variable to make the code
more readable, although we could also do that here. 2) The name of the function
could help make it clear what these names have in common.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:408
> +    propertyNames.add(vm.propertyNames->builtinNames().thenPublicName());
>      propertyNames.add(vm.propertyNames->toStringTagSymbol);
>      propertyNames.add(vm.propertyNames->hasInstanceSymbol);
>      propertyNames.add(vm.propertyNames->isConcatSpreadableSymbol);

Would be even better if somehow the list of property names existed only once,
and could be used both for the if statement above and this adding here.

> Source/WebCore/bindings/js/JSLocationCustom.cpp:58
> +    if (propertyName == vm.propertyNames->builtinNames().thenPublicName() ||
propertyName == vm.propertyNames->toStringTagSymbol || propertyName ==
vm.propertyNames->hasInstanceSymbol || propertyName ==
vm.propertyNames->isConcatSpreadableSymbol) {

Annoying that this repeats the same list but does not share code with
DOMWindow. Would be nice to fix that.

> Source/WebCore/bindings/js/JSLocationCustom.cpp:185
> +    propertyNames.add(vm.propertyNames->builtinNames().thenPublicName());
>      propertyNames.add(vm.propertyNames->toStringTagSymbol);
>      propertyNames.add(vm.propertyNames->hasInstanceSymbol);
>      propertyNames.add(vm.propertyNames->isConcatSpreadableSymbol);

Ditto.