Specify default parameter values for callback parameters.
Created attachment 277717 [details] Patch
Comment on attachment 277717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277717&action=review > Source/WebCore/Modules/geolocation/Geolocation.idl:33 > + optional PositionErrorCallback? errorCallback = null, Do we need to have an explicit "null" default value? Also PositionErrorCallback should probably not be marked nullable. Should we add a FIXME? Is there a way to not change at all the IDL? > Source/WebCore/Modules/notifications/Notification.h:127 > + static void requestPermission(Document&, RefPtr<NotificationPermissionCallback>&& = nullptr); Do we need = nullptr? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3472 > + die "Optional parameters of non-nullable wrapper types are not supported" if $optional && !$parameter->isNullable && $codeGenerator->IsWrapperType($argType); Can we die only in the cases where optional parameters do not have default values? If so, can we move this line after the code below that gives implicit default parameter values with an additional check of available default value? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3488 > + # For callback parameters, we treat the generated bindings treat undefined as null, so use null as implicit default value. s/we treat//
Comment on attachment 277717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277717&action=review >> Source/WebCore/Modules/geolocation/Geolocation.idl:33 >> + optional PositionErrorCallback? errorCallback = null, > > Do we need to have an explicit "null" default value? > > Also PositionErrorCallback should probably not be marked nullable. > Should we add a FIXME? Is there a way to not change at all the IDL? Oh, the default implicit value for nullable parameters is null so we don't technically need to update the IDL. I merely thought it was nice to be explicit in the IDL. I could be convinced otherwise though. >> Source/WebCore/Modules/notifications/Notification.h:127 >> + static void requestPermission(Document&, RefPtr<NotificationPermissionCallback>&& = nullptr); > > Do we need = nullptr? Probably not, I'll check. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3472 >> + die "Optional parameters of non-nullable wrapper types are not supported" if $optional && !$parameter->isNullable && $codeGenerator->IsWrapperType($argType); > > Can we die only in the cases where optional parameters do not have default values? > If so, can we move this line after the code below that gives implicit default parameter values with an additional check of available default value? Not really. For e.g. the following: void open(optional Node n = null); It has a default value and yet the bindings generator is not able to deal with this because the type is not nullable (We will throw if the parameter is omitted, which is probably not what the developer wants). >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3488 >> + # For callback parameters, we treat the generated bindings treat undefined as null, so use null as implicit default value. > > s/we treat// oops.
Created attachment 277731 [details] Patch
Comment on attachment 277731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277731&action=review > Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.h:49 > + DOMWindowWebDatabase() { } > + ~DOMWindowWebDatabase() { } Shouldn’t these be deleted instead? > Source/WebCore/Modules/webdatabase/Database.h:67 > - PassRefPtr<SQLTransactionBackend> runTransaction(PassRefPtr<SQLTransaction>, bool readOnly, const ChangeVersionData*); > + PassRefPtr<SQLTransactionBackend> runTransaction(Ref<SQLTransaction>&&, bool readOnly, const ChangeVersionData*); Since you are touching this, I would suggest changing the return value to return a RefPtr rather than a PassRefPtr.
Comment on attachment 277717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277717&action=review >>> Source/WebCore/Modules/geolocation/Geolocation.idl:33 >>> + optional PositionErrorCallback? errorCallback = null, >> >> Do we need to have an explicit "null" default value? >> >> Also PositionErrorCallback should probably not be marked nullable. >> Should we add a FIXME? Is there a way to not change at all the IDL? > > Oh, the default implicit value for nullable parameters is null so we don't technically need to update the IDL. I merely thought it was nice to be explicit in the IDL. I could be convinced otherwise though. I think it’s nicer to be brief instead of writing all those "= null". I think we should return and remove them.
Created attachment 277797 [details] Patch
Created attachment 277798 [details] Patch
Comment on attachment 277798 [details] Patch Clearing flags on attachment: 277798 Committed r200289: <http://trac.webkit.org/changeset/200289>
All reviewed patches have been landed. Closing bug.