Android implementation of text-to-speech code for Web Speech Synthesis API

Android implementation of text-to-speech code for Web Speech Synthesis API

BUG=171887
TBR=thakis

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

[dmazzoni, 2013-05-14 20:44:10]

@dtrainor: No rush - I was hoping you could review this
since you have a bit of familiarity with the TextToSpeech
API from your accessibility work.

Context: this enables the web speech synthesis API
(https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html)
to work on Android too. The logic is all cross-platform,
and there are already platform-specific implementations for
Mac, Windows, and Linux.

This is my first Android patch for Chrome, so please check
carefully and if in doubt, assume I don't know what I'm
doing. :)

That said, I have tested this and it seems to work fine, and
also the web speech synthesis API is behind a runtime flag
so the risk is low.

[David Trainor- moved to gerrit, 2013-05-15 20:55:28]

Mostly nits.  One comment about adding a destroy() java method.  Aside from that
looks good!

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:41:
initialize();
move this up on the same line as the if() or put {} around it for Java style.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:49:
nativeOnEndEvent(mNativeTtsPlatformImplAndroid,
Maybe check mNativeTtsPlatformImplAndroid for 0?  See tts_android.cc comment.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:78:
public static TtsPlatformImpl create(int nativeTtsPlatformImplAndroid,
Put javadocs on the public methods.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:90:
return mVoices.size();
Would it just be safe to return 0 if mVoices is null?

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:106: if
(lang != mCurrentLanguage) {
return false if !isInitialized() (and/or assert)?

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:115:
params.put(TextToSpeech.Engine.KEY_PARAM_VOLUME, Double.toString(volume));
{ } around this even though it's one line.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:134:
engineLabel = info.label;
Move up to same line as if.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:140:
continue;
Move up to same line as if.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:145:
voice.name += " " + locales[i].getDisplayCountry();
Add { } around this line or move up.

If this is going to be displayed to the user, does it support RTL?  Do we need
to?

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:156:
nativeVoicesChanged(mNativeTtsPlatformImplAndroid);
Maybe add if (mNativeTtsPlatformImplAndroid != 0)?

https://codereview.chromium.org/15012027/diff/1/chrome/browser/speech/tts_and...
File chrome/browser/speech/tts_android.cc (right):

https://codereview.chromium.org/15012027/diff/1/chrome/browser/speech/tts_and...
chrome/browser/speech/tts_android.cc:31:
TtsPlatformImplAndroid::~TtsPlatformImplAndroid() {
I know it probably can't happen, but it's nice to call
Java_TtsPlatformImpl_destroy() and have that clear the internal pointer to this
object, then check that to make sure we don't call back to native.  It's just
safe in case somehow another object has a reference to the Java class (like the
runnable posted in initialize() or the callback inner class).

[dmazzoni, 2013-05-16 07:35:13]

+bulach for approval of chrome/browser/android

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:41:
initialize();
On 2013/05/15 20:55:28, David Trainor wrote:
> move this up on the same line as the if() or put {} around it for Java style.

Done.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:49:
nativeOnEndEvent(mNativeTtsPlatformImplAndroid,
On 2013/05/15 20:55:28, David Trainor wrote:
> Maybe check mNativeTtsPlatformImplAndroid for 0?  See tts_android.cc comment.

Done.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:78:
public static TtsPlatformImpl create(int nativeTtsPlatformImplAndroid,
On 2013/05/15 20:55:28, David Trainor wrote:
> Put javadocs on the public methods.

Done.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:90:
return mVoices.size();
On 2013/05/15 20:55:28, David Trainor wrote:
> Would it just be safe to return 0 if mVoices is null?

Sure

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:90:
return mVoices.size();
On 2013/05/15 20:55:28, David Trainor wrote:
> Would it just be safe to return 0 if mVoices is null?

How about I just assert mInitialized == true everywhere?

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:106: if
(lang != mCurrentLanguage) {
On 2013/05/15 20:55:28, David Trainor wrote:
> return false if !isInitialized() (and/or assert)?

I like asserting better, that doesn't imply there's a legitimate reason to call
this when not initialized.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:115:
params.put(TextToSpeech.Engine.KEY_PARAM_VOLUME, Double.toString(volume));
On 2013/05/15 20:55:28, David Trainor wrote:
> { } around this even though it's one line.

Done.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:134:
engineLabel = info.label;
On 2013/05/15 20:55:28, David Trainor wrote:
> Move up to same line as if.

Done.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:140:
continue;
On 2013/05/15 20:55:28, David Trainor wrote:
> Move up to same line as if.

Done.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:145:
voice.name += " " + locales[i].getDisplayCountry();
On 2013/05/15 20:55:28, David Trainor wrote:
> Add { } around this line or move up.

Done.

> If this is going to be displayed to the user, does it support RTL?  Do we need
> to?

Developers have the option to show these to a user, but they're not guaranteed
to be easily human-readable. I believe it will be okay - worst case "U.S.
English" and "English U.S." are equally intelligible.

Also, FWIW, I don't believe there are any TTS voices for RTL languages on
Android yet, but I could be wrong.

https://codereview.chromium.org/15012027/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:156:
nativeVoicesChanged(mNativeTtsPlatformImplAndroid);
On 2013/05/15 20:55:28, David Trainor wrote:
> Maybe add if (mNativeTtsPlatformImplAndroid != 0)?

Done.

https://codereview.chromium.org/15012027/diff/1/chrome/browser/speech/tts_and...
File chrome/browser/speech/tts_android.cc (right):

https://codereview.chromium.org/15012027/diff/1/chrome/browser/speech/tts_and...
chrome/browser/speech/tts_android.cc:31:
TtsPlatformImplAndroid::~TtsPlatformImplAndroid() {
On 2013/05/15 20:55:28, David Trainor wrote:
> I know it probably can't happen, but it's nice to call
> Java_TtsPlatformImpl_destroy() and have that clear the internal pointer to
this
> object, then check that to make sure we don't call back to native.  It's just
> safe in case somehow another object has a reference to the Java class (like
the
> runnable posted in initialize() or the callback inner class).

Done.

[bulach, 2013-05-16 11:10:28]

good stuff, thanks dominic! I have a few suggestions and one question below re:
threading model:

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:23:
public class TtsPlatformImpl {
nit: if this class is only used as a bridge to c++, make it private.. in
general, we tend to use java visibility rule for java code, so I guess all
methods in this class could be private as well, since they're only
@CalledByNative or by this class itself..

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:24:
private class TtsVoice {
nit: static

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:25:
public String name;
nit: mName, mLanguage
also, they don't need to be public, but they could be final

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:41:
initialize();
not clear to me what is the threading model here.
is it initialize() called in the UI thread? should it be? :)
my question here is that the UtteranceProgressListener is yielding (back?) to
the UI thread, but this one is not, so probably worth a comment.

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
File chrome/browser/speech/tts_android.cc (right):

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.cc:52: if (success) {
nit: the "else" is unecessary, and it'd probably be easier to read as:

if (!success)
  return false;
utterance_ = utterance;
utterance_id_ = utterance_id;
return true;

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.cc:82: VoiceData& data = out_voices->back();
I think we don't normally use mutable refs.. presumably VoiceData is
copy-constructable, right?
perhaps:
 VoiceData data;
 data.native = true;
 data.... = ...;
 ...
 out_voices->push_back(data);

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.cc:107: utterance_.clear();
nit: 100-107 is fairly similar to 112-118, so perhaps extract it into a
"SendFinalTTSEvent(utterance_id, event, size)" ?

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
File chrome/browser/speech/tts_android.h (right):

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.h:17: 
nit: 
// TtsPlatformImpl methods:
also, could do with removing some of the \n

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.h:33: //
nit: maybe remove the extra // here and 40-42 below

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.h:57: friend struct
DefaultSingletonTraits<TtsPlatformImplAndroid>;
nit: friend decl should go right after the private: lavel

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.h:62: 
nit: remove extra \n

[dmazzoni, 2013-05-16 16:36:40]

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:23:
public class TtsPlatformImpl {
On 2013/05/16 11:10:28, bulach wrote:
> nit: if this class is only used as a bridge to c++, make it private.. in
> general, we tend to use java visibility rule for java code, so I guess all
> methods in this class could be private as well, since they're only
> @CalledByNative or by this class itself..

I'm not sure I understand - I thought a top-level class can't be private.

I made all of the methods private.

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:24:
private class TtsVoice {
On 2013/05/16 11:10:28, bulach wrote:
> nit: static

Done.

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:25:
public String name;
On 2013/05/16 11:10:28, bulach wrote:
> nit: mName, mLanguage
> also, they don't need to be public, but they could be final

Done.

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:41:
initialize();
On 2013/05/16 11:10:28, bulach wrote:
> not clear to me what is the threading model here.
> is it initialize() called in the UI thread? should it be? :)
> my question here is that the UtteranceProgressListener is yielding (back?) to
> the UI thread, but this one is not, so probably worth a comment.

I added a class comment to clarify: all C++ calls must happen on the UI
thread. Android callbacks may occur on a different thread, so we have to
go back to the UI thread when calling back to C++.

I think you're right about initialize, though - to be safe, I changed it
so that it calls initialized on the UI thread. Now there's no need for
anything to be synchronized because anything that changes the
state of this class happens on the UI thread.

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
File chrome/browser/speech/tts_android.cc (right):

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.cc:52: if (success) {
On 2013/05/16 11:10:28, bulach wrote:
> nit: the "else" is unecessary, and it'd probably be easier to read as:
> 
> if (!success)
>   return false;
> utterance_ = utterance;
> utterance_id_ = utterance_id;
> return true;

Done.

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.cc:82: VoiceData& data = out_voices->back();
On 2013/05/16 11:10:28, bulach wrote:
> I think we don't normally use mutable refs..

I'm using this pattern consistently in chrome/browser/speech and it's sometimes
used elsewhere - I agree it requires caution, but I think it's allowed when used
safely.

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.cc:107: utterance_.clear();
On 2013/05/16 11:10:28, bulach wrote:
> nit: 100-107 is fairly similar to 112-118, so perhaps extract it into a
> "SendFinalTTSEvent(utterance_id, event, size)" ?

Done.

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
File chrome/browser/speech/tts_android.h (right):

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.h:17: 
On 2013/05/16 11:10:28, bulach wrote:
> nit: 
> // TtsPlatformImpl methods:
> also, could do with removing some of the \n

Done.

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.h:33: //
On 2013/05/16 11:10:28, bulach wrote:
> nit: maybe remove the extra // here and 40-42 below

Done.

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.h:57: friend struct
DefaultSingletonTraits<TtsPlatformImplAndroid>;
On 2013/05/16 11:10:28, bulach wrote:
> nit: friend decl should go right after the private: lavel

Done.

https://codereview.chromium.org/15012027/diff/6001/chrome/browser/speech/tts_...
chrome/browser/speech/tts_android.h:62: 
On 2013/05/16 11:10:28, bulach wrote:
> nit: remove extra \n

Done.

[bulach, 2013-05-17 10:09:04]

thanks for the clarifications! rietveld is not happy and I can't see a few files
:-/ however, I think we're all good :) just some clarifications and tiny nits
below.

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://codereview.chromium.org/15012027/diff/6001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:23:
public class TtsPlatformImpl {
On 2013/05/16 16:36:40, Dominic Mazzoni wrote:
> On 2013/05/16 11:10:28, bulach wrote:
> > nit: if this class is only used as a bridge to c++, make it private.. in
> > general, we tend to use java visibility rule for java code, so I guess all
> > methods in this class could be private as well, since they're only
> > @CalledByNative or by this class itself..
> 
> I'm not sure I understand - I thought a top-level class can't be private.
> 
> I made all of the methods private.

sorry, my bad. :) I meant the class would be "package private", i.e., without
the "public" modifier.
just to clarify:
"public" would mean that _any_ other java class could use it, which is not the
case.
"package private" restricts a bit more.
the methods themselves can all be "private", since they're only used by native..
not sure if it's any clearer :) but please let me know if it makes any sense..

https://codereview.chromium.org/15012027/diff/16001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://codereview.chromium.org/15012027/diff/16001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:38:
private TextToSpeech mTextToSpeech;
nit: this could be "final"

https://codereview.chromium.org/15012027/diff/16001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:106:
Context context) {
nit: align with parens

https://codereview.chromium.org/15012027/diff/16001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:170:
float rate, float pitch, float volume) {
nit: align with parens

[dmazzoni, 2013-05-17 16:41:58]

> sorry, my bad. :) I meant the class would be "package private", i.e., without
> the "public" modifier.

Oh, duh. Sorry, it's been a while since I've done Java, I forgot about package
private.

https://codereview.chromium.org/15012027/diff/16001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://codereview.chromium.org/15012027/diff/16001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:38:
private TextToSpeech mTextToSpeech;
On 2013/05/17 10:09:04, bulach wrote:
> nit: this could be "final"

Done.

https://codereview.chromium.org/15012027/diff/16001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:106:
Context context) {
On 2013/05/17 10:09:04, bulach wrote:
> nit: align with parens

Done.

https://codereview.chromium.org/15012027/diff/16001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:170:
float rate, float pitch, float volume) {
On 2013/05/17 10:09:04, bulach wrote:
> nit: align with parens

Done.

[David Trainor- moved to gerrit, 2013-05-17 18:03:33]

lgtm

lgtm but let Marcus sign off too.

https://chromiumcodereview.appspot.com/15012027/diff/41001/chrome/android/jav...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://chromiumcodereview.appspot.com/15012027/diff/41001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:202: //
Note: Android supports multiple speech engines, but querying the
assert mNativeTtsPlatformImplAndroid != 0; ?

[dmazzoni, 2013-05-18 20:47:15]

Trybots look happy!

https://codereview.chromium.org/15012027/diff/41001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java
(right):

https://codereview.chromium.org/15012027/diff/41001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TtsPlatformImpl.java:202: //
Note: Android supports multiple speech engines, but querying the
On 2013/05/17 18:03:33, David Trainor wrote:
> assert mNativeTtsPlatformImplAndroid != 0; ?

Done.

[bulach, 2013-05-20 05:50:01]

lgtm, thanks!

[commit-bot: I haz the power, 2013-05-20 05:50:50]

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

[dmazzoni, 2013-05-20 05:54:04]

TBR: thakis for chrome/browser/chrome_content_browser_client.cc

[commit-bot: I haz the power, 2013-05-20 14:56:52]

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

[commit-bot: I haz the power, 2013-05-20 16:21:56]

Change committed as 201091

[Nico, 2013-05-20 20:05:59]

lgtm