Implement StereoPannerNode for robust stereo panning

StereoPannerNode represents a processing node which positions an incoming audio stream in a stereo image using a low-cost equal-power panning algorithm.

The detailed specification of the node can be found here:
http://webaudio.github.io/web-audio-api/#the-stereopannernode-interface

See also:
https://github.com/WebAudio/web-audio-api/issues/372

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

[hongchan, 2014-11-11 18:49:43]

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

[Raymond Toy, 2014-11-11 19:41:47]

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
File LayoutTests/webaudio/stereopannernode-basic.html (right):

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-basic.html:20: }
I think the if statement is no longer needed.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-basic.html:34: 
The spec says channelCount cannot be increased above 2. You should have a test
for setting the channelCount to greater than 2. Spec also says channelCountMode
cannot be set to "max", so check for that as well.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
File LayoutTests/webaudio/stereopannernode-panning.html (right):

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:19: }
Don't think the if is needed anymore.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:35: impulseLength =
Math.round(timeStep * sampleRate);
This style is not generally allowed in blink or chromium code. But I don't know
what the actual style for JS code is.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:58: 
Describe what you're connecting together here and how you're going to test the
result.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:169: }
Chromium and blink style does not use braces for single-line cases.

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

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:370: {
Add ASSERT(isMainThread()), like for other creator methods.

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
File Source/modules/webaudio/StereoPannerNode.cpp (right):

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.cpp:50: m_pan =
AudioParam::create(context, 0.0);
No trailing ".0" on floating point constants unless required:
http://dev.chromium.org/blink/coding-style#TOC-Floating-point-literals

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.cpp:56: // can handles mono to stereo
and stereo to stereo conversion.
Typo: "handles" -> "handle"

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
File Source/modules/webaudio/StereoPannerNode.h (right):

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.h:61: AudioParam* pan() { return
m_pan.get(); }
Can this be const?

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
File Source/modules/webaudio/StereoPannerNode.idl (right):

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.idl:2: * Copyright (C) 2014, Google
Inc. All rights reserved.
Use the copyright style as mentioned here:
http://www.chromium.org/developers/coding-style, section File Headers

Do this for all of the new files that you've created.

https://codereview.chromium.org/691143007/diff/1/Source/platform/audio/Spatia...
File Source/platform/audio/Spatializer.h (right):

https://codereview.chromium.org/691143007/diff/1/Source/platform/audio/Spatia...
Source/platform/audio/Spatializer.h:45: PanningModelSurround = 2
Are PanningModelHRTF and PanningModelSurround supported yet? If not, I would
remove these.

[hongchan, 2014-11-12 00:06:54]

Thanks for the review. Please take a look at few followups.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
File LayoutTests/webaudio/stereopannernode-basic.html (right):

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-basic.html:20: }
On 2014/11/11 19:41:46, Raymond Toy wrote:
> I think the if statement is no longer needed.

Done.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-basic.html:34: 
On 2014/11/11 19:41:46, Raymond Toy wrote:
> The spec says channelCount cannot be increased above 2. You should have a test
> for setting the channelCount to greater than 2. Spec also says
channelCountMode
> cannot be set to "max", so check for that as well.

For this CL, I have decided to fix the channelCount value to 2, and
channelCountMode to 'clamped-max.' This needs to be revisited after the
discussion on up/down-mixing rules.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
File LayoutTests/webaudio/stereopannernode-panning.html (right):

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:19: }
On 2014/11/11 19:41:46, Raymond Toy wrote:
> Don't think the if is needed anymore.

Done.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:35: impulseLength =
Math.round(timeStep * sampleRate);
On 2014/11/11 19:41:46, Raymond Toy wrote:
> This style is not generally allowed in blink or chromium code. But I don't
know
> what the actual style for JS code is.

I looked it up Google's JS style guide, but it doesn't say anything about this
type of declaration. Although I believe the style is commonly found popular JS
libraries, I will just fix these and follow the convention from Chromium.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:58: 
On 2014/11/11 19:41:46, Raymond Toy wrote:
> Describe what you're connecting together here and how you're going to test the
> result.

I will describe what I am doing here. But I think that verifying the result
should be described below.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:169: }
On 2014/11/11 19:41:46, Raymond Toy wrote:
> Chromium and blink style does not use braces for single-line cases.

I just copied this part from the previous panner test file. Should I change
this?

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

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.cpp:370: {
On 2014/11/11 19:41:47, Raymond Toy wrote:
> Add ASSERT(isMainThread()), like for other creator methods.

Done.

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
File Source/modules/webaudio/StereoPannerNode.cpp (right):

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.cpp:50: m_pan =
AudioParam::create(context, 0.0);
On 2014/11/11 19:41:47, Raymond Toy wrote:
> No trailing ".0" on floating point constants unless required:
> http://dev.chromium.org/blink/coding-style#TOC-Floating-point-literals

Done.

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.cpp:56: // can handles mono to stereo
and stereo to stereo conversion.
On 2014/11/11 19:41:47, Raymond Toy wrote:
> Typo: "handles" -> "handle"

Done.

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.cpp:56: // can handles mono to stereo
and stereo to stereo conversion.
On 2014/11/11 19:41:47, Raymond Toy wrote:
> Typo: "handles" -> "handle"

Done.

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
File Source/modules/webaudio/StereoPannerNode.h (right):

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.h:61: AudioParam* pan() { return
m_pan.get(); }
On 2014/11/11 19:41:47, Raymond Toy wrote:
> Can this be const?

The structure of this node is based on GainNode and PannerNode. GainNode has the
same statement with a different AudioParam name - but it doesn't have const. Do
you think should I add it here?

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
File Source/modules/webaudio/StereoPannerNode.idl (right):

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.idl:2: * Copyright (C) 2014, Google
Inc. All rights reserved.
On 2014/11/11 19:41:47, Raymond Toy wrote:
> Use the copyright style as mentioned here:
> http://www.chromium.org/developers/coding-style, section File Headers
> 
> Do this for all of the new files that you've created.

Done for this file.

Should I do the same thing on the files that I didn't create? (i.e.
AudioContext)

https://codereview.chromium.org/691143007/diff/1/Source/platform/audio/Spatia...
File Source/platform/audio/Spatializer.h (right):

https://codereview.chromium.org/691143007/diff/1/Source/platform/audio/Spatia...
Source/platform/audio/Spatializer.h:45: PanningModelSurround = 2
On 2014/11/11 19:41:47, Raymond Toy wrote:
> Are PanningModelHRTF and PanningModelSurround supported yet? If not, I would
> remove these.

Done.

[Raymond Toy, 2014-11-12 00:16:45]

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
File LayoutTests/webaudio/stereopannernode-basic.html (right):

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-basic.html:20: }
On 2014/11/12 00:06:53, hoch wrote:
> On 2014/11/11 19:41:46, Raymond Toy wrote:
> > I think the if statement is no longer needed.
> 
> Done.

I think the style is to say "Done", after you upload the CL that has the change.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-basic.html:34: 
On 2014/11/12 00:06:53, hoch wrote:
> On 2014/11/11 19:41:46, Raymond Toy wrote:
> > The spec says channelCount cannot be increased above 2. You should have a
test
> > for setting the channelCount to greater than 2. Spec also says
> channelCountMode
> > cannot be set to "max", so check for that as well.
> 
> For this CL, I have decided to fix the channelCount value to 2, and
> channelCountMode to 'clamped-max.' This needs to be revisited after the
> discussion on up/down-mixing rules.

Will the spec say the channelCount is fixed to 2? I any case, I think you need
to test for setting the count > 2 and channelCountMode to "max", which is
definitely not allowed.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
File LayoutTests/webaudio/stereopannernode-panning.html (right):

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:35: impulseLength =
Math.round(timeStep * sampleRate);
On 2014/11/12 00:06:53, hoch wrote:
> On 2014/11/11 19:41:46, Raymond Toy wrote:
> > This style is not generally allowed in blink or chromium code. But I don't
> know
> > what the actual style for JS code is.
> 
> I looked it up Google's JS style guide, but it doesn't say anything about this
> type of declaration. Although I believe the style is commonly found popular JS
> libraries, I will just fix these and follow the convention from Chromium.

Up to you.  Chris always had me follow chromium (or blink?) style, but if the
Google JS style guide doesn't say, I think you're fine.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:58: 
On 2014/11/12 00:06:53, hoch wrote:
> On 2014/11/11 19:41:46, Raymond Toy wrote:
> > Describe what you're connecting together here and how you're going to test
the
> > result.
> 
> I will describe what I am doing here. But I think that verifying the result
> should be described below.

Acknowledged.

https://codereview.chromium.org/691143007/diff/1/LayoutTests/webaudio/stereop...
LayoutTests/webaudio/stereopannernode-panning.html:169: }
On 2014/11/12 00:06:53, hoch wrote:
> On 2014/11/11 19:41:46, Raymond Toy wrote:
> > Chromium and blink style does not use braces for single-line cases.
> 
> I just copied this part from the previous panner test file. Should I change
> this?

Actually, the blink style guide says this is ok in one of the examples.  So I
leave it up to you.

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
File Source/modules/webaudio/StereoPannerNode.h (right):

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.h:61: AudioParam* pan() { return
m_pan.get(); }
On 2014/11/12 00:06:54, hoch wrote:
> On 2014/11/11 19:41:47, Raymond Toy wrote:
> > Can this be const?
> 
> The structure of this node is based on GainNode and PannerNode. GainNode has
the
> same statement with a different AudioParam name - but it doesn't have const.
Do
> you think should I add it here?

I think the style is to be const when possible, so if you can, make this const,
even if GainNode and PannerNode are not.

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
File Source/modules/webaudio/StereoPannerNode.idl (right):

https://codereview.chromium.org/691143007/diff/1/Source/modules/webaudio/Ster...
Source/modules/webaudio/StereoPannerNode.idl:2: * Copyright (C) 2014, Google
Inc. All rights reserved.
On 2014/11/12 00:06:54, hoch wrote:
> On 2014/11/11 19:41:47, Raymond Toy wrote:
> > Use the copyright style as mentioned here:
> > http://www.chromium.org/developers/coding-style, section File Headers
> > 
> > Do this for all of the new files that you've created.
> 
> Done for this file.
> 
> Should I do the same thing on the files that I didn't create? (i.e.
> AudioContext)

No. Only use the new copyright template for new files. The old files should not
be changed.

[hongchan, 2014-11-12 00:43:02]

Patchset #2 (id:20001) has been deleted

[Raymond Toy, 2014-11-12 18:06:51]

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

[Raymond Toy, 2014-11-12 18:06:52]

lgtm with nits.

+haraken: Could you take a look at the oilpan stuff in StereoPannerNode.*? It
looks right to me.

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
File LayoutTests/webaudio/stereopannernode-basic.html (right):

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
LayoutTests/webaudio/stereopannernode-basic.html:34:
shouldThrow('panner.channelCountMode = "max"');
Is it possible to set the pan value outside the range [-1,1]? I assume it's
clipped to that range, so there should probably be a test here to show the
clipping.

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
File LayoutTests/webaudio/stereopannernode-panning.html (right):

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
LayoutTests/webaudio/stereopannernode-panning.html:40: var ctx = new
OfflineAudioContext(2, sampleRate * renderLength, sampleRate);
Probably better to use "context" instead of "ctx".

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
LayoutTests/webaudio/stereopannernode-panning.html:54: // Create multiple buffur
source nodes and panner nodes for test trials.
Typos: "buffur" -> buffer

Maybe instead of "for test trials", just say "testing". Currently reads a bit
strangely.

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
LayoutTests/webaudio/stereopannernode-panning.html:74: // To verify the result,
check if each source is an impulse turning at a
Typo? "turning" -> "starting"?

https://codereview.chromium.org/691143007/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/StereoPannerNode.cpp (right):

https://codereview.chromium.org/691143007/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/StereoPannerNode.cpp:55: // This node-specific default
mixing rules declare that StereoPannerNode
Typo: "This" -> "The"

https://codereview.chromium.org/691143007/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/StereoPannerNode.idl (right):

https://codereview.chromium.org/691143007/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/StereoPannerNode.idl:3: // Redistribution and use in
source and binary forms, with or without
The coding style indicates that the license header is just 3 lines long, and the
rest of this is not needed.

Be sure to do the same thing for all of the new files.

[hongchan, 2014-11-12 22:01:43]

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
File LayoutTests/webaudio/stereopannernode-basic.html (right):

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
LayoutTests/webaudio/stereopannernode-basic.html:34:
shouldThrow('panner.channelCountMode = "max"');
On 2014/11/12 18:06:52, Raymond Toy wrote:
> Is it possible to set the pan value outside the range [-1,1]? I assume it's
> clipped to that range, so there should probably be a test here to show the
> clipping.

For this CL, the clamping only happens internally. As we discussed, AudioParam
should handle the clamping - not the individual node. So the range check will be
done later with the improvement on AudioParam. (The internal clamping will be
removed along with it as well.)

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
File LayoutTests/webaudio/stereopannernode-panning.html (right):

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
LayoutTests/webaudio/stereopannernode-panning.html:40: var ctx = new
OfflineAudioContext(2, sampleRate * renderLength, sampleRate);
On 2014/11/12 18:06:52, Raymond Toy wrote:
> Probably better to use "context" instead of "ctx".

Done.

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
LayoutTests/webaudio/stereopannernode-panning.html:54: // Create multiple buffur
source nodes and panner nodes for test trials.
On 2014/11/12 18:06:52, Raymond Toy wrote:
> Typos: "buffur" -> buffer
> 
> Maybe instead of "for test trials", just say "testing". Currently reads a bit
> strangely.

Done.

https://codereview.chromium.org/691143007/diff/40001/LayoutTests/webaudio/ste...
LayoutTests/webaudio/stereopannernode-panning.html:74: // To verify the result,
check if each source is an impulse turning at a
On 2014/11/12 18:06:52, Raymond Toy wrote:
> Typo? "turning" -> "starting"?

Done.

https://codereview.chromium.org/691143007/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/StereoPannerNode.cpp (right):

https://codereview.chromium.org/691143007/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/StereoPannerNode.cpp:55: // This node-specific default
mixing rules declare that StereoPannerNode
On 2014/11/12 18:06:52, Raymond Toy wrote:
> Typo: "This" -> "The"

Done.

https://codereview.chromium.org/691143007/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/StereoPannerNode.idl (right):

https://codereview.chromium.org/691143007/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/StereoPannerNode.idl:3: // Redistribution and use in
source and binary forms, with or without
On 2014/11/12 18:06:52, Raymond Toy wrote:
> The coding style indicates that the license header is just 3 lines long, and
the
> rest of this is not needed.
> 
> Be sure to do the same thing for all of the new files.

Done.

[Raymond Toy, 2014-11-12 22:11:06]

lgtm.

I'd like haraken to take a quick look at the two files for oilpan issues before
you land this.

[hongchan, 2014-11-12 22:30:14]

On 2014/11/12 22:11:06, Raymond Toy wrote:
> lgtm.
> 
> I'd like haraken to take a quick look at the two files for oilpan issues
before
> you land this.

Thanks! Awaiting haraken's review.

[haraken, 2014-11-13 02:08:27]

LGTM

API owner reivew is needed for the IDL change.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
File Source/platform/audio/Spatializer.cpp (right):

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.cpp:24: // TODO: add the instantiation of
BinauralPanner here.

TODO => FIXME

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
File Source/platform/audio/Spatializer.h (right):

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.h:16: class PLATFORM_EXPORT Spatializer :
public GarbageCollectedFinalized<Spatializer> {

I guess "Finalized" is not needed.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.h:26: virtual ~Spatializer() { };

You can remove this destructor.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.h:41: Spatializer(PanningModel model) { }

Add explicit.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/St...
File Source/platform/audio/StereoPanner.h (right):

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/St...
Source/platform/audio/StereoPanner.h:15: StereoPanner(float sampleRate);

Add explicit.

[Raymond Toy, 2014-11-13 17:02:39]

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

[Raymond Toy, 2014-11-13 17:02:39]

tkent: Can you review the idl changes, as an API owner?

[hongchan, 2014-11-13 18:38:20]

Thanks. Awaiting IDL owner's review.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
File Source/platform/audio/Spatializer.cpp (right):

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.cpp:24: // TODO: add the instantiation of
BinauralPanner here.
On 2014/11/13 02:08:27, haraken wrote:
> 
> TODO => FIXME

Actually I removed ENUM items for additional panner nodes which are not
implemented yet. So I am going to remove this as well.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
File Source/platform/audio/Spatializer.h (right):

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.h:16: class PLATFORM_EXPORT Spatializer :
public GarbageCollectedFinalized<Spatializer> {
On 2014/11/13 02:08:27, haraken wrote:
> 
> I guess "Finalized" is not needed.

Done.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.h:26: virtual ~Spatializer() { };
On 2014/11/13 02:08:27, haraken wrote:
> 
> You can remove this destructor.

Done.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.h:41: Spatializer(PanningModel model) { }
On 2014/11/13 02:08:27, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/St...
File Source/platform/audio/StereoPanner.h (right):

https://codereview.chromium.org/691143007/diff/60001/Source/platform/audio/St...
Source/platform/audio/StereoPanner.h:15: StereoPanner(float sampleRate);
On 2014/11/13 02:08:27, haraken wrote:
> 
> Add explicit.

Done.

[tkent, 2014-11-14 01:17:43]

I think this feature is NOT trivial.  Please send "Intent to implement and ship"
to blink-dev.  No one will object it, but let's follow the process :)

[tkent, 2014-11-14 01:20:03]

https://codereview.chromium.org/691143007/diff/80001/Source/platform/audio/Sp...
File Source/platform/audio/Spatializer.h (right):

https://codereview.chromium.org/691143007/diff/80001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.h:19: PanningModelEqualPower = 0
enum with only one item looks non-sense.

If you have a plan to add more items.  Please add a comment.

[hongchan, 2014-11-14 22:46:47]

Patchset #5 (id:100001) has been deleted

[hongchan, 2014-11-14 22:49:00]

Patchset #5 (id:120001) has been deleted

[hongchan, 2014-11-14 22:51:15]

Intent to Implement and Ship was sent to blink-dev.

[hongchan, 2014-11-14 22:54:57]

Sorry for the spam. I marked 'done' on the comment from tkent.

https://codereview.chromium.org/691143007/diff/80001/Source/platform/audio/Sp...
File Source/platform/audio/Spatializer.h (right):

https://codereview.chromium.org/691143007/diff/80001/Source/platform/audio/Sp...
Source/platform/audio/Spatializer.h:19: PanningModelEqualPower = 0
On 2014/11/14 01:20:03, tkent wrote:
> enum with only one item looks non-sense.
> 
> If you have a plan to add more items.  Please add a comment.

Done.

[tkent, 2014-11-19 00:36:28]

lgtm

[tkent, 2014-11-19 00:36:40]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-11-19 00:37:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/691143007/140001

[commit-bot: I haz the power, 2014-11-19 00:46:49]

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

[commit-bot: I haz the power, 2014-11-19 00:46:51]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[hongchan, 2014-11-19 21:52:10]

The CQ bit was checked by hongchan@chromium.org

[commit-bot: I haz the power, 2014-11-19 21:53:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/691143007/160001

[commit-bot: I haz the power, 2014-11-19 23:38:57]

Committed patchset #6 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185632