Add Pause and Resume to Web TTS & Extension TTS APIs

Add Pause and Resume to Web TTS & Extension TTS APIs

The web speech spec already includes pause and resume, this
completes the implementation. For parity, this change also adds
support for Pause and Resume to Chrome's TTS extension API
and TTS Engine extension APIs.

BUG=171887
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203146

[dmazzoni, 2013-05-13 05:41:58]

dtseng: TTS code
kalman: OWNERS review for extension api code

[not at google - send to devlin, 2013-05-13 14:46:53]

extensions lgtm

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/ext...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:151:
DispatchEventToExtension(utterance->extension_id(), event.Pass());
Consider sending a warning to the console of the target extension if it listens
to onPause but not onResume or vice versa.

I forget where the code is, but look for where the IPC for
ExtensionMsg_AddMessageToConsole originates.

https://codereview.chromium.org/15108002/diff/22001/chrome/common/extensions/...
File chrome/common/extensions/api/tts_engine.json (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/common/extensions/...
chrome/common/extensions/api/tts_engine.json:109: "description": "Optional: if
an engine supports the pause event, it should also support the resume event, to
continue speaking the current utterance, if any. Note that a stop or speak event
also implies resuming."
What does "implies"  mean? That the extension should do it, or that Chrome tells
the extension to do it? (just clarify)

[David Tseng, 2013-05-13 17:49:54]

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
File chrome/browser/speech/tts_controller.cc (left):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
chrome/browser/speech/tts_controller.cc:136: } else {
What happens if we're currently paused and get a non-enqueued utterance?
Shouldn't that flush what's on the queue?

Also, can_enqueue seems like a bad name; how about should_enqueue (or just
enqueue)?

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
File chrome/browser/speech/tts_controller.cc (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
chrome/browser/speech/tts_controller.cc:135: if (paused_ || (IsSpeaking() &&
utterance->can_enqueue())) {
I don't remember why isSpeaking is needed here. It seems wrong now that I think
about it since a speak call can occur in-between queued up calls to native
speak(). What do you think?

[dmazzoni, 2013-05-13 19:34:38]

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
File chrome/browser/speech/tts_controller.cc (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
chrome/browser/speech/tts_controller.cc:135: if (paused_ || (IsSpeaking() &&
utterance->can_enqueue())) {
On 2013/05/13 17:49:54, David Tseng wrote:
> I don't remember why isSpeaking is needed here. It seems wrong now that I
think
> about it since a speak call can occur in-between queued up calls to native
> speak(). What do you think?

Are you thinking about the change we made on Mac where IsSpeaking checks
the platform speech synthesizer and queues even if anything is speaking?
If you set that aside for the moment, checking IsSpeaking is correct here -
if the user sets the enqueue flag, then we add it to the queue if another
utterance is already speaking. If the user doesn't set the enqueue flag,
then we always speak now.

Is that what you were worried about, or something else?

[David Tseng, 2013-05-13 20:17:26]

Still looking at the rest of the patch; the other comment still applies.


On Mon, May 13, 2013 at 12:34 PM, <dmazzoni@chromium.org> wrote:

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
File chrome/browser/speech/tts_controller.cc (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
chrome/browser/speech/tts_controller.cc:135: if (paused_ ||
(IsSpeaking() && utterance->can_enqueue())) {
On 2013/05/13 17:49:54, David Tseng wrote:
I don't remember why isSpeaking is needed here. It seems wrong now
that I think
about it since a speak call can occur in-between queued up calls to
native
speak(). What do you think?

Are you thinking about the change we made on Mac where IsSpeaking checks
the platform speech synthesizer and queues even if anything is speaking?
If you set that aside for the moment, checking IsSpeaking is correct
here -
if the user sets the enqueue flag, then we add it to the queue if
another
utterance is already speaking. If the user doesn't set the enqueue flag,
then we always speak now.


Is that what you were worried about, or something else?

It was just a passing comment because of some reports on Windows of us
dropping queued up utterances.

I'm worried about the case when you do something like (not sure if this is
possible):

speakFlush('a')
// Between calls when above has finished speaking.
speakQueued('b')

speakQueued('c') // Called when isSpeaking returns false between calls
above, so this would flush.

https://codereview.chromium.org/15108002/

[David Tseng, 2013-05-13 21:22:48]

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
File chrome/browser/speech/tts_linux.cc (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
chrome/browser/speech/tts_linux.cc:187: libspeechd_loader_.spd_pause(conn_);
Any issues with calling pause (or resume) repeatedly? I noticed you check under
win if we're not already paused before calling pause, but not for linux or mac.

[dmazzoni, 2013-05-14 16:50:27]

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/ext...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:151:
DispatchEventToExtension(utterance->extension_id(), event.Pass());
On 2013/05/13 14:46:53, kalman wrote:
> Consider sending a warning to the console of the target extension if it
listens
> to onPause but not onResume or vice versa.
> 
> I forget where the code is, but look for where the IPC for
> ExtensionMsg_AddMessageToConsole originates.

You were thinking of UIThreadExtensionFunction::WriteToConsole,
but that's for logging a message to the page that called an extension
function. I had to add a bit more code to log a message to the extension
background page's console - please take a look.

For the record, this isn't needed for onSpeak and onStop because it won't
even route speech to an extension that implements just one of those two.

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
File chrome/browser/speech/tts_controller.cc (left):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
chrome/browser/speech/tts_controller.cc:136: } else {
On 2013/05/13 17:49:54, David Tseng wrote:
> What happens if we're currently paused and get a non-enqueued utterance?
> Shouldn't that flush what's on the queue?

Done.

> Also, can_enqueue seems like a bad name; how about should_enqueue (or just
> enqueue)?

Can enqueue means that it is allowed to be enqueued if it can't be spoken now.
If it can be spoken now, it will be.

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
File chrome/browser/speech/tts_controller.cc (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
chrome/browser/speech/tts_controller.cc:135: if (paused_ || (IsSpeaking() &&
utterance->can_enqueue())) {
> David Tseng wrote:
> I'm worried about the case when you do something like
> (not sure if this is possible):
>
> speakFlush('a')
> // Between calls when above has finished speaking.
> speakQueued('b')
> speakQueued('c') // Called when isSpeaking returns false between calls
> above, so this would flush.

Got it. That won't happen because IsSpeaking checks current_utterance_,
which will be set if there's an utterance that we've called Speak on but
hasn't actually started speaking yet. IsSpeaking will never return false
while we have anything in the queue.

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
File chrome/browser/speech/tts_linux.cc (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/browser/speech/tts...
chrome/browser/speech/tts_linux.cc:187: libspeechd_loader_.spd_pause(conn_);
On 2013/05/13 21:22:48, David Tseng wrote:
> Any issues with calling pause (or resume) repeatedly? I noticed you check
under
> win if we're not already paused before calling pause, but not for linux or
mac.

The Linux speech dispatcher is the only one that actually fires a pause
and resume event which we can forward directly, so we don't need to
duplicate that logic.

On Mac: we create a new synthesizer for each utterance, so
calling Stop automatically clears the paused state. However, it
could have sent duplicate pause events, so I went ahead and added
a flag.

https://codereview.chromium.org/15108002/diff/22001/chrome/common/extensions/...
File chrome/common/extensions/api/tts_engine.json (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/common/extensions/...
chrome/common/extensions/api/tts_engine.json:109: "description": "Optional: if
an engine supports the pause event, it should also support the resume event, to
continue speaking the current utterance, if any. Note that a stop or speak event
also implies resuming."
On 2013/05/13 14:46:53, kalman wrote:
> What does "implies"  mean? That the extension should do it, or that Chrome
tells
> the extension to do it? (just clarify)

Clarified: the extension should exit the paused state.

[dmazzoni, 2013-05-20 14:57:45]

Hey, just a reminder that I'm ready for both of you to take another look -
thanks!

[not at google - send to devlin, 2013-05-20 17:29:53]

some more minor comments after which lgtm

https://codereview.chromium.org/15108002/diff/43001/chrome/browser/speech/ext...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/15108002/diff/43001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:42: Profile*
profile, EventRouter* event_router, std::string extension_id) {
passing in the profile and event router would be unnecessary if this were a
method.

https://codereview.chromium.org/15108002/diff/43001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:57:
constants::kErrorMissingPauseOrResume));
this is pretty handy, I think there are lots of places where API implementations
could write warnings to the console... we just don't, and at least one reason is
because it's a bit of a pain.

So maybe you should add a version of UIThreadExtensionFunction::WriteToConsole
which takes a custom RVH to write to? I'd just overload WriteToConsole...

https://codereview.chromium.org/15108002/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/api/tts.json (right):

https://codereview.chromium.org/15108002/diff/43001/chrome/common/extensions/...
chrome/common/extensions/api/tts.json:193: "parameters": []
incidentally maybe all of these "parameters: []" should be removed, the schema
code can all handle it not being there so it just means we're creating a bunch
of unnecessary empty lists.

[David Tseng, 2013-05-20 17:39:50]

https://codereview.chromium.org/15108002/diff/22001/chrome/common/extensions/...
File chrome/common/extensions/api/tts_engine.json (right):

https://codereview.chromium.org/15108002/diff/22001/chrome/common/extensions/...
chrome/common/extensions/api/tts_engine.json:104: "description": "Optional: if
an engine supports the pause event, it should pause the current utterance being
spoken, if any, until a resume, stop, or speak event is fired, all of which
leave the paused state."
Is this true? The tts controller continues to have paused set inside of the
speak call for non-enqueued utterances?

[David Tseng, 2013-05-20 17:44:19]

https://codereview.chromium.org/15108002/diff/43001/chrome/common/extensions/...
File chrome/common/extensions/api/tts.json (right):

https://codereview.chromium.org/15108002/diff/43001/chrome/common/extensions/...
chrome/common/extensions/api/tts.json:186: "description": "Pauses speech
synthesis, potentially in the middle of an utterance. A call to resume or stop
will un-pause speech.",
Should speak also unpause speech?

[dmazzoni, 2013-05-21 03:12:43]

On 2013/05/20 17:44:19, David Tseng wrote:
> chrome/common/extensions/api/tts.json:186: "description": "Pauses speech
> synthesis, potentially in the middle of an utterance. A call to resume or stop
> will un-pause speech.",
> Should speak also unpause speech?

In this case, I'd rather stay consistent with the web speech api, which says
this for "speak": If the SpeechSynthesis instance is paused, it remains paused.

That allows you to pause, then queue up a bunch of speech, then resume to begin
speaking whatever was first in the queue.

[dmazzoni, 2013-05-21 04:56:27]

https://codereview.chromium.org/15108002/diff/43001/chrome/browser/speech/ext...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/15108002/diff/43001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:42: Profile*
profile, EventRouter* event_router, std::string extension_id) {
On 2013/05/20 17:29:53, kalman wrote:
> passing in the profile and event router would be unnecessary if this were a
> method.

Method of what class? This isn't being called from a class, it's being called
from a global "event dispatcher" of sorts.

https://codereview.chromium.org/15108002/diff/43001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:57:
constants::kErrorMissingPauseOrResume));
On 2013/05/20 17:29:53, kalman wrote:
> this is pretty handy, I think there are lots of places where API
implementations
> could write warnings to the console... we just don't, and at least one reason
is
> because it's a bit of a pain.
> 
> So maybe you should add a version of UIThreadExtensionFunction::WriteToConsole
> which takes a custom RVH to write to? I'd just overload WriteToConsole...

A method in UIThreadExtensionFunction wouldn't help here - this is being called
from code that dispatches an event to an extension, not from an extension
function
handler.

I could implement a global function like this:

    WriteToExtensionConsole(Profile* profile, const string& extension_id);

That seems a little more useful than the RVH.

[David Tseng, 2013-05-21 16:50:41]

https://codereview.chromium.org/15108002/diff/53001/chrome/common/extensions/...
File chrome/common/extensions/api/tts_engine.json (right):

https://codereview.chromium.org/15108002/diff/53001/chrome/common/extensions/...
chrome/common/extensions/api/tts_engine.json:104: "description": "Optional: if
an engine supports the pause event, it should pause the current utterance being
spoken, if any, until a resume, stop, or speak event is fired, all of which
should clear the paused state."
In light of the other comment, shouldn't the engine always receive resume before
receiving a speak event? (i.e. remove speak from the list above).

[not at google - send to devlin, 2013-05-21 17:22:54]

https://codereview.chromium.org/15108002/diff/43001/chrome/browser/speech/ext...
File chrome/browser/speech/extension_api/tts_engine_extension_api.cc (right):

https://codereview.chromium.org/15108002/diff/43001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_engine_extension_api.cc:42: Profile*
profile, EventRouter* event_router, std::string extension_id) {
On 2013/05/21 04:56:28, Dominic Mazzoni wrote:
> On 2013/05/20 17:29:53, kalman wrote:
> > passing in the profile and event router would be unnecessary if this were a
> > method.
> 
> Method of what class? This isn't being called from a class, it's being called
> from a global "event dispatcher" of sorts.

ah sorry, misread and assumed this was on an extension function not global.
never mind.

[dmazzoni, 2013-05-28 07:37:31]

https://codereview.chromium.org/15108002/diff/53001/chrome/common/extensions/...
File chrome/common/extensions/api/tts_engine.json (right):

https://codereview.chromium.org/15108002/diff/53001/chrome/common/extensions/...
chrome/common/extensions/api/tts_engine.json:104: "description": "Optional: if
an engine supports the pause event, it should pause the current utterance being
spoken, if any, until a resume, stop, or speak event is fired, all of which
should clear the paused state."
On 2013/05/21 16:50:41, David Tseng wrote:
> In light of the other comment, shouldn't the engine always receive resume
before
> receiving a speak event? (i.e. remove speak from the list above).

Agreed.

After thinking about this some more, the current code will never tell and engine
to Start
while it's in the Paused state. After a Pause, it will only get either a Resume
or a Stop.
Clarified the comment to indicate that either Resume or Stop should clear the
paused
state.

[David Tseng, 2013-05-28 16:30:51]

lgtm


On Tue, May 28, 2013 at 12:37 AM, <dmazzoni@chromium.org> wrote:

>
> https://codereview.chromium.**org/15108002/diff/53001/**
>
chrome/common/extensions/api/**tts_engine.json<https://codereview.chromium.org/15108002/diff/53001/chrome/common/extensions/api/tts_engine.json>
> File chrome/common/extensions/api/**tts_engine.json (right):
>
> https://codereview.chromium.**org/15108002/diff/53001/**
>
chrome/common/extensions/api/**tts_engine.json#newcode104<https://codereview.chromium.org/15108002/diff/53001/chrome/common/extensions/api/tts_engine.json#newcode104>
> chrome/common/extensions/api/**tts_engine.json:104: "description":
> "Optional: if an engine supports the pause event, it should pause the
> current utterance being spoken, if any, until a resume, stop, or speak
> event is fired, all of which should clear the paused state."
> On 2013/05/21 16:50:41, David Tseng wrote:
>
>> In light of the other comment, shouldn't the engine always receive
>>
> resume before
>
>> receiving a speak event? (i.e. remove speak from the list above).
>>
>
> Agreed.
>
> After thinking about this some more, the current code will never tell
> and engine to Start
> while it's in the Paused state. After a Pause, it will only get either a
> Resume or a Stop.
> Clarified the comment to indicate that either Resume or Stop should
> clear the paused
> state.
>
>
https://codereview.chromium.**org/15108002/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-05-29 05:01:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/15108002/80001

[commit-bot: I haz the power, 2013-05-29 07:05:04]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-30 13:59:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/15108002/104001

[commit-bot: I haz the power, 2013-05-30 15:17:23]

Change committed as 203146