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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 6 10:56:29 PST 2018

Chris Dumez <cdumez at apple.com> has asked  for review:
Bug 182399: [WebIDL] Support optional Promise arguments

Attachment 332898: Patch


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

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

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

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

More information about the webkit-reviews mailing list