Bug 157188 - [Web IDL] Specify default parameter values for callback parameters
Summary: [Web IDL] Specify default parameter values for callback parameters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-29 10:25 PDT by Chris Dumez
Modified: 2016-04-29 22:40 PDT (History)
9 users (show)

See Also:


Attachments
Patch (43.32 KB, patch)
2016-04-29 10:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (43.30 KB, patch)
2016-04-29 13:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (42.21 KB, patch)
2016-04-29 21:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (42.27 KB, patch)
2016-04-29 21:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-04-29 10:25:16 PDT
Specify default parameter values for callback parameters.
Comment 1 Chris Dumez 2016-04-29 10:57:00 PDT
Created attachment 277717 [details]
Patch
Comment 2 youenn fablet 2016-04-29 13:26:15 PDT
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 3 Chris Dumez 2016-04-29 13:32:22 PDT
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.
Comment 4 Chris Dumez 2016-04-29 13:42:26 PDT
Created attachment 277731 [details]
Patch
Comment 5 Darin Adler 2016-04-29 21:26:05 PDT
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 6 Darin Adler 2016-04-29 21:27:27 PDT
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.
Comment 7 Chris Dumez 2016-04-29 21:46:27 PDT
Created attachment 277797 [details]
Patch
Comment 8 Chris Dumez 2016-04-29 21:49:24 PDT
Created attachment 277798 [details]
Patch
Comment 9 WebKit Commit Bot 2016-04-29 22:40:26 PDT
Comment on attachment 277798 [details]
Patch

Clearing flags on attachment: 277798

Committed r200289: <http://trac.webkit.org/changeset/200289>
Comment 10 WebKit Commit Bot 2016-04-29 22:40:32 PDT
All reviewed patches have been landed.  Closing bug.