Bug 182399 – [WebIDL] Support optional Promise arguments

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[WebIDL] Support optional Promise arguments

[Andy Estes]

Created attachment 332898 [details]
Patch

[Saam Barati]

Comment on attachment 332898 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:6282
> +    auto promise = state->argument(0).isUndefined() ? nullptr : convert<IDLPromise<IDLVoid>>(*state, state->uncheckedArgument(0));

Seems weird to do argument(0) followed by uncheckedArgument(0) here. I think it's probably semantically correct (you can't have non-undefined passed unless an argument was passed there), but I'd just use argument(0) twice. The compiler should eliminate the second bounds check anyways.

[Andy Estes]

rdar://problem/36754552

[Andy Estes]

(In reply to Saam Barati from comment #2)
> Comment on attachment 332898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332898&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:6282
> > +    auto promise = state->argument(0).isUndefined() ? nullptr : convert<IDLPromise<IDLVoid>>(*state, state->uncheckedArgument(0));
> 
> Seems weird to do argument(0) followed by uncheckedArgument(0) here. I think
> it's probably semantically correct (you can't have non-undefined passed
> unless an argument was passed there), but I'd just use argument(0) twice.
> The compiler should eliminate the second bounds check anyways.

Ok, I'll look to do this in a follow-up. This change will update a bunch of bindings test expectations and I'd like to keep this patch small for merging purposes.

[Chris Dumez]

Comment on attachment 332898 [details]
Patch

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

Change seems fine since Promise types cannot be nullable as per Web IDL spec (therefore, we will never need to distinguish null parameter from missing/undefined parameter). A few comments though.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1540
> +    return "WTFMove(${name})" if $type->isNullable || (ref($context) eq "IDLArgument" && $context->isOptional);

Could we instead use below:
return "${name}.releaseNonNull()" if $codeGenerator->IsCallbackInterface($type) || $codeGenerator->IsCallbackFunction($type) || ($codeGenerator->IsPromiseType($type) && !$context->isOptional);

I do not like that this line is not Promise type specific.

>>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:6282
>>> +    auto promise = state->argument(0).isUndefined() ? nullptr : convert<IDLPromise<IDLVoid>>(*state, state->uncheckedArgument(0));
>> 
>> Seems weird to do argument(0) followed by uncheckedArgument(0) here. I think it's probably semantically correct (you can't have non-undefined passed unless an argument was passed there), but I'd just use argument(0) twice. The compiler should eliminate the second bounds check anyways.
> 
> Ok, I'll look to do this in a follow-up. This change will update a bunch of bindings test expectations and I'd like to keep this patch small for merging purposes.

I would keep using uncheckedArgument(), we do this on purpose in the bindings generator.

Does this build though? is the compiler able to figure out the auto type given the ternary and nullptr ?

[WebKit Commit Bot]

Comment on attachment 332898 [details]
Patch

Rejecting attachment 332898 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 332898, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/6384993

[Andy Estes]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 332898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332898&action=review
> 
> Change seems fine since Promise types cannot be nullable as per Web IDL spec
> (therefore, we will never need to distinguish null parameter from
> missing/undefined parameter). A few comments though.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1540
> > +    return "WTFMove(${name})" if $type->isNullable || (ref($context) eq "IDLArgument" && $context->isOptional);
> 
> Could we instead use below:
> return "${name}.releaseNonNull()" if
> $codeGenerator->IsCallbackInterface($type) ||
> $codeGenerator->IsCallbackFunction($type) ||
> ($codeGenerator->IsPromiseType($type) && !$context->isOptional);
> 
> I do not like that this line is not Promise type specific.

We could do that, but I think the WTFMove() is right for all optionals.

> 
> >>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:6282
> >>> +    auto promise = state->argument(0).isUndefined() ? nullptr : convert<IDLPromise<IDLVoid>>(*state, state->uncheckedArgument(0));
> >> 
> >> Seems weird to do argument(0) followed by uncheckedArgument(0) here. I think it's probably semantically correct (you can't have non-undefined passed unless an argument was passed there), but I'd just use argument(0) twice. The compiler should eliminate the second bounds check anyways.
> > 
> > Ok, I'll look to do this in a follow-up. This change will update a bunch of bindings test expectations and I'd like to keep this patch small for merging purposes.
> 
> I would keep using uncheckedArgument(), we do this on purpose in the
> bindings generator.
> 
> Does this build though? is the compiler able to figure out the auto type
> given the ternary and nullptr ?

Yes, it builds.

[Andy Estes]

Created attachment 333198 [details]
Patch

[Andy Estes]

Committed r228189: <https://trac.webkit.org/r228189>