Support basic stereo pan positioning for earcons

Support basic stereo pan positioning for earcons

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/e4d9f70ee5a04ea13bef02797eeb0e7014c29951
Cr-Commit-Position: refs/heads/master@{#437538}

[David Tseng, 2016-12-05 18:46:09]

Description was changed from

==========
Support basic stereo pan positioning for earcons

BUG=
==========

to

==========
Support basic stereo pan positioning for earcons

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-12-05 18:52:12]

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

[dmazzoni, 2016-12-05 20:07:45]

Neat!

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

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:162:
EarconEngine.MAX_PAN_X_POSITION = 2;
I'd call it something like ABS_POSITION or MAGNITUDE to clarify that the range
is from -2 to 2

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:220:
panNode.setOrientation(0, -1, 0);
Are you sure this is right? Did the original orientation not work? If these are
x, y, z coordinates then 0, 0, 1 would be towards the computer and 0, -1, 0
would be straight up, I think.

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:729:
var left = (rect.left + rect.width / 2) / container.width;
Maybe just x? It's really the midpoint of the rect, not the left

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:732:
left = left > 1.0 ? 1.0 : left;
x = Math.max(Math.min(x, 1.0), 0.0) or something like that?

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:737:
left = left * 2 * EarconEngine.MAX_PAN_X_POSITION -
x = (2 * x - 1) * EarconEngine.MAX_PAN_X_POSITION

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

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/next_earcons.js:54:
var rect = node.location;
Could you use this.locations_ instead, which is computed for the focus ring
calculation? The nice thing about sharing it with that code already calls
boundsForRange and taken the whole range into account, so it will work for an
individual word or a range spanning multiple nodes

[David Tseng, 2016-12-08 19:05:38]

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

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:162:
EarconEngine.MAX_PAN_X_POSITION = 2;
On 2016/12/05 20:07:45, dmazzoni wrote:
> I'd call it something like ABS_POSITION or MAGNITUDE to clarify that the range
> is from -2 to 2

Sure; done.

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:220:
panNode.setOrientation(0, -1, 0);
On 2016/12/05 20:07:45, dmazzoni wrote:
> Are you sure this is right? Did the original orientation not work? If these
are
> x, y, z coordinates then 0, 0, 1 would be towards the computer and 0, -1, 0
> would be straight up, I think.

I was fiddling with the audio listener earlier. Reverted this change as it
doesn't seem to matter which direction this points (probably because they're all
fairly close). (I think I was visualizing the 3d plane rotated 90 degrees from
this in the usual Cartesian sense where z goes up/down).

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:729:
var left = (rect.left + rect.width / 2) / container.width;
On 2016/12/05 20:07:45, dmazzoni wrote:
> Maybe just x? It's really the midpoint of the rect, not the left

Done.

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:732:
left = left > 1.0 ? 1.0 : left;
On 2016/12/05 20:07:45, dmazzoni wrote:
> x = Math.max(Math.min(x, 1.0), 0.0) or something like that?

Done.

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/earcon_engine.js:737:
left = left * 2 * EarconEngine.MAX_PAN_X_POSITION -
On 2016/12/05 20:07:45, dmazzoni wrote:
> x = (2 * x - 1) * EarconEngine.MAX_PAN_X_POSITION

Done.

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

https://codereview.chromium.org/2550223002/diff/1/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/chromevox/cvox2/background/next_earcons.js:54:
var rect = node.location;
On 2016/12/05 20:07:45, dmazzoni wrote:
> Could you use this.locations_ instead, which is computed for the focus ring
> calculation? The nice thing about sharing it with that code already calls
> boundsForRange and taken the whole range into account, so it will work for an
> individual word or a range spanning multiple nodes

I made it so we're plumbing through rects from the playEarcon call sites. I
don't know if we need word level resolution, so leaving it there at node level
resolution for ranges with multiple nodes.

[dmazzoni, 2016-12-08 21:36:01]

lgtm

I think sub-node positioning might sound really cool, but
either way this is great.

[David Tseng, 2016-12-08 22:01:19]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-12-08 22:02:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 23:25:04]

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

[commit-bot: I haz the power, 2016-12-08 23:25:06]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Tseng, 2016-12-08 23:44:46]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-12-08 23:44:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2550223002/#ps40001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-12-08 23:45:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 23:56:05]

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

[commit-bot: I haz the power, 2016-12-08 23:56:05]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[David Tseng, 2016-12-09 14:18:55]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2016-12-09 14:19:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 15:08:40]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481293135138560,
"parent_rev": "612c157fb50de8222bab6f47b3e6b465ec733884", "commit_rev":
"36b4fae51a96aa64908c2fba97c38dee8aad5de2"}

[commit-bot: I haz the power, 2016-12-09 15:09:08]

Description was changed from

==========
Support basic stereo pan positioning for earcons

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Support basic stereo pan positioning for earcons

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2550223002
==========

[commit-bot: I haz the power, 2016-12-09 15:09:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-09 15:12:58]

Description was changed from

==========
Support basic stereo pan positioning for earcons

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2550223002
==========

to

==========
Support basic stereo pan positioning for earcons

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/e4d9f70ee5a04ea13bef02797eeb0e7014c29951
Cr-Commit-Position: refs/heads/master@{#437538}
==========

[commit-bot: I haz the power, 2016-12-09 15:12:59]

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