Bug 211256 – [JSC] intlBooleanOption should return TriState instead of taking an out param

• Component: New Bugs
• Status: RESOLVED FIXED

[Ross Kirsling]

[JSC] intlBooleanOption should return Optional<bool> instead of taking an out param

[Ross Kirsling]

Created attachment 398095 [details]
Patch

[Ross Kirsling]

Comment on attachment 398095 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlCollator.cpp:182
> -    JSValue options = optionsValue;
> -    if (!optionsValue.isUndefined()) {
> +    JSObject* options;
> +    if (optionsValue.isUndefined())
> +        options = constructEmptyObject(vm, globalObject->nullPrototypeObjectStructure());
> +    else {

The spec does say to construct an empty object here, but since every option will be undefined anyway, we technically could just keep a null JSObject*. Not sure if there's a preferred style for this.

[Darin Adler]

Comment on attachment 398095 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [JSC] intlBooleanOption should return Optional<bool> instead of taking an out param

Sam Weinig has advised us to not use Optional<bool> because it’s too easy to get wrong, with a subtle distinction between !x and !*x / !x.value() for example. I am OK with it personally, though.

>> Source/JavaScriptCore/runtime/IntlCollator.cpp:182
>> +    else {
> 
> The spec does say to construct an empty object here, but since every option will be undefined anyway, we technically could just keep a null JSObject*. Not sure if there's a preferred style for this.

I think a nullptr for JSObject is better: less pressure on the garbage collector, smaller code size. Would we have to add a lot of null checks? Another option would be to use a pointer to a known empty object.

[Yusuke Suzuki]

Comment on attachment 398095 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/IntlCollator.cpp:182
>>> +    else {
>> 
>> The spec does say to construct an empty object here, but since every option will be undefined anyway, we technically could just keep a null JSObject*. Not sure if there's a preferred style for this.
> 
> I think a nullptr for JSObject is better: less pressure on the garbage collector, smaller code size. Would we have to add a lot of null checks? Another option would be to use a pointer to a known empty object.

Isn't it observable if user sets `Object.prototype.someOptionName` getter? If it is observable, we need to create an object here.

[Ross Kirsling]

(In reply to Yusuke Suzuki from comment #4)
> Isn't it observable if user sets `Object.prototype.someOptionName` getter?
> If it is observable, we need to create an object here.

This particular case won't be observable since we're meant to create an object with null prototype.

[Ross Kirsling]

I guess the question here then is whether this is an improvement:

> -    if (options.isUndefined())
> +    if (!options)
>          return fallback;
> -    JSObject* opts = options.toObject(globalObject);
> -    RETURN_IF_EXCEPTION(scope, String());
> -
> -    JSValue value = opts->get(globalObject, property);
> +    JSValue value = options->get(globalObject, property);

Given that current JSC code style tends to assume that JSObject* params are non-null when not otherwise specified.

Perhaps I should just leave everything other than intlBooleanOption alone after all.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #6)
> Perhaps I should just leave everything other than intlBooleanOption alone
> after all.

* everything other than the Optional<bool> change, I mean.

[Ross Kirsling]

Based on Slack discussion with Mark and Yusuke, I'm going to change this to use WTF::TriState and then rename MixedTriState to IndeterminateTriState (following Boost) in a separate patch.

[Ross Kirsling]

Created attachment 398125 [details]
Patch

Asking for re-review just to be certain.

[Mark Lam]

Comment on attachment 398125 [details]
Patch

r=me

[Ross Kirsling]

Created attachment 398129 [details]
Patch for landing

[EWS]

Committed r260976: <https://trac.webkit.org/changeset/260976>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398129 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62689888>