[webkit-reviews] review requested: [Bug 182399] [WebIDL] Support optional Promise arguments : [Attachment 332898] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has asked  for review:
Bug 182399: [WebIDL] Support optional Promise arguments
https://bugs.webkit.org/show_bug.cgi?id=182399

Attachment 332898: Patch

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




--- Comment #5 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 332898
  --> https://bugs.webkit.org/attachment.cgi?id=332898
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 ?