Support audio ducking and suspension in ChromeVox

Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts events.

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

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

[David Tseng, 2016-12-09 18:04:58]

Description was changed from

==========
Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts
events.

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

to

==========
Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts
events.

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

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

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

[dmazzoni, 2016-12-09 18:12:49]

Description was changed from

==========
Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts
events.

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

to

==========
Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts
events.

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

[dmazzoni, 2016-12-09 18:30:11]

Let's confirm the status of the flag this depends on,
I want to make sure this isn't a wasted effort.
If it's going to be enabled eventually, maybe add
a comment in the code noting that this feature won't
work without a certain flag.

https://codereview.chromium.org/2563013003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html
(right):

https://codereview.chromium.org/2563013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:40:
Enable audio ducking.
How about: Duck other audio when ChromeVox is speaking

https://codereview.chromium.org/2563013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:47:
Enable audio suspension.
How about radio buttons? It doesn't make sense to me to
support both checkboxes when there's no point to having
them both on.

Something like, "When speaking, (a) Do nothing, (b) Duck other audio, (c)
Suspend other audio"

https://codereview.chromium.org/2563013003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js
(right):

https://codereview.chromium.org/2563013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js:84:
'useAudioDucking': false,
Nit: sort these

https://codereview.chromium.org/2563013003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/media_automation_handler.js
(right):

https://codereview.chromium.org/2563013003/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/cvox2/background/media_automation_handler.js:37:
if (localStorage['useAudioDucking'] == 'true')
It doesn't make sense to me to do both.
I'd prefer something like:

if (localStorage['audioPolicy'] == 'duck')
  ...
else if (localStorage['audioPolicy'] == 'suspend')
  ...

[David Tseng, 2016-12-12 23:39:40]

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

[David Tseng, 2016-12-12 23:39:41]

+mlamouri. Could you please comment on the enable default media session flag and
its eventual promotion to default on desktop? Thanks.

[mlamouri (slow - plz ping), 2016-12-13 13:37:37]

We are still working on this feature and hope to start experimenting in the wild
early 2017.

[David Tseng, 2016-12-13 15:18:20]

On Tue, Dec 13, 2016 at 5:37 AM, <mlamouri@chromium.org> wrote:

> We are still working on this feature and hope to start experimenting in
> the wild
> early 2017.
>
> Are you open to turning this on by default for accessibility (limited only
to Chrome OS)?


> https://codereview.chromium.org/2563013003/
>
> --
> 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.
>

-- 
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.

[David Tseng, 2016-12-13 15:20:07]

On Tue, Dec 13, 2016 at 7:18 AM, David Tseng <dtseng@chromium.org> wrote:

>
>
> On Tue, Dec 13, 2016 at 5:37 AM, <mlamouri@chromium.org> wrote:
>
>> We are still working on this feature and hope to start experimenting in
>> the wild
>> early 2017.
>>
>> Are you open to turning this on by default for accessibility (limited
> only to Chrome OS)?
>
>
>> https://codereview.chromium.org/2563013003/
>>
>> --
>> 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.
>>
>
>

-- 
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.

[mlamouri (slow - plz ping), 2016-12-13 15:25:55]

On 2016/12/13 at 15:18:20, dtseng wrote:
> On Tue, Dec 13, 2016 at 5:37 AM, <mlamouri@chromium.org> wrote:
> 
> > We are still working on this feature and hope to start experimenting in
> > the wild
> > early 2017.
> >
> > Are you open to turning this on by default for accessibility (limited only
> to Chrome OS)?

I wouldn't make this decision in a code review :) Maybe we should discuss this
over email. We are going to do some real changes in Q1 so at least you should be
ready for these. Maybe you should start an email thread or open a launch bug?

[David Tseng, 2016-12-13 20:45:26]

PTAL dmazzoni@.

Addressed feedback. I'm not opposed to having a lower level way to do this, but
I think it would be great to get this in sooner and possibly have us whitelisted
for the default media session.

[dmazzoni, 2016-12-13 21:02:19]

lgtm

Add a comment somewhere in the code
indicating this requires the --enable-default-media-session
flag, otherwise someone will find the code and try to make
it work and get confused that it doesn't.

I'm also wondering if the UI should be hidden if the
flag is not set?

Otherwise lgtm with nits

https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html
(right):

https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:52:
</select>
nit: indent

https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js
(right):

https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js:66: //
TODO(dtseng): Leaking state about multiple key maps here until we have a
nit: looks like indentation accidentally changed here?

https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
(right):

https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:478:
When playing audio
Add an ellipsis at the end

https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:481:
play at normal volume even if ChromeVox is speaking
Capitalize the first letter

I know it's finishing the sentence "When playing audio", but the general style
is still initial caps

[David Tseng, 2016-12-13 21:22:17]

dmazzoni@chromium.org writes:

> lgtm
>
> Add a comment somewhere in the code
> indicating this requires the --enable-default-media-session
> flag, otherwise someone will find the code and try to make
> it work and get confused that it doesn't.
>

Good idea; done.

> I'm also wondering if the UI should be hidden if the
> flag is not set?

Yup; makes sense; done.
>
> Otherwise lgtm with nits
>
>
>
>
https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html
> (right):
>
>
https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/chromevox/background/options.html:52:
> </select>
> nit: indent
>
Done.
>
https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js
> (right):
>
>
https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/chromeos/chromevox/chromevox/background/prefs.js:66:
> // TODO(dtseng): Leaking state about multiple key maps here until we
> have a
> nit: looks like indentation accidentally changed here?
Done
>
>
https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
> File
> chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd
> (right):
>
>
https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:478:
> When playing audio
> Add an ellipsis at the end
Done
>
>
https://codereview.chromium.org/2563013003/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/chromeos/chromevox/strings/chromevox_strings.grd:481:
> play at normal volume even if ChromeVox is speaking
> Capitalize the first letter
Done (for the two below as well)
>
> I know it's finishing the sentence "When playing audio", but the general
> style is still initial caps
>
> https://codereview.chromium.org/2563013003/


-- 

http://www.example.com

-- 
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.

[David Tseng, 2016-12-13 21:25:09]

dtseng@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[David Tseng, 2016-12-13 21:25:09]

+ rdevlin for automation.js.

[Devlin, 2016-12-13 21:38:39]

https://codereview.chromium.org/2563013003/diff/100001/third_party/closure_co...
File third_party/closure_compiler/externs/automation.js (right):

https://codereview.chromium.org/2563013003/diff/100001/third_party/closure_co...
third_party/closure_compiler/externs/automation.js:5: // This file was generated
by:
This is a generated file, but I don't see any modifications in the idl in this
patch set?

[David Tseng, 2016-12-13 21:55:49]

rdevlin.cronin@chromium.org writes:

>
https://codereview.chromium.org/2563013003/diff/100001/third_party/closure_co...
> File third_party/closure_compiler/externs/automation.js (right):
>
>
https://codereview.chromium.org/2563013003/diff/100001/third_party/closure_co...
> third_party/closure_compiler/externs/automation.js:5: // This file was
> generated by:
> This is a generated file, but I don't see any modifications in the idl
> in this patch set?
>
It was in the dependent/parent change for ax_enums.idl. Unused there, used
here.


c/r/e/automation_internal_custom_bindings.cc

> https://codereview.chromium.org/2563013003/


-- 

http://www.example.com

-- 
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.

[mlamouri (slow - plz ping), 2016-12-14 12:26:00]

mlamouri@chromium.org changed reviewers:
- mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-12-14 12:26:01]

Removing myself from reviewers because it looks like you don't need me. Please
add me back if I misunderstood.

[David Tseng, 2016-12-14 17:23:38]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 17:24:16]

Dry run: 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-14 17:33:20]

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

[commit-bot: I haz the power, 2016-12-14 17:33:21]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Devlin, 2016-12-14 22:24:20]

On 2016/12/13 21:55:49, David Tseng wrote:
> mailto:rdevlin.cronin@chromium.org writes:
> 
> >
>
https://codereview.chromium.org/2563013003/diff/100001/third_party/closure_co...
> > File third_party/closure_compiler/externs/automation.js (right):
> >
> >
>
https://codereview.chromium.org/2563013003/diff/100001/third_party/closure_co...
> > third_party/closure_compiler/externs/automation.js:5: // This file was
> > generated by:
> > This is a generated file, but I don't see any modifications in the idl
> > in this patch set?
> >
> It was in the dependent/parent change for ax_enums.idl. Unused there, used
> here.
> 
> 
> c/r/e/automation_internal_custom_bindings.cc
> 
> > https://codereview.chromium.org/2563013003/
> 
> 
> -- 
> 
> http://www.example.com
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 

From patching in the cl and running the generation command, it looks like the
externs file varies pretty wildly from the generated version.  Is this one we
maintain by hand?  If so, why doesn't the tool work?

[David Tseng, 2016-12-14 23:32:57]

rdevlin.cronin@chromium.org writes:

> On 2016/12/13 21:55:49, David Tseng wrote:
>> mailto:rdevlin.cronin@chromium.org writes:
>
>> >
>
>
https://codereview.chromium.org/2563013003/diff/100001/third_party/closure_co...
>> > File third_party/closure_compiler/externs/automation.js (right):
>> >
>> >
>
>
https://codereview.chromium.org/2563013003/diff/100001/third_party/closure_co...
>> > third_party/closure_compiler/externs/automation.js:5: // This file was
>> > generated by:
>> > This is a generated file, but I don't see any modifications in the idl
>> > in this patch set?
>> >
>> It was in the dependent/parent change for ax_enums.idl. Unused there, used
>> here.
>

Yes, most of this is by hand.

For one, afaik, the idl's, don't really support classes, instance members,
right? So, it can't really be expressed accurately from what I can tell. The idl
parser needs fixing.

>
>> c/r/e/automation_internal_custom_bindings.cc
>
>> > https://codereview.chromium.org/2563013003/
>
>
>> --
>
>> http://www.example.com
>
>> --
>> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>  From patching in the cl and running the generation command, it looks like  
> the
> externs file varies pretty wildly from the generated version.  Is this one  
> we
> maintain by hand?  If so, why doesn't the tool work?
>
> https://codereview.chromium.org/2563013003/


-- 
Foo X. Bar
http://www.example.com

-- 
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.

[David Tseng, 2016-12-18 17:44:31]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-12-18 17:44:32]

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/2563013003/#ps120001
(title: "Interruption capture")

[commit-bot: I haz the power, 2016-12-18 17:44:35]

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

[David Tseng, 2016-12-18 17:46:40]

dtseng@chromium.org changed reviewers:
- rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-12-18 18:31:03]

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

[commit-bot: I haz the power, 2016-12-18 18:31:04]

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-29 03:42:59]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 03:43:12]

Dry run: 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-29 03:57:24]

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

[commit-bot: I haz the power, 2016-12-29 03:57:25]

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

[David Tseng, 2016-12-29 04:22:32]

The CQ bit was checked by dtseng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 04:22:50]

Dry run: 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-29 05:20:43]

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

[commit-bot: I haz the power, 2016-12-29 05:20:44]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2016-12-29 05:49:28]

The CQ bit was checked by dtseng@chromium.org

[David Tseng, 2016-12-29 05:49:28]

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/2563013003/#ps160001
(title: "Fix compile.")

[commit-bot: I haz the power, 2016-12-29 05:49:34]

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-29 05:53:23]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1482990568271330,
"parent_rev": "34dcfa72a583c777e63b4258533c91f80ec9c889", "commit_rev":
"517ce71a2bfa5e23da1bc75315052ba4688c4733"}

[commit-bot: I haz the power, 2016-12-29 05:53:51]

Description was changed from

==========
Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts
events.

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

to

==========
Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts
events.

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

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

[commit-bot: I haz the power, 2016-12-29 05:53:53]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2017-01-02 15:51:49]

Description was changed from

==========
Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts
events.

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

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

to

==========
Support audio ducking and suspension in ChromeVox

- adds options to automatically duck/unduck, suspend/resume audio based on tts
events.

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

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

[commit-bot: I haz the power, 2017-01-02 15:51:50]

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