Move the exception logic to the AudioNode creator

Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

The result should be that AbstractAudioContext::createFoo() does nothing
except call Foo::create() and returns the created node.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.

Committed: https://crrev.com/d3e0367d4b1f7637609f09f5fc3dbab97ae11b11
Cr-Commit-Position: refs/heads/master@{#395914}

[Raymond Toy, 2016-05-12 23:24:34]

Description was changed from

==========
Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.
==========

to

==========
Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

The result should be that AbstractAudioContext::createFoo() does nothing except
call Foo::create() and returns the created node.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.
==========

[Raymond Toy, 2016-05-12 23:26:48]

Description was changed from

==========
Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

The result should be that AbstractAudioContext::createFoo() does nothing except
call Foo::create() and returns the created node.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.
==========

to

==========
Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

The result should be that AbstractAudioContext::createFoo() does nothing
except call Foo::create() and returns the created node.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.
==========

[Raymond Toy, 2016-05-12 23:31:42]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2016-05-12 23:31:43]

PTAL

[hongchan, 2016-05-13 01:20:13]

I suggest we should go by Chromium style starting from this CL. You have used
the style here and there, and I think the pattern looks good.

However, there are some inconsistencies in terms of the style. Some parts of
code are still using WebKit style.

I do not have a strong opinion about it, but at least we can make them
consistent.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp:130:
ASSERT(isMainThread());
DCHECK?

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp:132: const
unsigned ChannelMergerDefaultNumberOfInputs = 6;
Can we add TODO here? - "Create a global config file that contains all the const
variable for WebAudio."

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp:138:
ASSERT(isMainThread());
Ditto.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ChannelSplitterNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelSplitterNode.cpp:85:
ASSERT(isMainThread());
Let's use DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelSplitterNode.cpp:93:
ASSERT(isMainThread());
Ditto.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:176:
ASSERT(isMainThread());
DCHECK?

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:178: if
(context.isContextClosed()) {
Perhaps we can make this to a method? This check-and-return pattern is almost
everywhere.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/DelayNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DelayNode.cpp:54:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DelayNode.cpp:62:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/DynamicsCompressorNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DynamicsCompressorNode.cpp:148:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/GainNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/GainNode.cpp:126:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:37:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:44: if
(feedbackCoef.size() == 0 || (feedbackCoef.size() > IIRFilter::kMaxOrder + 1)) {
This line is over 80 cols. What should we do? Wrapping function arguments as
shown other places implies we're using Chromium style, so I believe the CL
should be consistent with it.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:57: if
(feedforwardCoef.size() == 0 || (feedforwardCoef.size() > IIRFilter::kMaxOrder +
1)) {
Over 80 cols.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.cpp:214:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.cpp:225:
"HTMLMediaElement already connected previously to a different
MediaElementSourceNode.");
Over 80 cols. Might have to break this into multiple strings and join them with
'+'.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:127:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp:121:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp:140:
MediaStreamAudioSourceNode* node = new MediaStreamAudioSourceNode(context,
mediaStream, audioTrack, provider.release());
Over 80 cols. Let's wrap these arguments.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp:143:
// FIXME: Only stereo streams are supported right now. We should be able to
accept multi-channel streams.
Over 80 cols. Let's change it to 'TODO'. You can put my ID here.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp:149:
return node;
We should return nullptr when |node| is invalid and do the rest otherwise.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/OscillatorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OscillatorNode.cpp:340:
setHandler(OscillatorHandler::create(*this, context.sampleRate(),
m_frequency->handler(), m_detune->handler()));
Over 80 cols. Let's wrap this!

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OscillatorNode.cpp:343:
OscillatorNode* OscillatorNode::create(AbstractAudioContext& context,
ExceptionState& exceptionState)
Over 80 cols.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OscillatorNode.cpp:345:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/PannerNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/PannerNode.cpp:503: PannerNode*
PannerNode::create(AbstractAudioContext& context, ExceptionState&
exceptionState)
Over 80 cols.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/PannerNode.cpp:505:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp:59:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:266:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:276:
ASSERT(isMainThread());
Ditto.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:287:
ASSERT(isMainThread());
Ditto.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:309: "number
of input channels and output channels cannot both be zero.");
input channels and output channels => input and output channels

Then we can contain this within 80 cols.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:330: 
In the change above, we have few "80-col" issues. Perhaps we should discuss this
in person.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:352:
ScriptProcessorNode* node =  new ScriptProcessorNode(context,
context.sampleRate(), bufferSize, numberOfInputChannels,
numberOfOutputChannels);
Let's wrap arguments.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:355: //
context keeps reference until we stop making javascript rendering callbacks
If we move this comment above |if (node)| Then we can remove curly braces.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:359: return
node;
The same issue before - we should return node when it is valid. Otherwise we
should return nullptr.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:114:
ExceptionState&);
Let's make this consistent (by wrapping around 80 cols).

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/StereoPannerNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/StereoPannerNode.cpp:141:
setHandler(StereoPannerHandler::create(*this, context.sampleRate(),
m_pan->handler()));
80 cols.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/StereoPannerNode.cpp:144:
StereoPannerNode* StereoPannerNode::create(AbstractAudioContext& context,
ExceptionState& exceptionState)
Let's wrap arguments.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/StereoPannerNode.cpp:146:
ASSERT(isMainThread());
DCHECK.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/WaveShaperNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/WaveShaperNode.cpp:42:
WaveShaperNode* WaveShaperNode::create(AbstractAudioContext& context,
ExceptionState& exceptionState)
Let's wrap arguments.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/WaveShaperNode.cpp:44:
ASSERT(isMainThread());
DCHECK.

[Raymond Toy, 2016-05-13 15:59:51]

I'll fix the ASSERT issues, though since this is kind of new code.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp:132: const
unsigned ChannelMergerDefaultNumberOfInputs = 6;
On 2016/05/13 01:20:11, hoch wrote:
> Can we add TODO here? - "Create a global config file that contains all the
const
> variable for WebAudio."

Having a global for a constant that is used in exactly one place seems overkill.

Can I just hardwire the 6 into the call? (With a comment that 6 is the default.)

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:44: if
(feedbackCoef.size() == 0 || (feedbackCoef.size() > IIRFilter::kMaxOrder + 1)) {
On 2016/05/13 01:20:12, hoch wrote:
> This line is over 80 cols. What should we do? Wrapping function arguments as
> shown other places implies we're using Chromium style, so I believe the CL
> should be consistent with it.

Blink code is still governed by blink style.  Blink style also allows function
args to be wrapped if it helps readability.

Plus, I hate 80-col limit because it's too short.  I set my editor to wrap at
100 (or 120?) which I find to be a good compromise (for now) between 80 and
infinity.

[Raymond Toy, 2016-05-13 16:36:03]

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:178: if
(context.isContextClosed()) {
On 2016/05/13 01:20:11, hoch wrote:
> Perhaps we can make this to a method? This check-and-return pattern is almost
> everywhere.

I thought about that.  But we'd still have to do something like

if (context.throwIfContextClosed())
  return nullptr;

Only one line shorter.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:309: "number
of input channels and output channels cannot both be zero.");
On 2016/05/13 01:20:13, hoch wrote:
> input channels and output channels => input and output channels
> 
> Then we can contain this within 80 cols.

I chose this originally to kind of match the text in the other exceptions below.

Plus, we shouldn't shorten a user message just to make source code fit in 80
cols.  That's the wrong approach.

[Raymond Toy, 2016-05-13 18:41:47]

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:178: if
(context.isContextClosed()) {
On 2016/05/13 16:36:01, Raymond Toy wrote:
> On 2016/05/13 01:20:11, hoch wrote:
> > Perhaps we can make this to a method? This check-and-return pattern is
almost
> > everywhere.
> 
> I thought about that.  But we'd still have to do something like
> 
> if (context.throwIfContextClosed())
>   return nullptr;
> 
> Only one line shorter.

After some further discussions, we decided it would be best handled in the
AudioNode constructor, which we will implement in a followup CL.

[hongchan, 2016-05-13 21:35:48]

https://codereview.chromium.org/1952793002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:267: 
Let's add a comment here. In fact, I think we should remove const vars and make
them into a comment.

[Raymond Toy, 2016-05-16 23:11:53]

https://codereview.chromium.org/1952793002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:267: 
On 2016/05/13 21:35:48, hoch wrote:
> Let's add a comment here. In fact, I think we should remove const vars and
make
> them into a comment.

Done.

[Raymond Toy, 2016-05-17 21:31:44]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 21:32:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1952793002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1952793002/160001

[commit-bot: I haz the power, 2016-05-17 23:50:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-17 23:50:55]

Dry run: This issue passed the CQ dry run.

[Raymond Toy, 2016-05-20 21:09:32]

PTAL

[hongchan, 2016-05-20 21:28:32]

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:355: //
context keeps reference until we stop making javascript rendering callbacks
On 2016/05/13 01:20:13, hoch wrote:
> If we move this comment above |if (node)| Then we can remove curly braces.

Perhaps it needs one more look here?

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:359: return
node;
On 2016/05/13 01:20:13, hoch wrote:
> The same issue before - we should return node when it is valid. Otherwise we
> should return nullptr.

Is this addressed somewhere?

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:114:
ExceptionState&);
On 2016/05/13 01:20:13, hoch wrote:
> Let's make this consistent (by wrapping around 80 cols).

Can you take one more look on this?

https://codereview.chromium.org/1952793002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp:145:
// TODO(hongchan): Only stereo streams are supported right now. We should be
able to accept
I think we agreed to wrap the comment at 80 cols.

https://codereview.chromium.org/1952793002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp
(right):

https://codereview.chromium.org/1952793002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:144: }
Just curious: when m_destinationNode is not valid, what will happen here? I
don't think it is unlikely but we don't verify that anywhere.

[Raymond Toy, 2016-05-20 21:46:10]

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:355: //
context keeps reference until we stop making javascript rendering callbacks
On 2016/05/13 01:20:13, hoch wrote:
> If we move this comment above |if (node)| Then we can remove curly braces.

I like the braces; moving the comment out just so I can remove braces seems
unnecessary.

(FWIW, I hate the rule that braces are not needed.  It's a needless extra rule
and it's a great source of silly bugs when modifying code.)

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:359: return
node;
On 2016/05/20 21:28:32, hoch wrote:
> On 2016/05/13 01:20:13, hoch wrote:
> > The same issue before - we should return node when it is valid. Otherwise we
> > should return nullptr.
> 
> Is this addressed somewhere?

If line 352 fails to create a ScriptProcessor, node is null, and we'll return a
null here.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:114:
ExceptionState&);
On 2016/05/20 21:28:32, hoch wrote:
> On 2016/05/13 01:20:13, hoch wrote:
> > Let's make this consistent (by wrapping around 80 cols).
> 
> Can you take one more look on this?

What kind of consistency?  Just wrap line 108 to 80 cols (which isn't required)?
 Unwrap line 109-114 to 80 cols?

Make line 108 look like 109-114?  What about 107 and 106?

https://codereview.chromium.org/1952793002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp
(right):

https://codereview.chromium.org/1952793002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:144: }
On 2016/05/20 21:28:32, hoch wrote:
> Just curious: when m_destinationNode is not valid, what will happen here? I
> don't think it is unlikely but we don't verify that anywhere.

If m_destination doesn't exist, the AudioParams in the listener can't get the
sample rate that's needed for setting up the audio params in the listener.  I
think the context time (in the destination node) is also needed to note when the
listener has moved so the panner's can be updated.

[hongchan, 2016-05-20 22:57:49]

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:355: //
context keeps reference until we stop making javascript rendering callbacks
On 2016/05/20 21:46:09, Raymond Toy wrote:
> On 2016/05/13 01:20:13, hoch wrote:
> > If we move this comment above |if (node)| Then we can remove curly braces.
> 
> I like the braces; moving the comment out just so I can remove braces seems
> unnecessary.
> 
> (FWIW, I hate the rule that braces are not needed.  It's a needless extra rule
> and it's a great source of silly bugs when modifying code.)

It goes both ways. Without braces, we can easily tell this is a single action
based on the condition. Also the condition is the part of 'comment' here. Isn't
it?

Anyhow, I will leave this up to you.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:359: return
node;
On 2016/05/20 21:46:09, Raymond Toy wrote:
> On 2016/05/20 21:28:32, hoch wrote:
> > On 2016/05/13 01:20:13, hoch wrote:
> > > The same issue before - we should return node when it is valid. Otherwise
we
> > > should return nullptr.
> > 
> > Is this addressed somewhere?
> 
> If line 352 fails to create a ScriptProcessor, node is null, and we'll return
a
> null here.

But can we be explicit about that? We can easily return null in the if clause
above.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.h:114:
ExceptionState&);
On 2016/05/20 21:46:10, Raymond Toy wrote:
> On 2016/05/20 21:28:32, hoch wrote:
> > On 2016/05/13 01:20:13, hoch wrote:
> > > Let's make this consistent (by wrapping around 80 cols).
> > 
> > Can you take one more look on this?
> 
> What kind of consistency?  Just wrap line 108 to 80 cols (which isn't
required)?
>  Unwrap line 109-114 to 80 cols?
> 
> Make line 108 look like 109-114?  What about 107 and 106?

Which style do you want to have here? wrapped or unwrapped? It's up to you - I
just want them to be consistent.

https://codereview.chromium.org/1952793002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp
(right):

https://codereview.chromium.org/1952793002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:144: }
On 2016/05/20 21:46:10, Raymond Toy wrote:
> On 2016/05/20 21:28:32, hoch wrote:
> > Just curious: when m_destinationNode is not valid, what will happen here? I
> > don't think it is unlikely but we don't verify that anywhere.
> 
> If m_destination doesn't exist, the AudioParams in the listener can't get the
> sample rate that's needed for setting up the audio params in the listener.  I
> think the context time (in the destination node) is also needed to note when
the
> listener has moved so the panner's can be updated.

Okay, I like to throw more questions here but I don't think they are relevant to
this CL. Another time!

[Raymond Toy, 2016-05-20 23:12:01]

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp:130:
ASSERT(isMainThread());
On 2016/05/13 01:20:11, hoch wrote:
> DCHECK?

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp:138:
ASSERT(isMainThread());
On 2016/05/13 01:20:11, hoch wrote:
> Ditto.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ChannelSplitterNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelSplitterNode.cpp:85:
ASSERT(isMainThread());
On 2016/05/13 01:20:11, hoch wrote:
> Let's use DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelSplitterNode.cpp:93:
ASSERT(isMainThread());
On 2016/05/13 01:20:11, hoch wrote:
> Ditto.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:176:
ASSERT(isMainThread());
On 2016/05/13 01:20:11, hoch wrote:
> DCHECK?

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/DelayNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DelayNode.cpp:54:
ASSERT(isMainThread());
On 2016/05/13 01:20:11, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DelayNode.cpp:62:
ASSERT(isMainThread());
On 2016/05/13 01:20:11, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/DynamicsCompressorNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DynamicsCompressorNode.cpp:148:
ASSERT(isMainThread());
On 2016/05/13 01:20:12, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/GainNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/GainNode.cpp:126:
ASSERT(isMainThread());
On 2016/05/13 01:20:12, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/IIRFilterNode.cpp:37:
ASSERT(isMainThread());
On 2016/05/13 01:20:12, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaElementAudioSourceNode.cpp:214:
ASSERT(isMainThread());
On 2016/05/13 01:20:12, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioDestinationNode.cpp:127:
ASSERT(isMainThread());
On 2016/05/13 01:20:12, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp
(right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp:121:
ASSERT(isMainThread());
On 2016/05/13 01:20:12, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/MediaStreamAudioSourceNode.cpp:143:
// FIXME: Only stereo streams are supported right now. We should be able to
accept multi-channel streams.
On 2016/05/13 01:20:12, hoch wrote:
> Over 80 cols. Let's change it to 'TODO'. You can put my ID here.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/PannerNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/PannerNode.cpp:505:
ASSERT(isMainThread());
On 2016/05/13 01:20:12, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/PeriodicWave.cpp:59:
ASSERT(isMainThread());
On 2016/05/13 01:20:12, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:266:
ASSERT(isMainThread());
On 2016/05/13 01:20:13, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:276:
ASSERT(isMainThread());
On 2016/05/13 01:20:13, hoch wrote:
> Ditto.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:287:
ASSERT(isMainThread());
On 2016/05/13 01:20:13, hoch wrote:
> Ditto.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/StereoPannerNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/StereoPannerNode.cpp:146:
ASSERT(isMainThread());
On 2016/05/13 01:20:13, hoch wrote:
> DCHECK.

Done.

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/WaveShaperNode.cpp (right):

https://codereview.chromium.org/1952793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/WaveShaperNode.cpp:44:
ASSERT(isMainThread());
On 2016/05/13 01:20:13, hoch wrote:
> DCHECK.

Done.

[hongchan, 2016-05-24 20:44:06]

lgtm

[Raymond Toy, 2016-05-25 15:19:03]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2016-05-25 15:19:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1952793002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1952793002/200001

[commit-bot: I haz the power, 2016-05-25 16:52:03]

Description was changed from

==========
Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

The result should be that AbstractAudioContext::createFoo() does nothing
except call Foo::create() and returns the created node.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.
==========

to

==========
Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

The result should be that AbstractAudioContext::createFoo() does nothing
except call Foo::create() and returns the created node.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.
==========

[commit-bot: I haz the power, 2016-05-25 16:52:04]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-25 16:53:53]

Description was changed from

==========
Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

The result should be that AbstractAudioContext::createFoo() does nothing
except call Foo::create() and returns the created node.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.
==========

to

==========
Move the exception logic to the AudioNode creator

The logic for handling exceptions is mostly in
AbstractAudioContext::createFoo() method.  The right place for this is
in the actual method that creates the node, FooNode::create().  Move
the logic there.

The result should be that AbstractAudioContext::createFoo() does nothing
except call Foo::create() and returns the created node.

This will be important when AudioNode constructors are added.

BUG=608766
TESTS=none; internal change that should be transparent.

Committed: https://crrev.com/d3e0367d4b1f7637609f09f5fc3dbab97ae11b11
Cr-Commit-Position: refs/heads/master@{#395914}
==========

[commit-bot: I haz the power, 2016-05-25 16:53:54]

Patchset 11 (id:??) landed as
https://crrev.com/d3e0367d4b1f7637609f09f5fc3dbab97ae11b11
Cr-Commit-Position: refs/heads/master@{#395914}