Stop utterances from a tab when that tab is closed.

Stop utterances from a tab when that tab is closed.

BUG=402957, 418806
Committed: https://crrev.com/8880b279a491ca69db564a975cce3bbfd8f5c3e3
Cr-Commit-Position: refs/heads/master@{#302987}

[dmazzoni, 2014-10-31 20:23:28]

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

[David Tseng, 2014-10-31 22:33:04]

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
File chrome/browser/speech/tts_controller_impl.cc (right):

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:478: if (current_utterance_ &&
!current_utterance_->extension_id().empty()) {
Why the redundant check for current_utterance?

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:479: #if !defined(OS_ANDROID)
Can this happen? (i.e. an utterance with an extension id set on Android).

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:484:
GetPlatformImpl()->clear_error();
What's the purpose of doing this?

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:485:
GetPlatformImpl()->StopSpeaking();
Is this behavior part of the web speech spec?I'm curious what it says for other
browser navigation like refresh or user navigation. Would this code cover those
cases?

What if I wanted to speak a goodbye message during navigation for example?

[dmazzoni, 2014-10-31 22:44:22]

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
File chrome/browser/speech/tts_controller_impl.cc (right):

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:478: if (current_utterance_ &&
!current_utterance_->extension_id().empty()) {
On 2014/10/31 22:33:04, David Tseng wrote:
> Why the redundant check for current_utterance?

Fixed.

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:479: #if !defined(OS_ANDROID)
On 2014/10/31 22:33:04, David Tseng wrote:
> Can this happen? (i.e. an utterance with an extension id set on Android).

No. I copied this from other code where we handle Stop, but I think we can get
rid of the ifdef now. Previously this was code that only compiled on a system
with extensions, but now that's been abstracted so the ifdef isn't needed.

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:484:
GetPlatformImpl()->clear_error();
On 2014/10/31 22:33:04, David Tseng wrote:
> What's the purpose of doing this?

We do this before every call to the platform TTS so that if it calls back with
an error we know it's from the latest request.

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:485:
GetPlatformImpl()->StopSpeaking();
On 2014/10/31 22:33:04, David Tseng wrote:
> Is this behavior part of the web speech spec?I'm curious what it says for
other
> browser navigation like refresh or user navigation. Would this code cover
those
> cases?
> 
> What if I wanted to speak a goodbye message during navigation for example?

See crbug.com/418806, there are concerns about a really long message that the
user would have no way to stop, and adware, and things like that.

The thought is that it should behave like any other tab audio; when you close
the tab it's gone.

If the user really wants something to say goodbye, they should probably install
an extension or app version, or maybe even something like service workers that
can run when a tab isn't active.

[David Tseng, 2014-10-31 22:52:28]

lgtm

[David Tseng, 2014-11-02 19:55:44]

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
File chrome/browser/speech/tts_controller_impl.cc (right):

https://codereview.chromium.org/692203002/diff/1/chrome/browser/speech/tts_co...
chrome/browser/speech/tts_controller_impl.cc:485:
GetPlatformImpl()->StopSpeaking();
On 2014/10/31 22:44:22, dmazzoni wrote:
> On 2014/10/31 22:33:04, David Tseng wrote:
> > Is this behavior part of the web speech spec?I'm curious what it says for
> other
> > browser navigation like refresh or user navigation. Would this code cover
> those
> > cases?
> > 
> > What if I wanted to speak a goodbye message during navigation for example?
> 
> See crbug.com/418806, there are concerns about a really long message that the
> user would have no way to stop, and adware, and things like that.
> 
> The thought is that it should behave like any other tab audio; when you close
> the tab it's gone.
> 
> If the user really wants something to say goodbye, they should probably
install
> an extension or app version, or maybe even something like service workers that
> can run when a tab isn't active.

Are there any concerns over really long utterances in general (not just after
tab close)? Any thoughts given to chunking? Obviously, not for this cl.

[dmazzoni, 2014-11-03 16:36:01]

On 2014/11/02 19:55:44, David Tseng wrote:
> Are there any concerns over really long utterances in general (not just after
> tab close)? Any thoughts given to chunking? Obviously, not for this cl.

The network speech extension has a low character limit and we need to add
chunking there. There's a bug filed for that.

Do you think we need to do chunking for native speech?

[dmazzoni, 2014-11-03 16:39:21]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-11-03 16:40:27]

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

[commit-bot: I haz the power, 2014-11-03 17:01:35]

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

[commit-bot: I haz the power, 2014-11-03 17:01:36]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Ken Russell (switch to Gerrit), 2014-11-03 19:53:41]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2014-11-03 19:53:41]

The various GPU test failures with:
[700:1799:1103/091947:FATAL:lock.cc(20)] Check failed:
owning_thread_ref_.is_null().

look real. Please contact me if you have any trouble reproducing them. See
http://www.chromium.org/developers/testing/gpu-testing for more info.

[David Tseng, 2014-11-03 20:15:24]

On 2014/11/03 16:36:01, dmazzoni wrote:
> On 2014/11/02 19:55:44, David Tseng wrote:
> > Are there any concerns over really long utterances in general (not just
after
> > tab close)? Any thoughts given to chunking? Obviously, not for this cl.
> 
> The network speech extension has a low character limit and we need to add
> chunking there. There's a bug filed for that.
> 
> Do you think we need to do chunking for native speech?

We should test on Windows as I don't recall how well it performs under those
conditions.

[dmazzoni, 2014-11-04 00:09:53]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-11-04 00:11:16]

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

[Ken Russell (switch to Gerrit), 2014-11-04 00:23:52]

On 2014/11/04 00:11:16, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/692203002/20001

The random test failures in e.g.
http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...
look both real, and caused by this CL. Please investigate them.

[3911:3911:1103/162113:FATAL:lock.cc(20)] Check failed:
owning_thread_ref_.is_null().

[dmazzoni, 2014-11-04 00:27:41]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2014-11-04 00:28:12]

On 2014/11/04 00:23:52, Ken Russell wrote:
> On 2014/11/04 00:11:16, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/692203002/20001
> 
> The random test failures in e.g.
>
http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...
> look both real, and caused by this CL. Please investigate them.
> 
> [3911:3911:1103/162113:FATAL:lock.cc(20)] Check failed:
> owning_thread_ref_.is_null().

Sure, I'll uncheck the commit box and try to repro locally.

[dmazzoni, 2014-11-06 07:55:25]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-11-06 07:56:30]

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

[commit-bot: I haz the power, 2014-11-06 08:45:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-06 08:45:32]

Patchset 4 (id:??) landed as
https://crrev.com/8880b279a491ca69db564a975cce3bbfd8f5c3e3
Cr-Commit-Position: refs/heads/master@{#302987}