WebAudio: Remove AudioNode::RefType.

WebAudio: Remove AudioNode::RefType.

The motivation of this CL is to reduce manual ref()/deref().

AudioNode has two types of ref()/deref(); RefTypeNormal and RefTypeConnection.
It made the code complex, and we couldn't use RefPtr<> for the connection type
references.

This CL removes AudioNode::RefType.
 - m_normalRefCount and m_connectionRefCount were updated independently. After
   this CL, connection type references updates both of them.
 - We use RefPtr<> for connection type references, and call
   AudioNode::makeConnection() and AudioNode::breakConnection() explicitly when
   connections are updated.

BUG=392788
TEST=none; This CL should not have behavior changes.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177910

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178026

[tkent, 2014-06-27 08:55:04]

Would you review this please?

My goal is to simplify lifetime management in modules/webaudio/.  This CL is the
first step.

[Raymond Toy, 2014-06-27 18:01:03]

On 2014/06/27 08:55:04, tkent wrote:
> Would you review this please?
> 
> My goal is to simplify lifetime management in modules/webaudio/.  This CL is
the
> first step.

That's really fantastic!

I have not yet reviewed the code, but I'm not sure you can update both
m_normalRefCount and m_connectionRefCount at the same time as your description
says. normalRefCount is basically tied to the corresponding JavaScript object.
connectionRefCount is intended to keep things alive even though the
corresponding JavaScript object has already been collected. This is needed for
nodes that have significant tail time such as convolution nodes that need to
keep playing many seconds after the JS object is gone.

Although I guess if they're the same, this just makes the JS object live longer
than before and won't go away until the underlying audio object goes away. I
don't know if that's acceptable or not.

[tkent, 2014-07-08 06:32:08]

On 2014/06/27 18:01:03, Raymond Toy wrote:
> Although I guess if they're the same, this just makes the JS object live
longer
> than before and won't go away until the underlying audio object goes away. I
> don't know if that's acceptable or not.

It won't happen.  This CL doesn't change lifetime of any objects.

[Raymond Toy, 2014-07-09 21:52:38]

On 2014/07/08 06:32:08, tkent wrote:
> On 2014/06/27 18:01:03, Raymond Toy wrote:
> > Although I guess if they're the same, this just makes the JS object live
> longer
> > than before and won't go away until the underlying audio object goes away. I
> > don't know if that's acceptable or not.
> 
> It won't happen.  This CL doesn't change lifetime of any objects.

Ah, because you moved the connectionRef stuff to the wasConnected method. This
makes sense now.

[Raymond Toy, 2014-07-09 22:10:34]

Looks good overall. Just some questions.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioBufferSourceNode.h (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioBufferSourceNode.h:135: // This RefPtr makes the
node graph. We need to call AudioNode::
What do you mean by "makes the node graph"?

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioBufferSourceNode.h:137: // before deref().
Does this comment about wasConnected() and willBeDisconnected() apply only to
m_pannerNode?

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:590: node->wasConnected();
Can we use a different name for wasConnected()? Every time I read that I keep
thinking it's asking if the node was connected, that that a node is being
connected.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:600: m_referencedNodes.remove(i);
Would it make sense to have remove(i) call willBeDisconnected instead of doing
it here?

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:481: fprintf(stderr, "%p: %d:
AudioNode::ref(%d) %d %d\n", this, nodeType(), refType, m_normalRefCount,
m_connectionRefCount);
Please fix this fprintf since refType is gone.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:530: // This function should be called
before deref().
Should or must be called before deref()?

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:544: fprintf(stderr, "%p: %d:
AudioNode::deref(%d) %d %d\n", this, nodeType(), refType, m_normalRefCount,
m_connectionRefCount);
Fix fprintf since refType is gone now.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.h:101: void wasConnected();
Add a comment on what wasConnected() does and when it should be called, like
what you have for willBeDisconnected().

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.h:104: // This function should be called
before releasing connection-type
"should" or "must" be called?

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioNodeOutput.h (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNodeOutput.h:143: HashSet<RefPtr<AudioNode> >
m_inputNodes;
What is this hash table for? Why is this needed now?

And m_inputNodes MUST be synchronized with m_inputs, right?

[tkent, 2014-07-10 08:07:42]

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioBufferSourceNode.h (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioBufferSourceNode.h:135: // This RefPtr makes the
node graph. We need to call AudioNode::
On 2014/07/09 22:10:33, Raymond Toy wrote:
> What do you mean by "makes the node graph"?

It means a connection type reference.  I'll improve the wording.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioBufferSourceNode.h:137: // before deref().
On 2014/07/09 22:10:33, Raymond Toy wrote:
> Does this comment about wasConnected() and willBeDisconnected() apply only to
> m_pannerNode?

Right.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:590: node->wasConnected();
On 2014/07/09 22:10:33, Raymond Toy wrote:
> Can we use a different name for wasConnected()? Every time I read that I keep
> thinking it's asking if the node was connected, that that a node is being
> connected.

How about didConnect?

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:600: m_referencedNodes.remove(i);
On 2014/07/09 22:10:33, Raymond Toy wrote:
> Would it make sense to have remove(i) call willBeDisconnected instead of doing
> it here?

We can do it by adding a class wrapping a Vector.  However, I feel it's
overkill.
I'd like to keep this simpler because I'll update the code again and again.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioNode.cpp (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:481: fprintf(stderr, "%p: %d:
AudioNode::ref(%d) %d %d\n", this, nodeType(), refType, m_normalRefCount,
m_connectionRefCount);
On 2014/07/09 22:10:34, Raymond Toy wrote:
> Please fix this fprintf since refType is gone.

will do

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:530: // This function should be called
before deref().
On 2014/07/09 22:10:33, Raymond Toy wrote:
> Should or must be called before deref()?

Must.  I'll remove this comment.  The comment in AudioNode.h is enough.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:544: fprintf(stderr, "%p: %d:
AudioNode::deref(%d) %d %d\n", this, nodeType(), refType, m_normalRefCount,
m_connectionRefCount);
On 2014/07/09 22:10:34, Raymond Toy wrote:
> Fix fprintf since refType is gone now.

will do.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioNode.h (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.h:101: void wasConnected();
On 2014/07/09 22:10:34, Raymond Toy wrote:
> Add a comment on what wasConnected() does and when it should be called, like
> what you have for willBeDisconnected().

Will do

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.h:104: // This function should be called
before releasing connection-type
On 2014/07/09 22:10:34, Raymond Toy wrote:
> "should" or "must" be called?

must.  Will update.

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioNodeOutput.h (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNodeOutput.h:143: HashSet<RefPtr<AudioNode> >
m_inputNodes;
On 2014/07/09 22:10:34, Raymond Toy wrote:
> What is this hash table for? Why is this needed now?
> 
> And m_inputNodes MUST be synchronized with m_inputs, right?

This holds connection references, and replaces manual ref()/deref().

Yes, MUST.

[tkent, 2014-07-10 09:22:49]

I uploaded new patch.  Please take a look at it.

[Raymond Toy, 2014-07-10 14:46:29]

lgtm, with a nit on the name didConnect.  Is makeConnection better?  If so, then
maybe willDisconnect should be breakConnection?

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:590: node->wasConnected();
On 2014/07/10 08:07:42, tkent wrote:
> On 2014/07/09 22:10:33, Raymond Toy wrote:
> > Can we use a different name for wasConnected()? Every time I read that I
keep
> > thinking it's asking if the node was connected, that that a node is being
> > connected.
> 
> How about didConnect?

I still read that as asking if the node did connect (to something).  How about
makeConnection?

https://codereview.chromium.org/349213007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:600: m_referencedNodes.remove(i);
On 2014/07/10 08:07:42, tkent wrote:
> On 2014/07/09 22:10:33, Raymond Toy wrote:
> > Would it make sense to have remove(i) call willBeDisconnected instead of
doing
> > it here?
> 
> We can do it by adding a class wrapping a Vector.  However, I feel it's
> overkill.
> I'd like to keep this simpler because I'll update the code again and again.

Oh, right.  Yeah, keep it simple as it is.

[tkent, 2014-07-11 00:37:22]

On 2014/07/10 14:46:29, Raymond Toy wrote:
> lgtm, with a nit on the name didConnect.  Is makeConnection better?  If so,
then
> maybe willDisconnect should be breakConnection?

ok, renamed so.

[tkent, 2014-07-11 00:37:26]

The CQ bit was checked by tkent@chromium.org

[tkent, 2014-07-11 00:37:38]

The CQ bit was unchecked by tkent@chromium.org

[tkent, 2014-07-11 00:38:23]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-07-11 00:38:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/349213007/60001

[commit-bot: I haz the power, 2014-07-11 01:38:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16348)

[commit-bot: I haz the power, 2014-07-11 02:46:58]

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

[commit-bot: I haz the power, 2014-07-11 02:46:59]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[tkent, 2014-07-11 02:59:37]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-07-11 02:59:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/349213007/60001

[tkent, 2014-07-11 07:07:30]

The CQ bit was unchecked by tkent@chromium.org

[tkent, 2014-07-11 07:21:00]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-07-11 07:22:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/349213007/60001

[commit-bot: I haz the power, 2014-07-11 07:40:09]

Change committed as 177910

[falken, 2014-07-11 08:52:56]

On 2014/07/11 07:40:09, I haz the power (commit-bot) wrote:
> Change committed as 177910

It looks like after this patch the following test started flakily failing
(timeout and crash):
webaudio/delaynode-max-nondefault-delay.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[tkent, 2014-07-11 08:56:11]

On 2014/07/11 08:52:56, falken wrote:
> On 2014/07/11 07:40:09, I haz the power (commit-bot) wrote:
> > Change committed as 177910
> 
> It looks like after this patch the following test started flakily failing
> (timeout and crash):
> webaudio/delaynode-max-nondefault-delay.html

Thank you.  I'll revert it.

[tkent, 2014-07-11 08:56:33]

A revert of this CL has been created in
https://codereview.chromium.org/385953003/ by tkent@chromium.org.

The reason for reverting is: Made webaudio/delaynode-max-nondefault-delay.html
falky.
.

[tkent, 2014-07-14 03:56:04]

https://codereview.chromium.org/349213007/diff/60001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioNodeOutput.h (right):

https://codereview.chromium.org/349213007/diff/60001/Source/modules/webaudio/...
Source/modules/webaudio/AudioNodeOutput.h:143: HashSet<RefPtr<AudioNode> >
m_inputNodes;
Using HashSet<> was wrong.  We can associate an AudioNodeOutput to multiple
AudioNodeInputs for single AudioNode.
I'm changing m_inputs type from HashSet<AudioNodeInput*> to
HashMap<AudioNodeInput*, RefPtr<AudioNode>>.

[tkent, 2014-07-14 03:56:07]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-07-14 03:56:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tkent@chromium.org/349213007/80001

[commit-bot: I haz the power, 2014-07-14 04:55:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-07-14 05:54:08]

Change committed as 178026