Bug 154666

Summary: [Web IDL] Mark DOMString parameters as nullable when they should be
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, ggaren, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 154654    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-02-24 20:55:37 PST
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.
Comment 1 Chris Dumez 2016-02-24 22:38:07 PST
Created attachment 272174 [details]
Patch
Comment 2 Chris Dumez 2016-02-24 22:39:32 PST
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
Comment 3 Chris Dumez 2016-02-25 15:25:34 PST
Created attachment 272251 [details]
Patch
Comment 4 Chris Dumez 2016-02-25 15:53:24 PST
Created attachment 272254 [details]
Patch
Comment 5 Chris Dumez 2016-02-25 15:53:53 PST
Now with a detailed changelog.
Comment 6 Darin Adler 2016-02-25 18:04:05 PST
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!
Comment 7 Chris Dumez 2016-02-25 19:46:43 PST
Created attachment 272282 [details]
Patch
Comment 8 WebKit Commit Bot 2016-02-25 20:36:33 PST
Comment on attachment 272282 [details]
Patch

Clearing flags on attachment: 272282

Committed r197156: <http://trac.webkit.org/changeset/197156>
Comment 9 WebKit Commit Bot 2016-02-25 20:36:37 PST
All reviewed patches have been landed.  Closing bug.