Bug 154666 – [Web IDL] Mark DOMString parameters as nullable when they should be

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

[Chris Dumez]

Mark DOMString parameters as nullable when they should be. We currently emulate nullable DOMString attributes by using [TreatNullAs=NullString, TreatUndefinedAs=NullString] but this is non-standard and very verbose. Also, sometimes developers forget the [TreatUndefinedAs=NullString] part and the behavior ends up being wrong for undefined.

With this done, we should be able to drop [TreatUndefinedAs=NullString] entirely. Only [TreatNullAs=NullString] remains and this one will be renamed to [TreatNullAs=EmptyString] via Bug 154654.

[Chris Dumez]

Created attachment 272174 [details]
Patch

[Chris Dumez]

Comment on attachment 272174 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272174&action=review

> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:103
> +    [RaisesException] RTCDataChannel createDataChannel([TreatNullAs=NullString] DOMString label, optional Dictionary options);

Matches the spec:
http://w3c.github.io/webrtc-pc/#rtcpeerconnection-interface-extensions-1

[Chris Dumez]

Created attachment 272251 [details]
Patch

[Chris Dumez]

Created attachment 272254 [details]
Patch

[Chris Dumez]

Now with a detailed changelog.

[Darin Adler]

Comment on attachment 272254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272254&action=review

> Source/WebCore/ChangeLog:51
> +        - The namespaceURI parameter to getElementsByTagNameNS() is now marked
> +          as nullable even though it only treated null as a null String, not
> +          undefined. This was a bug and did not match the specification:
> +          https://dom.spec.whatwg.org/#document

Should this change be covered by a test? Seems really easy to test!

> Source/WebCore/ChangeLog:62
> +        - The namespaceURI parameter to getElementsByTagNameNS() is now marked
> +          as nullable even though it only treated null as a null String, not
> +          undefined. This was a bug and did not match the specification:
> +          https://dom.spec.whatwg.org/#interface-element

Should this change be covered by a test? Seems really easy to test!

> Source/WebCore/ChangeLog:77
> +        - Make the first parameter to lookupPrefix() / isDefaultNamespace()
> +          and lookupNamespaceURI() nullable. Previously, we treated null
> +          as a null string but not undefined. There is therefore a slight
> +          behavior change but this matches the specification:
> +          https://dom.spec.whatwg.org/#node

Should this change be covered by a test? Seems really easy to test!

> Source/WebCore/ChangeLog:100
> +        * storage/StorageEvent.idl:
> +        Mark oldValueArg / newValueArg parameters to initStorageEvent()
> +        as nullable. Previously, we only treated null as a null string,
> +        not undefined. This is therefore a slight behavior change but
> +        it matches the specification:
> +        https://www.w3.org/TR/webstorage/#storageeventinit

Should this change be covered by a test? Seems really easy to test!

[Chris Dumez]

Created attachment 272282 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 272282 [details]
Patch

Clearing flags on attachment: 272282

Committed r197156: <http://trac.webkit.org/changeset/197156>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.