Bug 156777 – Drop [UsePointersEvenForNonNullableObjectArguments] from WebAudio

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Drop [UsePointersEvenForNonNullableObjectArguments] from WebAudio

[Chris Dumez]

Created attachment 276797 [details]
WIP patch

[Chris Dumez]

Created attachment 276798 [details]
WIP patch

[Chris Dumez]

Created attachment 276799 [details]
Patch

[youenn fablet]

Comment on attachment 276799 [details]
Patch

Patch seems almost good to me.
I have some questions (see below) before going r+.

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

> Source/WebCore/ChangeLog:11
> +        No new tests, no intended Web-exposed behavioral change.

There are some changes to AudioContext.idl which can be observed by web content.
DO we want to add some tests to track these changes?

> Source/WebCore/Modules/webaudio/AsyncAudioDecoder.cpp:54
> +void AsyncAudioDecoder::decodeAsync(Ref<ArrayBuffer>&& audioData, float sampleRate, RefPtr<AudioBufferCallback>&& successCallback, RefPtr<AudioBufferCallback>&& errorCallback)

I guess successCallback should not be null, contrary to errorCallback that is probably optional.
Do we want to make that visible in the signature and binding generator by making successCallback a Ref&&.
Or just add an ASSERT(successCallback).

> Source/WebCore/Modules/webaudio/AudioContext.idl:64
> +    [RaisesException] AudioBuffer createBuffer(ArrayBuffer buffer, boolean mixToMono);

This is observable by the JS layer although probably ok not to test.

> Source/WebCore/Modules/webaudio/AudioContext.idl:69
>  

Do we really need that change?
This seems strange to have successCallback nullable or even optional.
How are we supposed to get the result of decodeAudioData then?

> Source/WebCore/Modules/webaudio/OscillatorNode.idl:58
> +    void setPeriodicWave(PeriodicWave? wave);

I am a bit reluctant to change the IDL since it is probably not conforming to the spec.
Is it complex to fix setPeriodicWave? Should we leave that to another bug?

[Chris Dumez]

Comment on attachment 276799 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        No new tests, no intended Web-exposed behavioral change.
> 
> There are some changes to AudioContext.idl which can be observed by web content.
> DO we want to add some tests to track these changes?

After double-checking, the only change I see is the type of the exception being thrown when passing null.

>> Source/WebCore/Modules/webaudio/AsyncAudioDecoder.cpp:54
>> +void AsyncAudioDecoder::decodeAsync(Ref<ArrayBuffer>&& audioData, float sampleRate, RefPtr<AudioBufferCallback>&& successCallback, RefPtr<AudioBufferCallback>&& errorCallback)
> 
> I guess successCallback should not be null, contrary to errorCallback that is probably optional.
> Do we want to make that visible in the signature and binding generator by making successCallback a Ref&&.
> Or just add an ASSERT(successCallback).

The currently implementation correctly handles a null success callback at the moment. I do not want to change the Web-Exposed behavior in this patch.
Also note that in the spec, both callback are optional and the operation returns a Promise.

>> Source/WebCore/Modules/webaudio/AudioContext.idl:64
>> +    [RaisesException] AudioBuffer createBuffer(ArrayBuffer buffer, boolean mixToMono);
> 
> This is observable by the JS layer although probably ok not to test.

Hmm, the implementation was doing:
ASSERT(arrayBuffer);
394     if (!arrayBuffer) {
395         ec = SYNTAX_ERR;
396         return nullptr;
397     }

So we still throw an exception when passing null. However, we now throw a TypeError instead of a SyntaxError. The new behavior is compliant with Web IDL. Would you like me to write a test to make sure the exception thrown is indeed a TypeError?

>> Source/WebCore/Modules/webaudio/AudioContext.idl:69
>>  
> 
> Do we really need that change?
> This seems strange to have successCallback nullable or even optional.
> How are we supposed to get the result of decodeAudioData then?

We need this change if we want to maintain the previous behavior. I did not want to change the Web-Exposed behavior.

>> Source/WebCore/Modules/webaudio/OscillatorNode.idl:58
>> +    void setPeriodicWave(PeriodicWave? wave);
> 
> I am a bit reluctant to change the IDL since it is probably not conforming to the spec.
> Is it complex to fix setPeriodicWave? Should we leave that to another bug?

Well, it is not complex but it is a web-exposed behavior change, which is something I do not want to do in this patch. Starting to throw an exception in a case where we previously did not is always a compatibility risk.

[Chris Dumez]

Created attachment 276828 [details]
Patch

[Darin Adler]

Comment on attachment 276799 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AsyncAudioDecoder.cpp:54
>> +void AsyncAudioDecoder::decodeAsync(Ref<ArrayBuffer>&& audioData, float sampleRate, RefPtr<AudioBufferCallback>&& successCallback, RefPtr<AudioBufferCallback>&& errorCallback)
> 
> I guess successCallback should not be null, contrary to errorCallback that is probably optional.
> Do we want to make that visible in the signature and binding generator by making successCallback a Ref&&.
> Or just add an ASSERT(successCallback).

Yes, that’s something worthwhile to return here and do later, with compatibility and test coverage in mind, but it’s fine that this patch does not change behavior right now.

>> Source/WebCore/Modules/webaudio/AudioContext.idl:64
>> +    [RaisesException] AudioBuffer createBuffer(ArrayBuffer buffer, boolean mixToMono);
> 
> This is observable by the JS layer although probably ok not to test.

The change is that a null will result in TypeError instead of SYNTAX_ERR. Not sure how important test coverage would be, but seems it would be easy to add to some existing test.

>> Source/WebCore/Modules/webaudio/AudioContext.idl:69
>>  
> 
> Do we really need that change?
> This seems strange to have successCallback nullable or even optional.
> How are we supposed to get the result of decodeAudioData then?

Same change here where we would get TypeError instead of SYNTAX_ERR when audioData is null. Same thinking on test coverage.

And yes, we should come back here and fix the callback nullability later; but I think it’s smart to correctly express what the code is currently doing without making a behavior change at the moment. We can come back and deal with the FIXME, considering the impact of changing web-visible behavior, later. See my comment below about why I think this is the right thing to do.

It’s urgent to get the UsePointersEvenForNonNullableObjectArguments flag removed and it’s fine to just have correctly document our current incorrect nullability behavior on various arguments that we can then carefully fix one-by-one.

> Source/WebCore/Modules/webaudio/AudioContext.idl:73
> +    [Conditional=VIDEO, RaisesException] MediaElementAudioSourceNode createMediaElementSource(HTMLMediaElement mediaElement);

Here null will result in TypeError instead of INVALID_STATE_ERR. Not critical to test, but not hard either.

> Source/WebCore/Modules/webaudio/AudioContext.idl:75
> +    [Conditional=MEDIA_STREAM, RaisesException] MediaStreamAudioSourceNode createMediaStreamSource(MediaStream mediaStream);

Here null will result in TypeError instead of INVALID_STATE_ERR. Not critical to test, but not hard either.

>> Source/WebCore/Modules/webaudio/OscillatorNode.idl:58
>> +    void setPeriodicWave(PeriodicWave? wave);
> 
> I am a bit reluctant to change the IDL since it is probably not conforming to the spec.
> Is it complex to fix setPeriodicWave? Should we leave that to another bug?

I don’t agree with your reluctance. I think Chris is handling this right.

A function that correctly states in the IDL how it currently behaves, with a FIXME explaining that the nullability behavior is wrong, is much better than a class with UsePointersEvenForNonNullableObjectArguments modifying the whole class, which can have an effect on future functions added to the class and that could affect multiple attributes and functions in a way that’s likely unclear to someone looking at the source file.

I don’t think we need a separate bug in bugs.webkit.org for each of these "FIXME: should not be nullable" and other FIXMEs about how our implementation doesn’t match the specifications. We can create those bugs with grep if we want to, but I think what Chris is doing here is the best practice; we should definitely come through and change behavior, but we should be very careful about changes that fail when the old code didn’t fail.

I think Chris is drawing the line in the right place where he’s OK changing exactly which exception is raised, but not adding new exceptions in this patch.

[Darin Adler]

Comment on attachment 276828 [details]
Patch

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

> Source/WebCore/Modules/webaudio/OscillatorNode.idl:58
> +    // FIXME: The parameter should not be nullable.
> +    void setPeriodicWave(PeriodicWave? wave);

I’ve been putting these FIXMEs at the end of the line rather than before the function. I think that’s clearer when there are multiple functions.

> LayoutTests/webaudio/audiobuffer-expected.txt:15
> +PASS context.createBuffer(null, false) threw exception TypeError: Type error.

I’d like to see these for the others cases where we changed the type of the exception, too. But I don’t think this has to be in the same check-in.

[Chris Dumez]

Created attachment 276829 [details]
Patch

[Chris Dumez]

(In reply to comment #8)
> Comment on attachment 276828 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276828&action=review
> 
> > Source/WebCore/Modules/webaudio/OscillatorNode.idl:58
> > +    // FIXME: The parameter should not be nullable.
> > +    void setPeriodicWave(PeriodicWave? wave);
> 
> I’ve been putting these FIXMEs at the end of the line rather than before the
> function. I think that’s clearer when there are multiple functions.

Ok, Done.

> 
> > LayoutTests/webaudio/audiobuffer-expected.txt:15
> > +PASS context.createBuffer(null, false) threw exception TypeError: Type error.
> 
> I’d like to see these for the others cases where we changed the type of the
> exception, too. But I don’t think this has to be in the same check-in.

Ok, I added test cases for the others as well.

[WebKit Commit Bot]

Comment on attachment 276829 [details]
Patch

Clearing flags on attachment: 276829

Committed r199775: <http://trac.webkit.org/changeset/199775>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> >> Source/WebCore/Modules/webaudio/OscillatorNode.idl:58
> >> +    void setPeriodicWave(PeriodicWave? wave);
> > 
> > I am a bit reluctant to change the IDL since it is probably not conforming to the spec.
> > Is it complex to fix setPeriodicWave? Should we leave that to another bug?
> 
> I don’t agree with your reluctance. I think Chris is handling this right.

I would find it better to:
1. remove UsePointersEvenForNonNullableObjectArguments from all interfaces
2. mark individually each method causing issues as UsePointersEvenForNonNullableObjectArguments.
That said, I can live with the FIXME approach.

[youenn fablet]

(In reply to comment #13)
> > >> Source/WebCore/Modules/webaudio/OscillatorNode.idl:58
> > >> +    void setPeriodicWave(PeriodicWave? wave);
> > > 
> > > I am a bit reluctant to change the IDL since it is probably not conforming to the spec.
> > > Is it complex to fix setPeriodicWave? Should we leave that to another bug?
> > 
> > I don’t agree with your reluctance. I think Chris is handling this right.
> 
> I would find it better to:
> 1. remove UsePointersEvenForNonNullableObjectArguments from all interfaces
> 2. mark individually each method causing issues as
> UsePointersEvenForNonNullableObjectArguments.
> That said, I can live with the FIXME approach.

I filed bug 156844