Bug 157188 – [Web IDL] Specify default parameter values for callback parameters

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Specify default parameter values for callback parameters.

[Chris Dumez]

Created attachment 277717 [details]
Patch

[youenn fablet]

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//

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 277731 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Chris Dumez]

Created attachment 277797 [details]
Patch

[Chris Dumez]

Created attachment 277798 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 277798 [details]
Patch

Clearing flags on attachment: 277798

Committed r200289: <http://trac.webkit.org/changeset/200289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.