Port Blink patch: https://codereview.chromium.org/26913005
Created attachment 241579 [details] patch
Eric, Jer, can you please review this patch?
Comment on attachment 241579 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=241579&action=review The Blink patch includes changes to a test as well. I see we don't have dom-exceptions.html at all, but could you create it with just the start() tests? > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:375 > + startPlaying(false, 0, 0, buffer()->duration(), ec); Looking a startPlaying call site I have no idea what the first parameter means. I think an enum would be much better.
Created attachment 241703 [details] patch Eric, I'm asking another review because I'm not sure I've chosen the best name for the added enum. Please let me know what you think or if you have better suggestions :)
Created attachment 241704 [details] patch Cleaned up the test a bit.
Comment on attachment 241704 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=241704&action=review Not sure this code has enough test coverage. > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:375 > + startPlaying(BufferPlaybackMode::Entire, 0, 0, buffer() ? buffer()->duration() : 0, ec); The syntax here doesn’t seem quite right. Since BufferPlaybackMode is just an enum, not an enum class, we don’t need the enum name qualifier. Maybe it was intended to be an enum class? > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:428 > + grainOffset = std::max(0.0, grainOffset); This std::max is not needed since we check above for grainOffset < 0. > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:430 > + m_grainOffset = grainOffset; This should just be written in a single line: m_grainOffset = std::min(bufferDuration, grainOffset); It’s also slightly clearer not to modify the passed in arguments. > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:433 > + grainDuration = std::max(0.0, grainDuration); This std::max is not needed since we check above for grainDuration < 0. > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:435 > + m_grainDuration = grainDuration; This should just be written in a single line: m_grainDuration = std::min(bufferDuration - m_grainOffset, grainDuration); It’s also slightly clearer not to modify the passed in arguments. > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:457 > + // Handle unspecified duration where 0 means the rest of the buffer. > + if (!grainDuration) > + grainDuration = buffer()->duration(); I don’t understand the mention of “unspecified” here and “0”. Where is the code that turns unspecified into 0? Or is 0 a magic value? > Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:184 > when = std::max<double>(0, when); This std::max is not needed now that we check "when < 0" above, and should be removed.
(In reply to comment #6) > Comment on attachment 241704 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241704&action=review > > Not sure this code has enough test coverage. > I think we need a major update of our WebAudio implementation in general. We've fallen quite behind the spec :( > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:375 > > + startPlaying(BufferPlaybackMode::Entire, 0, 0, buffer() ? buffer()->duration() : 0, ec); > > The syntax here doesn’t seem quite right. Since BufferPlaybackMode is just > an enum, not an enum class, we don’t need the enum name qualifier. Maybe it > was intended to be an enum class? > I somehow got the habit of using enums in C++ as enum classes but yeah I can drop the enum qualifier. > > Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp:457 > > + // Handle unspecified duration where 0 means the rest of the buffer. > > + if (!grainDuration) > > + grainDuration = buffer()->duration(); > > I don’t understand the mention of “unspecified” here and “0”. Where is the > code that turns unspecified into 0? Or is 0 a magic value? > A 0-duration means the whole buffer should be played, if I understand the spec correctly. Thanks for the review Darin, I'll upload the patch-for-landing so EWS can check it.
Created attachment 241793 [details] patch for landing
Comment on attachment 241793 [details] patch for landing Clearing flags on attachment: 241793 Committed r176311: <http://trac.webkit.org/changeset/176311>
All reviewed patches have been landed. Closing bug.