Initial commit of new ChromeVox earcon engine.

Initial commit of new ChromeVox earcon engine.

A follow-up changelist will hook up Output to this new engine.

BUG=491243
Committed: https://crrev.com/553a404f19bbfe44153a83947f5010bbe77cb764
Cr-Commit-Position: refs/heads/master@{#348677}

[dmazzoni, 2015-08-07 21:17:48]

dmazzoni@chromium.org changed reviewers:
+ plundblad@chromium.org

[dmazzoni, 2015-08-07 21:17:48]

Peter, could you review this part of the earcon code? I'll integrate it later.

[Peter Lundblad, 2015-08-11 11:15:29]

lgtm

Bunch of minor comments below.  It'll be nice to play with these
in context.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi (right):

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi:89:
'chromevox_assets_cvox2_background_earcons': [
Need to add this variable to the sources list in the chromevox_copied_files
target in BUILD.gn.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js
(right):

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:29:
/** @type {number} The volume of the static sound, as an ampl factor. */
nit: be consistent ampl vs. amplification

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:33:
this.baseDelay = 45;
Since many other times are in whole seconds, should be be consistent and make
all times in either seconds or milliseconds?

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:41:
/** @type {string} The choice of the reverb impulse response to use. */
Say that this must we one of the items of EarconEngine.REVERBS

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:53:
/** @type {number} The delay between sounds in the on/off sweep effect. */
.. in ms ?

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:56:
/** @type {number} The delay between echos in the on/off sweep. */
In ms ?

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:90:
this.progressSources_ = null;
Do you need the distinction between null and empty array here?

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:96:
this.progressT_ = null;
Is it very beneficial to use T instead of Time here?  Also, document when this
is null.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:98:
/** @type {?number} The window.setInterval ID for progress sounds.
nit: start and end comment tokens on their own lines.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:103:
EarconEngine.SOUNDS.forEach((function(sound) {
Could save some repetitive code by concatenating the lists.
(The reason I suggest this here where it might seem marginally useful
is pu8rely readability.)  Your call.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:114:
* @type {Array<string>} The list of sound data files to load.
@const

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:124:
* @type {Array<string>} The list of reverb data files to load.
@const

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:130:
* @type {number} The scale factor for one half-step.
@const

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:135:
* @type {string} The base url for earcon sound resources.
@cosnt

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:142:
* @param {string} sound The name of the sound to load.
nit: call name?

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:188:
if (properties.pan) {
Compare to undefined here. Else, a caller can't set pan to 0 (in case masterPan
was changed).  I'd recommend that for all numeric optional parameters
because it is fairly subttle.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:201:
reverb = properties.reverb;
Indentation and please be consistent with one-line substatements either having
braces or not.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:240:
*     the default pitch, gain, pan, and reverb.
nit: odd line break above. (short line)

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:436:
if (!properties.time) {
Can you avoid modifying the passed in properties?

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:524:
freqDecay = Math.pow(0.75, pitches.length - j);
Indentation.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:532:
gainNode.gain.setValueAtTime(gain, startTime + j * this.sweepDelay);
Seems like the second arguments on these two lines could be simplfied
since j is 0.

[dmazzoni, 2015-09-14 18:06:38]

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi (right):

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/chromevox_assets.gypi:89:
'chromevox_assets_cvox2_background_earcons': [
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> Need to add this variable to the sources list in the chromevox_copied_files
> target in BUILD.gn.

Done, thanks.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js
(right):

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:29:
/** @type {number} The volume of the static sound, as an ampl factor. */
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> nit: be consistent ampl vs. amplification

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:33:
this.baseDelay = 45;
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> Since many other times are in whole seconds, should be be consistent and make
> all times in either seconds or milliseconds?

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:41:
/** @type {string} The choice of the reverb impulse response to use. */
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> Say that this must we one of the items of EarconEngine.REVERBS

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:53:
/** @type {number} The delay between sounds in the on/off sweep effect. */
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> .. in ms ?

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:56:
/** @type {number} The delay between echos in the on/off sweep. */
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> In ms ?

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:90:
this.progressSources_ = null;
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> Do you need the distinction between null and empty array here?

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:96:
this.progressT_ = null;
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> Is it very beneficial to use T instead of Time here?  Also, document when this
> is null.

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:98:
/** @type {?number} The window.setInterval ID for progress sounds.
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> nit: start and end comment tokens on their own lines.

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:103:
EarconEngine.SOUNDS.forEach((function(sound) {
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> Could save some repetitive code by concatenating the lists.
> (The reason I suggest this here where it might seem marginally useful
> is pu8rely readability.)  Your call.

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:114:
* @type {Array<string>} The list of sound data files to load.
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> @const

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:124:
* @type {Array<string>} The list of reverb data files to load.
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> @const

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:130:
* @type {number} The scale factor for one half-step.
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> @const

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:135:
* @type {string} The base url for earcon sound resources.
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> @cosnt

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:142:
* @param {string} sound The name of the sound to load.
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> nit: call name?

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:188:
if (properties.pan) {
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> Compare to undefined here. Else, a caller can't set pan to 0 (in case
masterPan
> was changed).  I'd recommend that for all numeric optional parameters
> because it is fairly subttle.

Good catch.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:201:
reverb = properties.reverb;
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> Indentation and please be consistent with one-line substatements either having
> braces or not.

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:240:
*     the default pitch, gain, pan, and reverb.
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> nit: odd line break above. (short line)

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:436:
if (!properties.time) {
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> Can you avoid modifying the passed in properties?

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:524:
freqDecay = Math.pow(0.75, pitches.length - j);
On 2015/08/11 11:15:29, Peter Lundblad wrote:
> Indentation.

Done.

https://codereview.chromium.org/1273363004/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:532:
gainNode.gain.setValueAtTime(gain, startTime + j * this.sweepDelay);
On 2015/08/11 11:15:28, Peter Lundblad wrote:
> Seems like the second arguments on these two lines could be simplfied
> since j is 0.

Done.

[dmazzoni, 2015-09-14 18:18:41]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-09-14 18:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
plundblad@chromium.org
Link to the patchset: https://codereview.chromium.org/1273363004/#ps40001
(title: "Address feedback from Peter")

[commit-bot: I haz the power, 2015-09-14 18:19:12]

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

[commit-bot: I haz the power, 2015-09-14 19:05:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-14 19:06:54]

Patchset 3 (id:??) landed as
https://crrev.com/553a404f19bbfe44153a83947f5010bbe77cb764
Cr-Commit-Position: refs/heads/master@{#348677}

[commit-bot: I haz the power, 2015-09-23 12:35:28]

Patchset 3 (id:??) landed as
https://crrev.com/553a404f19bbfe44153a83947f5010bbe77cb764
Cr-Commit-Position: refs/heads/master@{#348677}