[WebIDL] Support optional Promise arguments
Created attachment 332898 [details] Patch
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.
rdar://problem/36754552
(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.
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 ?
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
(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.
Created attachment 333198 [details] Patch
Committed r228189: <https://trac.webkit.org/r228189>