Replace ASSERT with DCHECK in WebAudio

Replace ASSERT with DCHECK in WebAudio

BUG=637509
R=hongchan@chromium.org,rtoy@chromium.org

Committed: https://crrev.com/261ef60b39f7bf1f3232b4fa595e6c78ff57c048
Cr-Commit-Position: refs/heads/master@{#412483}

[HyungwookLee, 2016-07-20 03:38:07]

PTAL.

[Raymond Toy, 2016-07-20 15:00:06]

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

[Raymond Toy, 2016-07-20 15:00:06]

Please rebase to a more recent version.  AbstractAudioContext.* no longer
exists.

[hongchan, 2016-07-20 16:11:19]

Thanks for working on this!

I didn't go through everything yet, but there are few places where you need to
use DCHECK_EQ, DCHECK_NE and so forth.

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp
(right):

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:587:
DCHECK(m_contextState == Running);
Perhaps use DCHECK_EQ?

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:590:
DCHECK(m_contextState == Suspended);
ditto.

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:593:
DCHECK(m_contextState != Closed);
DCHECK_NE here?

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:598: //
ASSERTs above failed; just return.
Change the comment accordingly.

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/AudioBasicInspectorNode.cpp
(right):

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AudioBasicInspectorNode.cpp:76:
DCHECK(input == &this->input(0));
DCHECK_EQ?

[HyungwookLee, 2016-07-22 03:30:34]

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp
(right):

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:587:
DCHECK(m_contextState == Running);
On 2016/07/20 16:11:19, hoch wrote:
> Perhaps use DCHECK_EQ?

Done.

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:590:
DCHECK(m_contextState == Suspended);
On 2016/07/20 16:11:19, hoch wrote:
> ditto.

Done.

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:593:
DCHECK(m_contextState != Closed);
On 2016/07/20 16:11:19, hoch wrote:
> DCHECK_NE here?

Done.

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp:598: //
ASSERTs above failed; just return.
On 2016/07/20 16:11:19, hoch wrote:
> Change the comment accordingly.

Done.

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/AudioBasicInspectorNode.cpp
(right):

https://codereview.chromium.org/2159403002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/AudioBasicInspectorNode.cpp:76:
DCHECK(input == &this->input(0));
On 2016/07/20 16:11:19, hoch wrote:
> DCHECK_EQ?

Done.

[Raymond Toy, 2016-07-22 14:59:26]

https://codereview.chromium.org/2159403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioNodeInput.cpp (right):

https://codereview.chromium.org/2159403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioNodeInput.cpp:175:
DCHECK(numberOfRenderingConnections() > 1 ||
handler().internalChannelCountMode() != AudioHandler::Max);
Since we're rewriting these anyway, I think I'd prefer

DCHECK_GT(numberOfRenderingConnections(), 1);
DCHECK_NE(handler().internalChannelCountMode(), AudioHandler::Max);

This is way more informative since you can tell which of the conditions is
actually failing.

I think there's at least one more place that does ASSERT(<multiple conditions>).
I think they should be split out.

[HyungwookLee, 2016-07-25 00:24:44]

https://codereview.chromium.org/2159403002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioNodeInput.cpp (right):

https://codereview.chromium.org/2159403002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioNodeInput.cpp:175:
DCHECK(numberOfRenderingConnections() > 1 ||
handler().internalChannelCountMode() != AudioHandler::Max);
On 2016/07/22 14:59:26, Raymond Toy wrote:
> Since we're rewriting these anyway, I think I'd prefer
> 
> DCHECK_GT(numberOfRenderingConnections(), 1);
> DCHECK_NE(handler().internalChannelCountMode(), AudioHandler::Max);
> 
> This is way more informative since you can tell which of the conditions is
> actually failing.
> 
> I think there's at least one more place that does ASSERT(<multiple
conditions>).
> I think they should be split out.

Done.

[Raymond Toy, 2016-07-25 16:56:53]

Looks pretty good. A few more things, I think.

But I'd really like to get the isGraphOwner thing fixed if possible. That is the
whole point of the bug.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:81:
DCHECK_GE(static_cast<int>(numberOfInputs()), 1);
I think this is better written as

DCHECK_GE(numberOfInputs, 1u)

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:79:
DCHECK(isGood);
Not for this CL, but I think it would be good if we had separate DCHECKs for
each part of isGood.

WDYT?

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioNodeInput.cpp:175:
DCHECK_GT(static_cast<int>(numberOfRenderingConnections()), 1);
Maybe use
DCHECK_GT(numberOfRenderingConnections(), 1u)

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioNodeOutput.cpp:124:
DCHECK_GT(static_cast<int>(m_renderingParamFanOutCount), 0);
Use
DCHECK_GT(..., 0U)

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:401:
DCHECK_GE(static_cast<int>(numberOfValues), 1);
DCHECK_GE(..., 1U)

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BiquadFilterNode.cpp:152:
DCHECK(frequencyHz && magResponse && phaseResponse);
Perhaps split this into 3 DCHECK's for each part.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ChannelMergerNode.cpp:74:
DCHECK_EQ(static_cast<int>(input.numberOfChannels()), 1);
DCHECK_EQ(..., 1U)

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:71: #if
ENABLE(ASSERT)
The whole of the original bug was to figure out how to get rid of this
ENABLE(ASSERT).

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/IIRProcessor.cpp:19:
DCHECK_GT(static_cast<int>(feedforwardLength), 0);
DCHECK_GT(..., 0u)

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:426:
DCHECK_EQ(static_cast<int>(m_resumeResolvers.size()), 0);
DCHECK_EQ(..., 0u)

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:286:
DCHECK_GE(static_cast<int>(numberOfInputs()), 1);
DCHECK_GE(...,1u)

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:179:
DCHECK_LT(static_cast<int>(doubleBufferIndex), 2);
DCHECK_LT(...,2u)

[HyungwookLee, 2016-07-26 00:42:41]

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:81:
DCHECK_GE(static_cast<int>(numberOfInputs()), 1);
On 2016/07/25 16:56:52, Raymond Toy wrote:
> I think this is better written as
> 
> DCHECK_GE(numberOfInputs, 1u)

Done.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:79:
DCHECK(isGood);
On 2016/07/25 16:56:52, Raymond Toy wrote:
> Not for this CL, but I think it would be good if we had separate DCHECKs for
> each part of isGood.
> 
> WDYT?

That's good idea to make sure each part of isGood.
I will make new patch after landing this CL.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioNodeInput.cpp:175:
DCHECK_GT(static_cast<int>(numberOfRenderingConnections()), 1);
On 2016/07/25 16:56:52, Raymond Toy wrote:
> Maybe use
> DCHECK_GT(numberOfRenderingConnections(), 1u)

Done.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioNodeOutput.cpp:124:
DCHECK_GT(static_cast<int>(m_renderingParamFanOutCount), 0);
On 2016/07/25 16:56:52, Raymond Toy wrote:
> Use
> DCHECK_GT(..., 0U)

Done.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:401:
DCHECK_GE(static_cast<int>(numberOfValues), 1);
On 2016/07/25 16:56:52, Raymond Toy wrote:
> DCHECK_GE(..., 1U)

Done.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BiquadFilterNode.cpp:152:
DCHECK(frequencyHz && magResponse && phaseResponse);
On 2016/07/25 16:56:52, Raymond Toy wrote:
> Perhaps split this into 3 DCHECK's for each part.

Done.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:71: #if
ENABLE(ASSERT)
On 2016/07/25 16:56:52, Raymond Toy wrote:
> The whole of the original bug was to figure out how to get rid of this
> ENABLE(ASSERT).

Yes, I will make new CL for removing ENABLE(ASSERT) of
DeferredTaskHandler::isGraphOwner() after landing this CL.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/IIRProcessor.cpp:19:
DCHECK_GT(static_cast<int>(feedforwardLength), 0);
On 2016/07/25 16:56:52, Raymond Toy wrote:
> DCHECK_GT(..., 0u)

Done.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:426:
DCHECK_EQ(static_cast<int>(m_resumeResolvers.size()), 0);
On 2016/07/25 16:56:53, Raymond Toy wrote:
> DCHECK_EQ(..., 0u)

Done.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:286:
DCHECK_GE(static_cast<int>(numberOfInputs()), 1);
On 2016/07/25 16:56:53, Raymond Toy wrote:
> DCHECK_GE(...,1u)

Done.

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

https://codereview.chromium.org/2159403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp:179:
DCHECK_LT(static_cast<int>(doubleBufferIndex), 2);
On 2016/07/25 16:56:53, Raymond Toy wrote:
> DCHECK_LT(...,2u)

Done.

[Raymond Toy, 2016-07-26 16:17:47]

Thanks for your work on this. I'm fine with the changes, but I'd like Hongchan
to take a final look.

I'm ok with not fixing the isGraphOwner part in this CL.  You've explained in
the bug why we can't do this here. (See the bug.)  Although, it would be really
nice if this CL could include the necessary changes.

[hongchan, 2016-07-26 23:53:26]

I agree with rtoy@ in general. You might have to consult the proper owner to fix
the MutexBase class. Once you're decided on how to reconcile it, I'll put
l-g-t-m on this.

[HyungwookLee, 2016-07-27 01:08:49]

On 2016/07/26 23:53:26, hoch wrote:
> I agree with rtoy@ in general. You might have to consult the proper owner to
fix
> the MutexBase class. Once you're decided on how to reconcile it, I'll put
> l-g-t-m on this.

To replace ASSERT with DCHECK in everywhere, I think removing #if ENABLE(ASSERT)
in MutexBase is necessary for this.
I will have code review by MutexBase's owner via creating new CL that includes
removing #if ENABLE(ASSERT) in MutexBase.

[hongchan, 2016-07-27 16:28:41]

On 2016/07/27 01:08:49, HyungwookLee wrote:
> On 2016/07/26 23:53:26, hoch wrote:
> > I agree with rtoy@ in general. You might have to consult the proper owner to
> fix
> > the MutexBase class. Once you're decided on how to reconcile it, I'll put
> > l-g-t-m on this.
> 
> To replace ASSERT with DCHECK in everywhere, I think removing #if
ENABLE(ASSERT)
> in MutexBase is necessary for this.
> I will have code review by MutexBase's owner via creating new CL that includes
> removing #if ENABLE(ASSERT) in MutexBase.

Sounds good. LMK if you land that CL.

[HyungwookLee, 2016-08-09 03:53:03]

On 2016/07/27 16:28:41, hoch wrote:
> On 2016/07/27 01:08:49, HyungwookLee wrote:
> > On 2016/07/26 23:53:26, hoch wrote:
> > > I agree with rtoy@ in general. You might have to consult the proper owner
to
> > fix
> > > the MutexBase class. Once you're decided on how to reconcile it, I'll put
> > > l-g-t-m on this.
> > 
> > To replace ASSERT with DCHECK in everywhere, I think removing #if
> ENABLE(ASSERT)
> > in MutexBase is necessary for this.
> > I will have code review by MutexBase's owner via creating new CL that
includes
> > removing #if ENABLE(ASSERT) in MutexBase.
> 
> Sounds good. LMK if you land that CL.

According to esprehn, Removing #if ENABLE(ASSERT) in MutexBase is not simple
issue.
He suggests to implement release mode as following instead of Removing #if
ENABLE(ASSERT) in MutexBase.
See https://codereview.chromium.org/2191853002/

#if ENABLE(ASSERT)
  bool locked() { return ...; }
#else
  bool locked() { RELEASE_NOTREACHED(); return false; }
#endif

Actually, In our case we need to return true for release mode.
We'd better to implement release mode in WebAudio module as following instead of
modify it in MutexBase.

#if ENABLE(ASSERT)
bool DeferredTaskHandler::isGraphOwner()
{
    return m_contextGraphMutex.locked();
}
#else
bool DeferredTaskHandler::isGraphOwner()
{
    return true;
}
#endif

[Raymond Toy, 2016-08-09 17:00:13]

On 2016/08/09 at 03:53:03, hyungwook.lee wrote:
> On 2016/07/27 16:28:41, hoch wrote:
> > On 2016/07/27 01:08:49, HyungwookLee wrote:
> > > On 2016/07/26 23:53:26, hoch wrote:
> > > > I agree with rtoy@ in general. You might have to consult the proper
owner to
> > > fix
> > > > the MutexBase class. Once you're decided on how to reconcile it, I'll
put
> > > > l-g-t-m on this.
> > > 
> > > To replace ASSERT with DCHECK in everywhere, I think removing #if
> > ENABLE(ASSERT)
> > > in MutexBase is necessary for this.
> > > I will have code review by MutexBase's owner via creating new CL that
includes
> > > removing #if ENABLE(ASSERT) in MutexBase.
> > 
> > Sounds good. LMK if you land that CL.
> 
> According to esprehn, Removing #if ENABLE(ASSERT) in MutexBase is not simple
issue.
> He suggests to implement release mode as following instead of Removing #if
ENABLE(ASSERT) in MutexBase.
> See https://codereview.chromium.org/2191853002/
> 
> #if ENABLE(ASSERT)
>   bool locked() { return ...; }
> #else
>   bool locked() { RELEASE_NOTREACHED(); return false; }
> #endif
> 
> Actually, In our case we need to return true for release mode.
> We'd better to implement release mode in WebAudio module as following instead
of modify it in MutexBase.
> 
> #if ENABLE(ASSERT)
> bool DeferredTaskHandler::isGraphOwner()
> {
>     return m_contextGraphMutex.locked();
> }
> #else
> bool DeferredTaskHandler::isGraphOwner()
> {
>     return true;
> }
> #endif

Thanks for looking into this.  I'm fine with you just changing the WebAudio
implementation in this way.

[HyungwookLee, 2016-08-10 13:00:44]

On 2016/08/09 17:00:13, Raymond Toy wrote:
> On 2016/08/09 at 03:53:03, hyungwook.lee wrote:
> > On 2016/07/27 16:28:41, hoch wrote:
> > > On 2016/07/27 01:08:49, HyungwookLee wrote:
> > > > On 2016/07/26 23:53:26, hoch wrote:
> > > > > I agree with rtoy@ in general. You might have to consult the proper
> owner to
> > > > fix
> > > > > the MutexBase class. Once you're decided on how to reconcile it, I'll
> put
> > > > > l-g-t-m on this.
> > > > 
> > > > To replace ASSERT with DCHECK in everywhere, I think removing #if
> > > ENABLE(ASSERT)
> > > > in MutexBase is necessary for this.
> > > > I will have code review by MutexBase's owner via creating new CL that
> includes
> > > > removing #if ENABLE(ASSERT) in MutexBase.
> > > 
> > > Sounds good. LMK if you land that CL.
> > 
> > According to esprehn, Removing #if ENABLE(ASSERT) in MutexBase is not simple
> issue.
> > He suggests to implement release mode as following instead of Removing #if
> ENABLE(ASSERT) in MutexBase.
> > See https://codereview.chromium.org/2191853002/
> > 
> > #if ENABLE(ASSERT)
> >   bool locked() { return ...; }
> > #else
> >   bool locked() { RELEASE_NOTREACHED(); return false; }
> > #endif
> > 
> > Actually, In our case we need to return true for release mode.
> > We'd better to implement release mode in WebAudio module as following
instead
> of modify it in MutexBase.
> > 
> > #if ENABLE(ASSERT)
> > bool DeferredTaskHandler::isGraphOwner()
> > {
> >     return m_contextGraphMutex.locked();
> > }
> > #else
> > bool DeferredTaskHandler::isGraphOwner()
> > {
> >     return true;
> > }
> > #endif
> 
> Thanks for looking into this.  I'm fine with you just changing the WebAudio
> implementation in this way.

Done

[Raymond Toy, 2016-08-10 16:29:15]

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

https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioBufferSourceNode.cpp:270: // It
can happen that framesThisTime is 0. DCHECK that we will actually exit the loop
in
The original "Assert" was correct.

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

https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioNodeOutput.cpp:124:
DCHECK_GT(m_renderingParamFanOutCount, 0u);
This is wrong.  The original ASSERT was an OR, so only one condition needed to
be true.  Now both conditions need to be true with DCHECK.

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

https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:76: return
true;
Please add comment on why true is the correct result when ASSERT is not enabled.
 At first glance in isolation, this is definitely wrong because it's not always
the case that we're the graph owner.

I wonder if the API would be better if we replaced all DCHECK(isGraphOwner) with
assertGraphOwnership() (or other name) and have assertGraphOwnership() do the
DCHECK there.

This has a penalty that we get an extra function call (maybe make it inline?),
but does make it very clear what should happen if ASSERT is disabled.

[HyungwookLee, 2016-08-11 08:35:15]

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

https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioNodeOutput.cpp:124:
DCHECK_GT(m_renderingParamFanOutCount, 0u);
On 2016/08/10 16:29:15, Raymond Toy wrote:
> This is wrong.  The original ASSERT was an OR, so only one condition needed to
> be true.  Now both conditions need to be true with DCHECK.

Done.

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

https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:76: return
true;
On 2016/08/10 16:29:15, Raymond Toy wrote:
> Please add comment on why true is the correct result when ASSERT is not
enabled.
>  At first glance in isolation, this is definitely wrong because it's not
always
> the case that we're the graph owner.
> 
> I wonder if the API would be better if we replaced all DCHECK(isGraphOwner)
with
> assertGraphOwnership() (or other name) and have assertGraphOwnership() do the
> DCHECK there.
> 
> This has a penalty that we get an extra function call (maybe make it inline?),
> but does make it very clear what should happen if ASSERT is disabled.

I think the best solution is removing #if ENABLE(ASSERT) in MutexBase.
I will try to do this but it looks takes some time.

How about divide replacing ASSERT(isGraphOwner()) with DCHECK into another cl?

[Raymond Toy, 2016-08-11 15:52:59]

On 2016/08/11 at 08:35:15, hyungwook.lee wrote:
>
https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webaudio/AudioNodeOutput.cpp (right):
> 
>
https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webaudio/AudioNodeOutput.cpp:124:
DCHECK_GT(m_renderingParamFanOutCount, 0u);
> On 2016/08/10 16:29:15, Raymond Toy wrote:
> > This is wrong.  The original ASSERT was an OR, so only one condition needed
to
> > be true.  Now both conditions need to be true with DCHECK.
> 
> Done.
> 
>
https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp
(right):
> 
>
https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:76: return
true;
> On 2016/08/10 16:29:15, Raymond Toy wrote:
> > Please add comment on why true is the correct result when ASSERT is not
enabled.
> >  At first glance in isolation, this is definitely wrong because it's not
always
> > the case that we're the graph owner.
> > 
> > I wonder if the API would be better if we replaced all DCHECK(isGraphOwner)
with
> > assertGraphOwnership() (or other name) and have assertGraphOwnership() do
the
> > DCHECK there.
> > 
> > This has a penalty that we get an extra function call (maybe make it
inline?),
> > but does make it very clear what should happen if ASSERT is disabled.
> 
> I think the best solution is removing #if ENABLE(ASSERT) in MutexBase.
> I will try to do this but it looks takes some time.
> 
> How about divide replacing ASSERT(isGraphOwner()) with DCHECK into another cl?

I'm ok if you want to do this, but you need to file a different bug and update
bug number in this description to point to the new bug.  The current linked bug 
is about fixing isGraphOwner.

[HyungwookLee, 2016-08-13 03:17:06]

Description was changed from

==========
Replace ASSERT with DCHECK in WebAudio

BUG=626446
R=hongchan@chromium.org
==========

to

==========
Replace ASSERT with DCHECK in WebAudio

BUG=637509
R=hongchan@chromium.org,rtoy@chromium.org
==========

[HyungwookLee, 2016-08-13 03:18:16]

On 2016/08/11 15:52:59, Raymond Toy wrote:
> On 2016/08/11 at 08:35:15, hyungwook.lee wrote:
> >
>
https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/webaudio/AudioNodeOutput.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/webaudio/AudioNodeOutput.cpp:124:
> DCHECK_GT(m_renderingParamFanOutCount, 0u);
> > On 2016/08/10 16:29:15, Raymond Toy wrote:
> > > This is wrong.  The original ASSERT was an OR, so only one condition
needed
> to
> > > be true.  Now both conditions need to be true with DCHECK.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2159403002/diff/120001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:76:
return
> true;
> > On 2016/08/10 16:29:15, Raymond Toy wrote:
> > > Please add comment on why true is the correct result when ASSERT is not
> enabled.
> > >  At first glance in isolation, this is definitely wrong because it's not
> always
> > > the case that we're the graph owner.
> > > 
> > > I wonder if the API would be better if we replaced all
DCHECK(isGraphOwner)
> with
> > > assertGraphOwnership() (or other name) and have assertGraphOwnership() do
> the
> > > DCHECK there.
> > > 
> > > This has a penalty that we get an extra function call (maybe make it
> inline?),
> > > but does make it very clear what should happen if ASSERT is disabled.
> > 
> > I think the best solution is removing #if ENABLE(ASSERT) in MutexBase.
> > I will try to do this but it looks takes some time.
> > 
> > How about divide replacing ASSERT(isGraphOwner()) with DCHECK into another
cl?
> 
> I'm ok if you want to do this, but you need to file a different bug and update
> bug number in this description to point to the new bug.  The current linked
bug 
> is about fixing isGraphOwner.

I've made new bugs in
https://bugs.chromium.org/p/chromium/issues/detail?id=637509 and update it in
description.

[Raymond Toy, 2016-08-15 15:08:16]

lgtm from me.  I'd lik hongchan@ to take a quick look too.

[hongchan, 2016-08-15 15:43:22]

lgtm with nits.

Please take a look if we can substitute ASSERT_UNUSED with DCHECK. If not
possible, go ahead and try to land the CL.

Thanks for your contribution!

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

https://codereview.chromium.org/2159403002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioNode.cpp:947:
ASSERT_UNUSED(numberOfOutputs, numberOfOutputs == m_connectedNodes.size());
Can we use DCHECK for this? I did not do the research yet so not sure if there
is a DCHECK equivalent for this assertion.

https://codereview.chromium.org/2159403002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioNode.cpp:949:
ASSERT_UNUSED(numberOfOutputs, numberOfOutputs == m_connectedParams.size());
Ditto.

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

https://codereview.chromium.org/2159403002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ChannelSplitterNode.cpp:56:
ASSERT_UNUSED(framesToProcess, framesToProcess == source->length());
Same here.

[HyungwookLee, 2016-08-16 03:16:04]

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

https://codereview.chromium.org/2159403002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioNode.cpp:947:
ASSERT_UNUSED(numberOfOutputs, numberOfOutputs == m_connectedNodes.size());
On 2016/08/15 15:43:21, hoch wrote:
> Can we use DCHECK for this? I did not do the research yet so not sure if there
> is a DCHECK equivalent for this assertion.

In my investigation, ASSERT_UNUSED() is considered ASSERT is not defined
condition as following.

#if ENABLE(ASSERT)
ASSERT_UNUSED(variable, assertion) ASSERT(assertion)
#else
#define ASSERT_UNUSED(variable, assertion) ((void)variable)
#endif

I think we can replace ASSERT_UNUSED with DCHECK also.

Done.

https://codereview.chromium.org/2159403002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioNode.cpp:949:
ASSERT_UNUSED(numberOfOutputs, numberOfOutputs == m_connectedParams.size());
On 2016/08/15 15:43:21, hoch wrote:
> Ditto.

Done.

[hongchan, 2016-08-16 05:32:35]

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

[commit-bot: I haz the power, 2016-08-16 05:32:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 06:32:55]

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

[commit-bot: I haz the power, 2016-08-16 06:32:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hongchan, 2016-08-16 15:33:28]

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

[commit-bot: I haz the power, 2016-08-16 15:33:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 19:02:16]

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

[commit-bot: I haz the power, 2016-08-16 19:02:17]

Dry run: This issue passed the CQ dry run.

[hongchan, 2016-08-16 22:01:49]

The CQ bit was checked by hongchan@chromium.org

[hongchan, 2016-08-16 22:01:49]

The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org,
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2159403002/#ps180001
(title: "")

[commit-bot: I haz the power, 2016-08-16 22:02:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 22:09:41]

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

[commit-bot: I haz the power, 2016-08-16 22:09:42]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hongchan, 2016-08-16 22:24:15]

On 2016/08/16 22:09:42, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

hyungwook.lee@

Please rebase this patch so I can commit it for you.

[HyungwookLee, 2016-08-17 01:56:46]

The CQ bit was checked by hyungwook.lee@navercorp.com

[HyungwookLee, 2016-08-17 01:56:46]

The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org,
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2159403002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-17 01:57:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 01:59:42]

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

[commit-bot: I haz the power, 2016-08-17 01:59:43]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[HyungwookLee, 2016-08-17 02:04:00]

The CQ bit was checked by hyungwook.lee@navercorp.com

[commit-bot: I haz the power, 2016-08-17 02:04:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 02:07:26]

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

[commit-bot: I haz the power, 2016-08-17 02:07:27]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[HyungwookLee, 2016-08-17 06:21:18]

The CQ bit was checked by hyungwook.lee@navercorp.com

[HyungwookLee, 2016-08-17 06:21:19]

The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org,
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2159403002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-17 06:21:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 08:34:38]

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

[commit-bot: I haz the power, 2016-08-17 08:34:41]

Exceeded global retry quota

[HyungwookLee, 2016-08-17 09:10:06]

The CQ bit was checked by hyungwook.lee@navercorp.com

[commit-bot: I haz the power, 2016-08-17 09:10:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 09:29:37]

Description was changed from

==========
Replace ASSERT with DCHECK in WebAudio

BUG=637509
R=hongchan@chromium.org,rtoy@chromium.org
==========

to

==========
Replace ASSERT with DCHECK in WebAudio

BUG=637509
R=hongchan@chromium.org,rtoy@chromium.org
==========

[commit-bot: I haz the power, 2016-08-17 09:29:39]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-17 09:31:10]

Description was changed from

==========
Replace ASSERT with DCHECK in WebAudio

BUG=637509
R=hongchan@chromium.org,rtoy@chromium.org
==========

to

==========
Replace ASSERT with DCHECK in WebAudio

BUG=637509
R=hongchan@chromium.org,rtoy@chromium.org

Committed: https://crrev.com/261ef60b39f7bf1f3232b4fa595e6c78ff57c048
Cr-Commit-Position: refs/heads/master@{#412483}
==========

[commit-bot: I haz the power, 2016-08-17 09:31:11]

Patchset 12 (id:??) landed as
https://crrev.com/261ef60b39f7bf1f3232b4fa595e6c78ff57c048
Cr-Commit-Position: refs/heads/master@{#412483}