Bug 214990

Created attachment 405628 [details]
Patch

Comment on attachment 405628 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:55
> +ExceptionOr<Ref<AudioBuffer>> AudioBuffer::create(const AudioBufferOptions&& options)

no const.

> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:67
> +    if (!buffer->m_length)

I'd rather we use the length() getter.

> Source/WebCore/Modules/webaudio/AudioBuffer.h:45
> +    static ExceptionOr<Ref<AudioBuffer>> create(const AudioBufferOptions&& = { });

no const

> Source/WebCore/Modules/webaudio/AudioBufferOptions.h:33
> +    unsigned numberOfChannels;

Please provide default values for these. Otherwise, when you used AudioBufferOptions { } earlier in your patch as default parameter value, it constructed a struct with uninitialized members.

> Source/WebCore/Modules/webaudio/AudioBufferOptions.h:34
> +    size_t length;

unsigned

> Source/WebCore/Modules/webaudio/OfflineAudioContextOptions.idl:25
> + 

Unnecessary change.

> LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-channels-expected.txt:9
> +FAIL X source.buffer = new buffer did not throw an exception. assert_true: expected true got false

This seems bad?

> LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-channels-expected.txt:11
> +FAIL X source.buffer = buffer again did not throw an exception. assert_true: expected true got false

how about this?

(In reply to Chris Dumez from comment #2)
> Comment on attachment 405628 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405628&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioBuffer.cpp:55
> > +ExceptionOr<Ref<AudioBuffer>> AudioBuffer::create(const AudioBufferOptions&& options)
> 
> no const.
> 
> > Source/WebCore/Modules/webaudio/AudioBuffer.cpp:67
> > +    if (!buffer->m_length)
> 
> I'd rather we use the length() getter.
> 
> > Source/WebCore/Modules/webaudio/AudioBuffer.h:45
> > +    static ExceptionOr<Ref<AudioBuffer>> create(const AudioBufferOptions&& = { });
> 
> no const
> 
> > Source/WebCore/Modules/webaudio/AudioBufferOptions.h:33
> > +    unsigned numberOfChannels;
> 
> Please provide default values for these. Otherwise, when you used
> AudioBufferOptions { } earlier in your patch as default parameter value, it
> constructed a struct with uninitialized members.
> 
> > Source/WebCore/Modules/webaudio/AudioBufferOptions.h:34
> > +    size_t length;
> 
> unsigned
> 
> > Source/WebCore/Modules/webaudio/OfflineAudioContextOptions.idl:25
> > + 
> 
> Unnecessary change.
> 
> > LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-channels-expected.txt:9
> > +FAIL X source.buffer = new buffer did not throw an exception. assert_true: expected true got false
> 
> This seems bad?

I think this is testing AudioBufferSourceNode::setBuffer() method, which will be fixed in a future patch.

> 
> > LayoutTests/imported/w3c/web-platform-tests/webaudio/the-audio-api/the-audiobuffersourcenode-interface/audiobuffersource-channels-expected.txt:11
> > +FAIL X source.buffer = buffer again did not throw an exception. assert_true: expected true got false
> 
> how about this?

Ditto above

Created attachment 405635 [details]
Patch

Comment on attachment 405635 [details]
Patch

LGTM but some related tests are failing in EWS.

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

> Source/WebCore/Modules/webaudio/AudioBuffer.idl:36
>      readonly attribute long length; // in sample-frames

Seems strange that we pass an unsigned long length in AudioBufferOptions but (probably) add a getter for the same value as long.
Probably we want it to be unsigned long as well given its name.

> Source/WebCore/Modules/webaudio/AudioBufferOptions.h:34
> +    unsigned length;

We are using unsigned here, as per WebIDL I believe, but size_t at some point for instance in AudioBuffer::create.
This is probably fine for this patch but I wonder whether we should try to be more consistent in the type used for length.

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:395
> +    return AudioBuffer::create(WTFMove(options));

No need to move options

> LayoutTests/imported/w3c/ChangeLog:19
> +        * web-platform-tests/webaudio/the-audio-api/the-oscillatornode-interface/osc-basic-waveform-expected.txt:

Nice to see all these newly passing tests.

(In reply to youenn fablet from comment #5)
> Comment on attachment 405635 [details]
> Patch
> 
> LGTM but some related tests are failing in EWS.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405635&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioBuffer.idl:36
> >      readonly attribute long length; // in sample-frames
> 
> Seems strange that we pass an unsigned long length in AudioBufferOptions but
> (probably) add a getter for the same value as long.
> Probably we want it to be unsigned long as well given its name.
> 

Yup, and according to spec it should be unsigned long as well, doesn't hurt to add in this patch I guess.

> > Source/WebCore/Modules/webaudio/AudioBufferOptions.h:34
> > +    unsigned length;
> 
> We are using unsigned here, as per WebIDL I believe, but size_t at some
> point for instance in AudioBuffer::create.
> This is probably fine for this patch but I wonder whether we should try to
> be more consistent in the type used for length.
> 

Right, should agree on something.

> > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:395
> > +    return AudioBuffer::create(WTFMove(options));
> 
> No need to move options

Got it.

> 
> > LayoutTests/imported/w3c/ChangeLog:19
> > +        * web-platform-tests/webaudio/the-audio-api/the-oscillatornode-interface/osc-basic-waveform-expected.txt:
> 
> Nice to see all these newly passing tests.

Just from a simple constructor :)

Created attachment 405691 [details]
Patch

Created attachment 405693 [details]
Patch

Resubmitted because old patch accidentally capitalized imported in LayoutTests/TestExpectations

Comment on attachment 405693 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:55
> +ExceptionOr<Ref<AudioBuffer>> AudioBuffer::create(const AudioBufferOptions& options)

We should try to remove the other AudioBuffer::create and just use this new one as a follow-up patch.
That will remove some potentially duplicated code.

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:395
> +    return AudioBuffer::create(options);

This could probably be made as a one-liner.

Created attachment 405698 [details]
Patch

(In reply to youenn fablet from comment #10)
> Comment on attachment 405693 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405693&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioBuffer.cpp:55
> > +ExceptionOr<Ref<AudioBuffer>> AudioBuffer::create(const AudioBufferOptions& options)
> 
> We should try to remove the other AudioBuffer::create and just use this new
> one as a follow-up patch.
> That will remove some potentially duplicated code.

Got it, there are some dependencies in files such as ScriptProcessorNode.

> 
> > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:395
> > +    return AudioBuffer::create(options);
> 
> This could probably be made as a one-liner.

Updated this, also changed length in AudioBufferOptions.h back to size_t to support this change.

(In reply to Clark Wang from comment #12)
> (In reply to youenn fablet from comment #10)
> > Comment on attachment 405693 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=405693&action=review
> > 
> > > Source/WebCore/Modules/webaudio/AudioBuffer.cpp:55
> > > +ExceptionOr<Ref<AudioBuffer>> AudioBuffer::create(const AudioBufferOptions& options)
> > 
> > We should try to remove the other AudioBuffer::create and just use this new
> > one as a follow-up patch.
> > That will remove some potentially duplicated code.
> 
> Got it, there are some dependencies in files such as ScriptProcessorNode.
> 
> > 
> > > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:395
> > > +    return AudioBuffer::create(options);
> > 
> > This could probably be made as a one-liner.
> 
> Updated this, also changed length in AudioBufferOptions.h back to size_t to
> support this change.

If you want to align one way, align to unsigned, not size_t.

Created attachment 405704 [details]
Patch

(In reply to Chris Dumez from comment #13)
> (In reply to Clark Wang from comment #12)
> > (In reply to youenn fablet from comment #10)
> > > Comment on attachment 405693 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=405693&action=review
> > > 
> > > > Source/WebCore/Modules/webaudio/AudioBuffer.cpp:55
> > > > +ExceptionOr<Ref<AudioBuffer>> AudioBuffer::create(const AudioBufferOptions& options)
> > > 
> > > We should try to remove the other AudioBuffer::create and just use this new
> > > one as a follow-up patch.
> > > That will remove some potentially duplicated code.
> > 
> > Got it, there are some dependencies in files such as ScriptProcessorNode.
> > 
> > > 
> > > > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:395
> > > > +    return AudioBuffer::create(options);
> > > 
> > > This could probably be made as a one-liner.
> > 
> > Updated this, also changed length in AudioBufferOptions.h back to size_t to
> > support this change.
> 
> If you want to align one way, align to unsigned, not size_t.


Modified the code to align to unsigned.

Comment on attachment 405704 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioBufferOptions.h:34
> +    unsigned length;

Needs an initializer

> Source/WebCore/Modules/webaudio/AudioBufferOptions.h:35
> +    float sampleRate;

Needs an initializer

Created attachment 405833 [details]
Patch

(In reply to Chris Dumez from comment #16)
> Comment on attachment 405704 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405704&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioBufferOptions.h:34
> > +    unsigned length;
> 
> Needs an initializer
> 
> > Source/WebCore/Modules/webaudio/AudioBufferOptions.h:35
> > +    float sampleRate;
> 
> Needs an initializer

Initialized these two members to the smallest values that are supported. Also made AudioBufferOptions a non-optional parameter in AudioBuffer to properly match spec.

Created attachment 405846 [details]
Patch

(In reply to Clark Wang from comment #19)
> Created attachment 405846 [details]
> Patch

Rebased, hopefully builds now.

Committed r265210: <https://trac.webkit.org/changeset/265210>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405846 [details].

<rdar://problem/66489353>