Add a name attribute for each AudioParam

Add a name attribute for each AudioParam

AudioParam's are fairly opaque but it's useful to know that an AudioParam belongs to a certain node and the name of the AudioParam itself.  This is especially useful for printing messages related to the AudioParam.

BUG=607699
TEST=none

Committed: https://crrev.com/0f7829c7596aa617ce042877b27e373991e4e682
Cr-Commit-Position: refs/heads/master@{#391102}

[Raymond Toy, 2016-04-29 16:45:28]

Description was changed from

==========
Add a name attribute for each AudioParam

BUG=607699
TEST=none
==========

to

==========
Add a name attribute for each AudioParam

AudioParam's are fairly opaque but it's useful to know that an AudioParam
belongs to a certain node and the name of the AudioParam itself.  This is
especially useful for printing messages related to the AudioParam.

BUG=607699
TEST=none
==========

[Raymond Toy, 2016-04-29 16:47:13]

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

[Raymond Toy, 2016-04-29 16:47:14]

PTAL.

[hongchan, 2016-04-29 22:22:27]

It looks okay, but I might have a little (or huge) suggestion.

Can we put this param type under the blink namespace?

namespace blink {

enum AudioParamType {
  ParamTypeUnknown, // 0
  ParamTypeAudioBufferSourcePlaybackRate,
  ....
  ParamTypeEnd // length
};

}

Some of items will be a bit verbose, but I think it doesn't look confusion at
least.

Some instantiation process looks actually vague. For example,

> m_delayTime(AudioParam::create(context, AudioParamHandler::DelayDelayTime,
0.0)

I will probably forget what this means and will have to look it up this is a
type.

WDYT?

https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.h (right):

https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.h:55: // Each AudioParam
gets an identifier here.  This is mostly for instrospection if warnings or
I believe you're following the conversion of AudioHandler::NodeType, but I think
this enum should be a part of AudioParam, not handler. I believe any user-facing
feature conceptually belongs to AudioParam.

Or we can put this enum under the blink namespace. (i.e. blink::AudioParamType)

https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.h:74: StereoPannerPan
Perhaps we need:
PannerPosition{X, Y, Z}
PannerOrientation{X, Y, Z}

[Raymond Toy, 2016-04-29 22:39:55]

https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.h (right):

https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.h:55: // Each AudioParam
gets an identifier here.  This is mostly for instrospection if warnings or
On 2016/04/29 22:22:26, hoch wrote:
> I believe you're following the conversion of AudioHandler::NodeType, but I
think
> this enum should be a part of AudioParam, not handler. I believe any
user-facing
> feature conceptually belongs to AudioParam.
> 
> Or we can put this enum under the blink namespace. (i.e.
blink::AudioParamType)

That's fine with me.

https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.h:74: StereoPannerPan
On 2016/04/29 22:22:27, hoch wrote:
> Perhaps we need:
> PannerPosition{X, Y, Z}
> PannerOrientation{X, Y, Z}
> 

Except we don't have those yet. :-)

[Raymond Toy, 2016-04-29 23:00:11]

https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParam.h (right):

https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParam.h:55: // Each AudioParam
gets an identifier here.  This is mostly for instrospection if warnings or
On 2016/04/29 22:22:26, hoch wrote:
> I believe you're following the conversion of AudioHandler::NodeType, but I
think
> this enum should be a part of AudioParam, not handler. I believe any
user-facing
> feature conceptually belongs to AudioParam.

Oh, this isn't user-facing.  Or at least as much user-facing as
AudioHandler::NodeType.

[hongchan, 2016-05-02 16:55:44]

On 2016/04/29 23:00:11, Raymond Toy wrote:
>
https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/AudioParam.h (right):
> 
>
https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AudioParam.h:55: // Each AudioParam
> gets an identifier here.  This is mostly for instrospection if warnings or
> On 2016/04/29 22:22:26, hoch wrote:
> > I believe you're following the conversion of AudioHandler::NodeType, but I
> think
> > this enum should be a part of AudioParam, not handler. I believe any
> user-facing
> > feature conceptually belongs to AudioParam.
> 
> Oh, this isn't user-facing.  Or at least as much user-facing as
> AudioHandler::NodeType.

Yeah, that was a unfortunate change that happened when we decided to separate
everything into Node-Handler pairs. This should've been a part of Node, not the
handler in my opinion. I'll just leave this up to you and have an adequate
comment on that. (so we can come back later when we do the refactoring in the
distant future.)

[Raymond Toy, 2016-05-02 16:58:35]

On 2016/05/02 16:55:44, hoch wrote:
> On 2016/04/29 23:00:11, Raymond Toy wrote:
> >
>
https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/webaudio/AudioParam.h (right):
> > 
> >
>
https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/webaudio/AudioParam.h:55: // Each
AudioParam
> > gets an identifier here.  This is mostly for instrospection if warnings or
> > On 2016/04/29 22:22:26, hoch wrote:
> > > I believe you're following the conversion of AudioHandler::NodeType, but I
> > think
> > > this enum should be a part of AudioParam, not handler. I believe any
> > user-facing
> > > feature conceptually belongs to AudioParam.
> > 
> > Oh, this isn't user-facing.  Or at least as much user-facing as
> > AudioHandler::NodeType.
> 
> Yeah, that was a unfortunate change that happened when we decided to separate
> everything into Node-Handler pairs. This should've been a part of Node, not
the
> handler in my opinion. I'll just leave this up to you and have an adequate
> comment on that. (so we can come back later when we do the refactoring in the
> distant future.)

File a bug if you think it should be moved.  I don't think this CL should move
or even comment on AudioNode.

[blink-reviews, 2016-05-02 16:59:25]

Okay. Fair enough. lgtm.

On Mon, May 2, 2016 at 9:58 AM rtoy@chromium.org via codereview.chromium.org
<reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/05/02 16:55:44, hoch wrote:
> > On 2016/04/29 23:00:11, Raymond Toy wrote:
> > >
> >
>
>
https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/webaudio/AudioParam.h (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/webaudio/AudioParam.h:55: // Each
> AudioParam
> > > gets an identifier here. This is mostly for instrospection if warnings
> or
> > > On 2016/04/29 22:22:26, hoch wrote:
> > > > I believe you're following the conversion of AudioHandler::NodeType,
> but I
> > > think
> > > > this enum should be a part of AudioParam, not handler. I believe any
> > > user-facing
> > > > feature conceptually belongs to AudioParam.
> > >
> > > Oh, this isn't user-facing. Or at least as much user-facing as
> > > AudioHandler::NodeType.
> >
> > Yeah, that was a unfortunate change that happened when we decided to
> separate
> > everything into Node-Handler pairs. This should've been a part of Node,
> not
> the
> > handler in my opinion. I'll just leave this up to you and have an
> adequate
> > comment on that. (so we can come back later when we do the refactoring
> in the
> > distant future.)
>
> File a bug if you think it should be moved. I don't think this CL should
> move
> or even comment on AudioNode.
>
> https://codereview.chromium.org/1928163002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-05-02 16:59:25]

Okay. Fair enough. lgtm.

On Mon, May 2, 2016 at 9:58 AM rtoy@chromium.org via codereview.chromium.org
<reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/05/02 16:55:44, hoch wrote:
> > On 2016/04/29 23:00:11, Raymond Toy wrote:
> > >
> >
>
>
https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/modules/webaudio/AudioParam.h (right):
> > >
> > >
> >
>
>
https://codereview.chromium.org/1928163002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/modules/webaudio/AudioParam.h:55: // Each
> AudioParam
> > > gets an identifier here. This is mostly for instrospection if warnings
> or
> > > On 2016/04/29 22:22:26, hoch wrote:
> > > > I believe you're following the conversion of AudioHandler::NodeType,
> but I
> > > think
> > > > this enum should be a part of AudioParam, not handler. I believe any
> > > user-facing
> > > > feature conceptually belongs to AudioParam.
> > >
> > > Oh, this isn't user-facing. Or at least as much user-facing as
> > > AudioHandler::NodeType.
> >
> > Yeah, that was a unfortunate change that happened when we decided to
> separate
> > everything into Node-Handler pairs. This should've been a part of Node,
> not
> the
> > handler in my opinion. I'll just leave this up to you and have an
> adequate
> > comment on that. (so we can come back later when we do the refactoring
> in the
> > distant future.)
>
> File a bug if you think it should be moved. I don't think this CL should
> move
> or even comment on AudioNode.
>
> https://codereview.chromium.org/1928163002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Raymond Toy, 2016-05-02 17:02:23]

The CQ bit was checked by rtoy@chromium.org

[hongchan, 2016-05-02 17:02:49]

lgtm

[commit-bot: I haz the power, 2016-05-02 17:02:49]

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

[commit-bot: I haz the power, 2016-05-02 17:14:32]

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

[commit-bot: I haz the power, 2016-05-02 17:14:33]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Raymond Toy, 2016-05-02 18:31:28]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2016-05-02 18:31:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/1928163002/#ps60001
(title: "Add default case, not reached")

[commit-bot: I haz the power, 2016-05-02 18:32:21]

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

[commit-bot: I haz the power, 2016-05-02 18:47:14]

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

[commit-bot: I haz the power, 2016-05-02 18:47:15]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Raymond Toy, 2016-05-02 19:52:29]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2016-05-02 19:52:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/1928163002/#ps80001
(title: "Fix compilation error")

[commit-bot: I haz the power, 2016-05-02 19:52:54]

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

[commit-bot: I haz the power, 2016-05-02 23:44:23]

Description was changed from

==========
Add a name attribute for each AudioParam

AudioParam's are fairly opaque but it's useful to know that an AudioParam
belongs to a certain node and the name of the AudioParam itself.  This is
especially useful for printing messages related to the AudioParam.

BUG=607699
TEST=none
==========

to

==========
Add a name attribute for each AudioParam

AudioParam's are fairly opaque but it's useful to know that an AudioParam
belongs to a certain node and the name of the AudioParam itself.  This is
especially useful for printing messages related to the AudioParam.

BUG=607699
TEST=none
==========

[commit-bot: I haz the power, 2016-05-02 23:44:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-02 23:46:36]

Description was changed from

==========
Add a name attribute for each AudioParam

AudioParam's are fairly opaque but it's useful to know that an AudioParam
belongs to a certain node and the name of the AudioParam itself.  This is
especially useful for printing messages related to the AudioParam.

BUG=607699
TEST=none
==========

to

==========
Add a name attribute for each AudioParam

AudioParam's are fairly opaque but it's useful to know that an AudioParam
belongs to a certain node and the name of the AudioParam itself.  This is
especially useful for printing messages related to the AudioParam.

BUG=607699
TEST=none

Committed: https://crrev.com/0f7829c7596aa617ce042877b27e373991e4e682
Cr-Commit-Position: refs/heads/master@{#391102}
==========

[commit-bot: I haz the power, 2016-05-02 23:46:37]

Patchset 5 (id:??) landed as
https://crrev.com/0f7829c7596aa617ce042877b27e373991e4e682
Cr-Commit-Position: refs/heads/master@{#391102}