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>