Merge m_didCallDispose and m_isMarkedForDeletion flags into one flag

Merge m_didCallDispose and m_isMarkedForDeletion flags into one flag

Currently we have the following two flags:

- m_didCallDispose is used to know whether AudioContext::dispose() is called or not.
- m_isMarkedForDeletion is used to guarantee the following facts:

--- AudioNode should not be re-registered to AudioNode::m_outputs after AudioNode::dispose is called.
--- AudioNode should not get marked as dirty after AudoNode::dispose is called.

We don't need the two flags; just one flag that is set in AudioContext::dispose() is enough.

This CL removes m_didCallDispose and m_isMarkedForDeletion and introduces m_isDisposeCalled. This CL doesn't change any behavior.

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

[haraken, 2014-08-12 14:29:52]

PTAL

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

https://codereview.chromium.org/460303003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:112: #if ENABLE(OILPAN)

Note: This CL depends on https://codereview.chromium.org/467683002/. After the
CL, disconnectAll() is called in both oilpan builds and non-oilpan builds here
(after m_isDisposeCalled is called).

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

https://codereview.chromium.org/460303003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.h:161: bool isDisposeCalled() const { return
m_isDisposeCalled; }

Nit: I renamed m_didCallDispose to m_isDisposeCalled, because "didCallDispose"
implies that it's a method called after dispose() is called.

[haraken, 2014-08-14 16:09:28]

tkent-san: The dependency CL was landed. Would you take a look at this?

[tkent, 2014-08-14 23:24:19]

> - AudioNode should not be re-registered to AudioNode::m_outputs after
AudioNode::dispose is called.
> - AudioNode should not get marked as dirty after AudoNode::dispose is called.

Please add C++ unit tests for them.

[tkent, 2014-08-14 23:25:01]

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

https://codereview.chromium.org/460303003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:108: // - the following disconnectAll()
from re-registering this AudioNode into the m_outputs.
I recommend to wrap code comments in 80 columns.

[haraken, 2014-08-15 01:27:18]

> Please add C++ unit tests for them.

This CL fixes leaks in webaudio/panner-loop.html in oilpan builds. (The leak
detector can detect the AudioNode leak, but AudioNode leaks are not treated as a
tree closer.)

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

https://codereview.chromium.org/460303003/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioNode.cpp:108: // - the following disconnectAll()
from re-registering this AudioNode into the m_outputs.
On 2014/08/14 23:25:01, tkent wrote:
> I recommend to wrap code comments in 80 columns.

Done.

[tkent, 2014-08-15 02:25:34]

On 2014/08/15 01:27:18, haraken wrote:
> This CL fixes leaks in webaudio/panner-loop.html in oilpan builds. (The leak
> detector can detect the AudioNode leak, but AudioNode leaks are not treated as
a
> tree closer.)

We don't leak AudioNode instances in Oilpan build with panner-loop.html.  We
leak in non-Oilpan build.
Is this change necessary only for non-Oilpan build?

Please update LayoutTests/LeakExpectations, and add 402742 to BUG= line.

[haraken, 2014-08-15 05:11:05]

On 2014/08/15 02:25:34, tkent wrote:
> On 2014/08/15 01:27:18, haraken wrote:
> > This CL fixes leaks in webaudio/panner-loop.html in oilpan builds. (The leak
> > detector can detect the AudioNode leak, but AudioNode leaks are not treated
as
> a
> > tree closer.)
> 
> We don't leak AudioNode instances in Oilpan build with panner-loop.html.  We
> leak in non-Oilpan build.
> Is this change necessary only for non-Oilpan build?

In short, I got confused.

- panner-loop.html is leaking in non-oilpan builds. This CL doesn't fix the
issue.
- panner-loop.html is not leaking in oilpan builds.
- When I rewrite a reference counting system of webaudio/
(https://codereview.chromium.org/438293003/), this CL is necessary to get rid of
memory leaks in non-oilpan builds.
- However, I've not yet succeeded in creating a test case that fails without
this CL and passes with this CL.

Still investigating.

[haraken, 2014-08-15 05:33:56]

I realized that this CL just merges two flags (m_didCallDispose and
m_isMarkedForDeletion) into one flag (m_isDisposeCalled). This refactoring
itself doesn't change any behavior but is necessary to guarantee that follow-up
changes don't cause leaks.

Updated the CL description. PTAL.

[tkent, 2014-08-15 05:50:55]

On 2014/08/15 05:33:56, haraken wrote:
> I realized that this CL just merges two flags (m_didCallDispose and
> m_isMarkedForDeletion) into one flag (m_isDisposeCalled). This refactoring
> itself doesn't change any behavior but is necessary to guarantee that
follow-up
> changes don't cause leaks.

It sounds reasonable.  lgtm

[haraken, 2014-08-15 06:00:53]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-08-15 06:01:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/460303003/20001

[commit-bot: I haz the power, 2014-08-15 06:26:03]

Committed patchset #2 (20001) as 180330