Implement Google network speech synthesis

Implement Google network speech synthesis.

See bug for context. Implements a component extension
that provides speech synthesis using Google's speech
synthesis API.

Adds a "remote" flag to the TTS and TTS Engine APIs
so that it's possible for clients to distinguish
between local and remote speech engines.

Adds a new private extension API to expose Google's
API key, needed to make the request.

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

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

[dmazzoni, 2013-10-16 21:33:33]

dtseng: please review all
gshires: review use of Google's network speech API
jhawkins: OWNERS review for chrome/browser/resources
kalman: OWNERS review for extensions

[not at google - send to devlin, 2013-10-16 21:41:51]

lgtm, pretty simple extension changes.

the systemPrivate API changes, as an API, are pretty uncontroversial so long as
they stay private. What would happen if an extension managed to capture it? (It
might be able to do so via monkey patching, maybe)? Is it definitely ok to be
exposing a method directly to an extension which uses our API keys? I suppose
the various sync APIs probably already do implicitly.

https://codereview.chromium.org/27034009/diff/6001/chrome/common/extensions/a...
File chrome/common/extensions/api/system_private.json (right):

https://codereview.chromium.org/27034009/diff/6001/chrome/common/extensions/a...
chrome/common/extensions/api/system_private.json:93: "description": "Get API key
to use for requests to Google services.",
s/Get/Gets Chrome's/

[not at google - send to devlin, 2013-10-16 21:44:04]

Oh, some more questions.
- what is the user of tts expected to do if it's remote? What if it's offline?

I am wondering actually whether you should send an email to apps-dev with this
change since it's a public APi change.

[dmazzoni, 2013-10-16 22:19:58]

> the systemPrivate API changes, as an API, are pretty uncontroversial so long
as
> they stay private. What would happen if an extension managed to capture it?
(It
> might be able to do so via monkey patching, maybe)? Is it definitely ok to be
> exposing a method directly to an extension which uses our API keys? I suppose
> the various sync APIs probably already do implicitly.

It's already possible to capture the API key via disassembly or using
chrome://net-internals. I don't think this makes it significantly easier to
intercept.

> - what is the user of tts expected to do if it's remote?

The client is supposed to query all available voices and either present them to
the user, or choose the one that's the most appropriate based on the
application. Some applications might prefer remote speech (good, consistent
quality across platforms), others might prefer local (lower latency, works
offline).

> What if it's offline?

Is there an extension API I could use to determine if Chrome is offline? If so I
could have this component extension remove its event listeners when offline.

> I am wondering actually whether you should send an email to apps-dev with this
> change since it's a public APi change.

Sure.

[dmazzoni, 2013-10-16 22:20:06]

https://codereview.chromium.org/27034009/diff/6001/chrome/common/extensions/a...
File chrome/common/extensions/api/system_private.json (right):

https://codereview.chromium.org/27034009/diff/6001/chrome/common/extensions/a...
chrome/common/extensions/api/system_private.json:93: "description": "Get API key
to use for requests to Google services.",
On 2013/10/16 21:41:51, kalman wrote:
> s/Get/Gets Chrome's/

Done.

[not at google - send to devlin, 2013-10-16 22:23:56]

On 2013/10/16 22:19:58, Dominic Mazzoni wrote:
> > the systemPrivate API changes, as an API, are pretty uncontroversial so long
> as
> > they stay private. What would happen if an extension managed to capture it?
> (It
> > might be able to do so via monkey patching, maybe)? Is it definitely ok to
be
> > exposing a method directly to an extension which uses our API keys? I
suppose
> > the various sync APIs probably already do implicitly.
> 
> It's already possible to capture the API key via disassembly or using
> chrome://net-internals. I don't think this makes it significantly easier to
> intercept.
> 
> > - what is the user of tts expected to do if it's remote?
> 
> The client is supposed to query all available voices and either present them
to
> the user, or choose the one that's the most appropriate based on the
> application. Some applications might prefer remote speech (good, consistent
> quality across platforms), others might prefer local (lower latency, works
> offline).
> 
> > What if it's offline?
> 
> Is there an extension API I could use to determine if Chrome is offline? If so
I
> could have this component extension remove its event listeners when offline.

There might be a web API? If not there might be a Chrome API? If not there was
definitely a proposal for one :) if it's a web API then I'm not sure it's hooked
up to event pages properly.

> 
> > I am wondering actually whether you should send an email to apps-dev with
this
> > change since it's a public APi change.
> 
> Sure.

[dmazzoni, 2013-10-16 22:48:03]

On Wed, Oct 16, 2013 at 3:23 PM, <kalman@chromium.org> wrote:

> There might be a web API? If not there might be a Chrome API? If not there
> was
> definitely a proposal for one :) if it's a web API then I'm not sure it's
> hooked
> up to event pages properly.


Yeah, looks like there's a web api but it's unlikely it would work when the
event page is in the background.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gshires1, 2013-10-17 00:34:50]

https://codereview.chromium.org/27034009/diff/14001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/tts_extension.js (right):

https://codereview.chromium.org/27034009/diff/14001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:27:
'https://www.google.com/speech-api/v2/synthesize?enc=mpeg&client=chrome',
ASR uses client=chromium
(Unless there's other reasoning or precedents, it probably makes sense to be
consistent and use chromium.)

https://codereview.chromium.org/27034009/diff/14001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:66: url +=
'&text=' + escape(utterance);
How long might text be?  (Assuming it could be quite long, might that cause a
problem with a long URL?)
I think it's better to send text as POST data.  (Also, POST might be better in
terms of not having to escape it.)

[dmazzoni, 2013-10-17 17:14:00]

https://codereview.chromium.org/27034009/diff/14001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/tts_extension.js (right):

https://codereview.chromium.org/27034009/diff/14001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:27:
'https://www.google.com/speech-api/v2/synthesize?enc=mpeg&client=chrome',
On 2013/10/17 00:34:50, gshires1 wrote:
> ASR uses client=chromium
> (Unless there's other reasoning or precedents, it probably makes sense to be
> consistent and use chromium.)

Done.

https://codereview.chromium.org/27034009/diff/14001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:66: url +=
'&text=' + escape(utterance);
On 2013/10/17 00:34:50, gshires1 wrote:
> How long might text be?  (Assuming it could be quite long, might that cause a
> problem with a long URL?)
> I think it's better to send text as POST data.  (Also, POST might be better in
> terms of not having to escape it.)

The maximum length is a good point, but I still prefer GET.

As a quick fix, I truncated the string to 32768 characters after escaping.
That's the same maximum we enforce in the TTS extension API already. Chrome can
handle much longer http urls, but really the client ought to be splitting it up
into smaller pieces - that's a lot of text already.

Switching to post would be nontrivial and may not be ideal. Right now we're
setting this url as the source of an html5 <audio> element; this gives us
streaming and caching automatically.

If we wanted to use POST, we'd have to use XmlHttpRequest and then send the
results to the web audio api. That would mean we'd have to wait for the full
audio file to finish downloading first. It also wouldn't be allowed to cache the
audio data in this case.

[David Tseng, 2013-10-17 20:43:49]

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:515: std::string
network_speech_synthesis_id = Add(
I'd love to see some tests (i.e. fetching the current list of voices, making
sure we see the new network tts voice, checking it has remote set to true, etc).

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/manifest.json (right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:18:
"event_types": [ "start", "end", "error" ],
interrupted?

What happens when a client of chrome.tts calls stop()?

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:25: "gender":
"male",
The only male in the bunch? Is there a choice of male or female offered by the
network tts?

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:67: "gender":
"female",
Are the genders subject to change? How did you generate this list of genders?

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:69:
"voice_name": "Google Network Speech Mandarin Chinese"
Do these voice names get localized at some point?

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/tts_extension.js (right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:22: function
TtsExtension() {
Do you want to declare this function globally?
Perhaps do:
var TtsExtension = function () {};

(Also use JS doc).

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:26:
speechServerUrl_:
@private, @const; SPEECH_SERVER_URL_

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:31:
audioElement_: null,
JS doc for all of these please.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:55: //
Truncate the utterance if it's too long.
Why not just chunk the utterance? (and queue up additional utterances to be
requested).

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:61:
this.onStop_();
Can we get this engine to respect queue mode? (perhaps maintaining a queue of
utterances and clearing on flush).

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:74: url +=
'&speed=' + (options.rate / 2.0);
Can you comment on the range expected?

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:77: url +=
'&pitch=' + (options.pitch / 2.0);
Ditto.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:80: url +=
'&vol=' + options.volume;
Couldn't we just handle volume directly in the playback?

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:85:
console.log(String(err));
console.error

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

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_extension_api_constants.cc:12: const
char kRemoteKey[] = "remote";
Should this list be sorted?

[David Tseng, 2013-10-17 20:56:29]

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/tts_extension.js (right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:25:
TtsExtension.prototype = {
I personally don't mind this style of js, but let's stick with the standard JS
style guide and do
TtsExtension.prototype.foo = function()

and initialize your instance members above in the constructor.

[dmazzoni, 2013-10-18 16:23:18]

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:515: std::string
network_speech_synthesis_id = Add(
On 2013/10/17 20:43:49, David Tseng wrote:
> I'd love to see some tests (i.e. fetching the current list of voices, making
> sure we see the new network tts voice, checking it has remote set to true,
etc).

Sure. I added a basic test to load the voices and make sure
they're remote.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/manifest.json (right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:18:
"event_types": [ "start", "end", "error" ],
On 2013/10/17 20:43:49, David Tseng wrote:
> interrupted?
> 
> What happens when a client of chrome.tts calls stop()?

Interrupted and Cancelled are implemented automatically by TtsController. An
extension implementing the tts_engine api doesn't have to implement it.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:25: "gender":
"male",
On 2013/10/17 20:43:49, David Tseng wrote:
> The only male in the bunch? Is there a choice of male or female offered by the
> network tts?

Glen can correct me if I'm wrong, but currently the network speech API doesn't
have a parameter to request a gender or a specific voice. You can only specify
the language and it gives you the best match.

Hopefully we can upgrade this in the future.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:67: "gender":
"female",
On 2013/10/17 20:43:49, David Tseng wrote:
> Are the genders subject to change? How did you generate this list of genders?

I tested it out and made this match what's currently served.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:69:
"voice_name": "Google Network Speech Mandarin Chinese"
On 2013/10/17 20:43:49, David Tseng wrote:
> Do these voice names get localized at some point?

It would be possible to get these localized but we'd have to put them through
the translation console manually and stick the translated resluts into the
extension's messages file. Do you think we should localize each of these strings
into every language?

The other option would just be to just use a language's local name, i.e.
Francais, Deutch, etc. but not translate the rest. Does that sound better in the
interim, or not worth it?

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/tts_extension.js (right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:22: function
TtsExtension() {
On 2013/10/17 20:43:49, David Tseng wrote:
> Do you want to declare this function globally?
> Perhaps do:
> var TtsExtension = function () {};
> 
> (Also use JS doc).

I think this is the Chrome idiomatic style for declaring a class, and plus
there's no reason not to make it a global since it's just an extension event
page. Added @constructor to jsdoc.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:25:
TtsExtension.prototype = {
On 2013/10/17 20:56:30, David Tseng wrote:
> I personally don't mind this style of js, but let's stick with the standard JS
> style guide and do
> TtsExtension.prototype.foo = function()
> 
> and initialize your instance members above in the constructor.

I don't have a strong preference, I was just trying to copy the idiomatic style
of Chrome javascript, especially other code in chrome/browser/resources such as
ntp4 and options. If there's a specific push towards another style I'm happy to
go with that.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:26:
speechServerUrl_:
On 2013/10/17 20:43:49, David Tseng wrote:
> @private, @const; SPEECH_SERVER_URL_

Done.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:31:
audioElement_: null,
On 2013/10/17 20:43:49, David Tseng wrote:
> JS doc for all of these please.

Done.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:55: //
Truncate the utterance if it's too long.
On 2013/10/17 20:43:49, David Tseng wrote:
> Why not just chunk the utterance? (and queue up additional utterances to be
> requested).

Both Chrome's tts extension api and the web speech api specify 32k as the
maximum limit for an utterance. I think it's better to have a limit like this -
that's quite a lot of text already - rather than worry about potential bugs or
memory leaks that could happen with unreasonably large input.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:61:
this.onStop_();
On 2013/10/17 20:43:49, David Tseng wrote:
> Can we get this engine to respect queue mode? (perhaps maintaining a queue of
> utterances and clearing on flush).

The queue is managed by Chrome. A ttsEngine just needs to handle one utterance
at a time.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:74: url +=
'&speed=' + (options.rate / 2.0);
On 2013/10/17 20:43:49, David Tseng wrote:
> Can you comment on the range expected?

Done.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:77: url +=
'&pitch=' + (options.pitch / 2.0);
On 2013/10/17 20:43:49, David Tseng wrote:
> Ditto.

Done.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:80: url +=
'&vol=' + options.volume;
On 2013/10/17 20:43:49, David Tseng wrote:
> Couldn't we just handle volume directly in the playback?

Sure, done. Actually I see one potential advantage of that is that it could
cache the same utterance to play at a different volume.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:85:
console.log(String(err));
On 2013/10/17 20:43:49, David Tseng wrote:
> console.error

Done.

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

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_extension_api_constants.cc:12: const
char kRemoteKey[] = "remote";
On 2013/10/17 20:43:49, David Tseng wrote:
> Should this list be sorted?

Sure, I sorted within each grouping.

[David Tseng, 2013-10-18 16:37:35]

On Fri, Oct 18, 2013 at 9:23 AM, <dmazzoni@chromium.org> wrote:

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/extensions...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/extensions...
chrome/browser/extensions/component_loader.cc:515: std::string
network_speech_synthesis_id = Add(
On 2013/10/17 20:43:49, David Tseng wrote:
I'd love to see some tests (i.e. fetching the current list of voices,
making
sure we see the new network tts voice, checking it has remote set to
true, etc).

Sure. I added a basic test to load the voices and make sure
they're remote.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/manifest.json
(right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:18:
"event_types": [ "start", "end", "error" ],
On 2013/10/17 20:43:49, David Tseng wrote:
interrupted?

What happens when a client of chrome.tts calls stop()?

Interrupted and Cancelled are implemented automatically by
TtsController. An extension implementing the tts_engine api doesn't have
to implement it.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:25:
"gender": "male",
On 2013/10/17 20:43:49, David Tseng wrote:
The only male in the bunch? Is there a choice of male or female
offered by the
network tts?

Glen can correct me if I'm wrong, but currently the network speech API
doesn't have a parameter to request a gender or a specific voice. You
can only specify the language and it gives you the best match.

Hopefully we can upgrade this in the future.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:67:
"gender": "female",
On 2013/10/17 20:43:49, David Tseng wrote:
Are the genders subject to change? How did you generate this list of
genders?

I tested it out and made this match what's currently served.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/manifest.json:69:
"voice_name": "Google Network Speech Mandarin Chinese"
On 2013/10/17 20:43:49, David Tseng wrote:
Do these voice names get localized at some point?

It would be possible to get these localized but we'd have to put them
through the translation console manually and stick the translated
resluts into the extension's messages file. Do you think we should
localize each of these strings into every language?

That would be nice (perhaps rather than a readable string here, provide a
message id).
I just think it's a little silly to hear "Mandarin Chinese" if you're in a
Chinese locale spoken badly with a Chinese voice…:)

The other option would just be to just use a language's local name, i.e.
Francais, Deutch, etc. but not translate the rest. Does that sound
better in the interim, or not worth it?

Sure. That sounds good. Actually, for certain clients, ideal because the
voice presumably would be loaded once the client wants to read the voice
name (so the name is read properly).

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/network_speech_synthesis/tts_extension.js
(right):

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:22:
function TtsExtension() {
On 2013/10/17 20:43:49, David Tseng wrote:
Do you want to declare this function globally?
Perhaps do:
var TtsExtension = function () {};

(Also use JS doc).

I think this is the Chrome idiomatic style for declaring a class, and
plus there's no reason not to make it a global since it's just an
extension event page. Added @constructor to jsdoc.

I've seen all sorts of styles in Chrome.
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml

is what we're following I think unless there's a documented Chrome style
guide somewhere I'm not aware of.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:25:
TtsExtension.prototype = {
On 2013/10/17 20:56:30, David Tseng wrote:
I personally don't mind this style of js, but let's stick with the
standard JS
style guide and do
TtsExtension.prototype.foo = function()

and initialize your instance members above in the constructor.

I don't have a strong preference, I was just trying to copy the
idiomatic style of Chrome javascript, especially other code in
chrome/browser/resources such as ntp4 and options. If there's a specific
push towards another style I'm happy to go with that.

See above.

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:26:
speechServerUrl_:
On 2013/10/17 20:43:49, David Tseng wrote:
@private, @const; SPEECH_SERVER_URL_

Done.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:31:
audioElement_: null,
On 2013/10/17 20:43:49, David Tseng wrote:
JS doc for all of these please.

Done.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:55:
// Truncate the utterance if it's too long.
On 2013/10/17 20:43:49, David Tseng wrote:
Why not just chunk the utterance? (and queue up additional utterances
to be
requested).

Both Chrome's tts extension api and the web speech api specify 32k as
the maximum limit for an utterance. I think it's better to have a limit
like this - that's quite a lot of text already - rather than worry about
potential bugs or memory leaks that could happen with unreasonably large
input.

I was thinking more of chunking the requests (i.e. only request the first
chunk; once that first chunk finishes reading, then make the request for
the second chunk). Basically, treat the speak call like a series of queued
up utterances.
Stop would clear the queued up utterances (strings).

Suppose I were a book reader (and I dumped an entire page of text into one
call).


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:61:
this.onStop_();
On 2013/10/17 20:43:49, David Tseng wrote:
Can we get this engine to respect queue mode? (perhaps maintaining a
queue of
utterances and clearing on flush).

The queue is managed by Chrome. A ttsEngine just needs to handle one
utterance at a time.

Realized that after I wrote this :). Why is the stop call necessary then?


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:74:
url += '&speed=' + (options.rate / 2.0);
On 2013/10/17 20:43:49, David Tseng wrote:
Can you comment on the range expected?

Done.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:77:
url += '&pitch=' + (options.pitch / 2.0);
On 2013/10/17 20:43:49, David Tseng wrote:
Ditto.

Done.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:80:
url += '&vol=' + options.volume;
On 2013/10/17 20:43:49, David Tseng wrote:
Couldn't we just handle volume directly in the playback?

Sure, done. Actually I see one potential advantage of that is that it
could cache the same utterance to play at a different volume.


https://codereview.chromium.org/27034009/diff/22001/chrome/browser/resources/...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:85:
console.log(String(err));
On 2013/10/17 20:43:49, David Tseng wrote:
console.error

Done.


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

https://codereview.chromium.org/27034009/diff/22001/chrome/browser/speech/ext...
chrome/browser/speech/extension_api/tts_extension_api_constants.cc:12:
const char kRemoteKey[] = "remote";
On 2013/10/17 20:43:49, David Tseng wrote:
Should this list be sorted?

Sure, I sorted within each grouping.


https://codereview.chromium.org/27034009/

To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni at google, 2013-10-18 22:07:26]

OK, multiple voices for the same language with different genders are now
supported, and the language names are translated.

I'll let one of the OWNERS of chrome/browser/resources make the final call
on style, because it's really unclear to me.

On Fri, Oct 18, 2013 at 9:37 AM, David Tseng <dtseng@chromium.org> wrote:

> The other option would just be to just use a language's local name, i.e.
>
Francais, Deutch, etc. but not translate the rest. Does that sound
> better in the interim, or not worth it?
>
> Sure. That sounds good. Actually, for certain clients, ideal because the
> voice presumably would be loaded once the client wants to read the voice
> name (so the name is read properly).
>

Done, take a look.

is what we're following I think unless there's a documented Chrome style
> guide somewhere I'm not aware of.
>

Is it inconsistent with that? Most of what I see in
chrome/browser/resources uses the ClassName.prototype = {} pattern.


> I was thinking more of chunking the requests (i.e. only request the first
> chunk; once that first chunk finishes reading, then make the request for
> the second chunk). Basically, treat the speak call like a series of queued
> up utterances.
>

Might be a nice optimization. The server is supposed to stream; iet's try
this and optimize later if needed.

Realized that after I wrote this :). Why is the stop call necessary then?
>

It's really only as a precaution. In theory it should be a no-op, but I
didn't want this code to have to make that assumption.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2013-10-21 21:03:07]

estade: could you do an OWNERS review for chrome/browser/resources?

dtseng: another look?

[dtseng, 2013-10-22 20:45:38]

LGTM

Leaving JS style up to chrome/browser/resources OWNERS.


On Fri, Oct 18, 2013 at 3:07 PM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> OK, multiple voices for the same language with different genders are now
> supported, and the language names are translated.
>
> I'll let one of the OWNERS of chrome/browser/resources make the final call
> on style, because it's really unclear to me.
>
> On Fri, Oct 18, 2013 at 9:37 AM, David Tseng <dtseng@chromium.org> wrote:
>
>> The other option would just be to just use a language's local name, i.e.
>>
>  Francais, Deutch, etc. but not translate the rest. Does that sound
>> better in the interim, or not worth it?
>>
>> Sure. That sounds good. Actually, for certain clients, ideal because the
>> voice presumably would be loaded once the client wants to read the voice
>> name (so the name is read properly).
>>
>
> Done, take a look.
>
> is what we're following I think unless there's a documented Chrome style
>> guide somewhere I'm not aware of.
>>
>
> Is it inconsistent with that? Most of what I see in
> chrome/browser/resources uses the ClassName.prototype = {} pattern.
>
>
>> I was thinking more of chunking the requests (i.e. only request the first
>> chunk; once that first chunk finishes reading, then make the request for
>> the second chunk). Basically, treat the speak call like a series of queued
>> up utterances.
>>
>
> Might be a nice optimization. The server is supposed to stream; iet's try
> this and optimize later if needed.
>
> Realized that after I wrote this :). Why is the stop call necessary then?
>>
>
> It's really only as a precaution. In theory it should be a no-op, but I
> didn't want this code to have to make that assumption.
>
>  To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2013-10-22 22:32:13]

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
File chrome/browser/resources/network_speech_synthesis/tts_extension.js (right):

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:8: * powered
by Google's speech synthesis API.
I don't see where TTS is defined in this file. Explanation of that
initialization would be helpful imo.

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:23: function
TtsExtension() {
nit: {} on one line

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:35:
'https://www.google.com/speech-api/v2/synthesize?enc=mpeg&client=chromium',
80

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:123: //
Firsttop any pending audio.
First stop?

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:151: if
(voiceName)
nit: more vertical whitespace somewhere in this vicinity

[Evan Stade, 2013-10-22 22:32:23]

(and lgtm)

[dmazzoni, 2013-10-22 22:41:02]

+thakis for chrome/browser/browser_resources.grd

[dmazzoni, 2013-10-22 22:45:29]

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
File chrome/browser/resources/network_speech_synthesis/tts_extension.js (right):

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:8: * powered
by Google's speech synthesis API.
On 2013/10/22 22:32:13, Evan Stade wrote:
> I don't see where TTS is defined in this file. Explanation of that
> initialization would be helpful imo.

Clarified the acronym here, thanks.

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:23: function
TtsExtension() {
On 2013/10/22 22:32:13, Evan Stade wrote:
> nit: {} on one line

Done.

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:35:
'https://www.google.com/speech-api/v2/synthesize?enc=mpeg&client=chromium',
On 2013/10/22 22:32:13, Evan Stade wrote:
> 80

Done.

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:123: //
Firsttop any pending audio.
On 2013/10/22 22:32:13, Evan Stade wrote:
> First stop?

Done.

https://codereview.chromium.org/27034009/diff/128001/chrome/browser/resources...
chrome/browser/resources/network_speech_synthesis/tts_extension.js:151: if
(voiceName)
On 2013/10/22 22:32:13, Evan Stade wrote:
> nit: more vertical whitespace somewhere in this vicinity

Done.

[Nico, 2013-10-22 22:54:18]

grd change lgtm

[commit-bot: I haz the power, 2013-10-23 17:32:27]

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

[commit-bot: I haz the power, 2013-10-23 18:09:49]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-10-23 18:20:25]

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

[commit-bot: I haz the power, 2013-10-23 21:48:31]

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

[commit-bot: I haz the power, 2013-10-25 23:07:15]

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

[commit-bot: I haz the power, 2013-10-26 01:54:07]

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

[commit-bot: I haz the power, 2013-10-31 20:45:34]

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

[commit-bot: I haz the power, 2013-10-31 23:10:55]

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

[commit-bot: I haz the power, 2013-11-01 00:10:34]

Change committed as 232242

[commit-bot: I haz the power, 2013-11-01 05:33:06]

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

[commit-bot: I haz the power, 2013-11-01 09:13:14]

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

[commit-bot: I haz the power, 2013-11-01 15:03:56]

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

[commit-bot: I haz the power, 2013-11-01 16:45:00]

Change committed as 232412