[webkit-reviews] review requested: [Bug 79701] Add playback state for AudioBufferSourceNode and add number of active nodes : [Attachment 130169] Patch

[bugzilla-daemon at webkit.org]

Raymond Toy <rtoy at chromium.org> has asked  for review:
Bug 79701: Add playback state for AudioBufferSourceNode and add number of
active nodes
https://bugs.webkit.org/show_bug.cgi?id=79701

Attachment 130169: Patch
https://bugs.webkit.org/attachment.cgi?id=130169&action=review

------- Additional Comments from Raymond Toy <rtoy at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130169&action=review


>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:117
>> +		|| (m_playbackState == FINISHED_STATE)
> 
> I think the logic in 116:117 can be made more clear if you simply check for
(m_playbackState == UNSCHEDULED_STATE || m_playbackState == FINISHED_STATE)

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:128
>> +		// SCHEDULED_STATE state to the PLAYING_STATE state.
> 
> "the SCHEDULED_STATE state to the PLAYING_STATE state" -> "SCHEDULED_STATE to
PLAYING_STATE"

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:356
>> +	    // Finished playing, so decrement the active source count.
> 
> WebKit style is to avoid redundant comments.

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:49
>> +	// UNSCHEDULED_STATE - Initial playback state. Node created, but not
yet scheduled.
> 
> "Node created" -> "Created"

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:51
>> +	// PLAYING_STATE - The buffer source is currently playing
> 
> "The buffer source is currently playing" -> "Generating sound."

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:52
>> +	// FINISHED_STATE - The buffer source has finished playing
> 
> "The buffer source has finished playing" -> "Finished generating sound."

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:87
>> +	// Playback state
> 
> WebKit style is to avoid redundant comments such as this

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:162
>> +	// Playback state.  
> 
> WebKit style is to avoid redundant comments such as this

Done.

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:163
>> +	enum PlaybackState m_playbackState;
> 
> the "enum" keyword is not needed here

Done.

>>>> Source/WebCore/webaudio/AudioContext.h:99
>>>> +	  unsigned int activeSourceCount() { return
static_cast<unsigned>(m_activeSourceCount); }
>>> 
>>> "unsigned int" -> "unsigned long"
>> 
>> Aren't 4 billion active sources enough?
> 
> I was just trying to keep this exactly consistent with the .idl file which is
"unsigned long"

Done.

>> Source/WebCore/webaudio/AudioContext.idl:50
>> +	    readonly attribute unsigned int activeSourceCount;
> 
> "unsigned int" -> "unsigned long"

Done.

>> LayoutTests/webaudio/audiobuffersource-playbackState.html:98
>> +	// Test unscheduled state. Create 3 second source, but don't schedule
it.
> 
> These tests look great, but it looks like there's an awful lot of boilerplate
copy/paste here.  I think it would be better to create a function which takes:
> * impulse buffer duration
> * noteOn time -- if any
> * expected state
> 
> I think it's probably overkill to hand-code/construct separate PASS/FAIL
messages.  It's obvious to see if the test passed or failed (since it outputs
PASS or FAIL already) and
> we should be able to construct a generic message that makes sense for either
case in the function I'm proposing above.
> 
> If a function is used as I propose then I think this code becomes a lot
easier to follow.

Common code factored out, and messages simplified.