1184142 - Support SpeechSynthesis API in Android

Status: RESOLVED FIXED

(Core :: Web Speech, defect)

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Implement SpeechService for Android

This is WIP.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1184142 - Support SpeechSynthesis API in Android.

Review commit: https://reviewboard.mozilla.org/r/51331/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51331/

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 8750257 [details]
Bug 1184142 - Support SpeechSynthesis API in Android.

https://reviewboard.mozilla.org/r/51331/#review48129

This looks really good! I can't find any real issues besides the one below. It is a shame Android doesn't have a more modern TTS service with pause, concurrent speech and boundary events.

::: mobile/android/base/java/org/mozilla/gecko/SpeechSynthesisService.java:210
(Diff revision 1)
> +    }
> +
> +    private void stop() {
> +        // stop method will discard other utteraces in the queue
> +        mTTS.stop();
> +        sendEventToGecko("end", mCurrentUtteranceId);

You aren't using onStop because it requires API 23? Is there no method in the listener that gets called as well? (onEnd, onError). I'm concerned that devices below API 23 use onEnd or onError when stop is called, and then you would get a redundant event. If you are certain this is not the case, then great!

Comment 4

[:Margaret Leibovic]

What's the plan to use this in the product? Is the idea to have text-to-speech in reader view, similar to desktop Firefox? Have you talked to our product/ux teams about how this should work?

I'm happy to review this patch, but I want to understand how you plan to use this new service.

Instead of passing messages over the bridge, would it make more sense to use JNI to implement this natively? It pains me to see yet another long-lived singleton in GeckoApp.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

SpeechSynthesis is a web API
https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#tts-section
So, not necessarily anything Firefox should use, but what web pages could use.

Comment 6

[Makoto Kato [:m_kato]]

(In reply to :Margaret Leibovic from comment #4)
> What's the plan to use this in the product? Is the idea to have
> text-to-speech in reader view, similar to desktop Firefox? Have you talked
> to our product/ux teams about how this should work?

As Web Speech Synthesis API, we are already turned on Nightly.  Also this will be turned on release channel of Firefox 49.

On Desktop browser, Chrome and Safari already support it and upcoming Edge 14 supports it.
On Mobile browser, Chrome Android and Safari Mobile already support it.
 
> Instead of passing messages over the bridge, would it make more sense to use
> JNI to implement this natively? It pains me to see yet another long-lived
> singleton in GeckoApp.

Could we handle/implement callback/listener interface on our JNI bridge?

Also, NDK has C header for TTS Engine developer, there is no C/C++ API for speech consumer.

Comment 7

[:Margaret Leibovic]

Ah, thanks for clarifying. I was confused by the addition of an XPCOM component, so I assumed this would be a privileged API.

I think we should get input from snorp or jchen about the best way to integrate this into Fennec.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://reviewboard.mozilla.org/r/51331/#review48843

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1549
(Diff revision 1)
>  
>          mContactService = new ContactService(EventDispatcher.getInstance(), this);
>  
>          mPromptService = new PromptService(this);
>  
> +        mSpeechSynthesisService = new SpeechSynthesisService();

Yeah, I'm not thrilled about adding another singleton in GeckoApp. I think we should use this as an opportunity to find a better home for this type of thing, since it's really not tied to the front-end code at all. My suggestion is to manage the lifetimes of these things in GeckoThread.

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> https://reviewboard.mozilla.org/r/51331/#review48843
> 
> ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1549
> (Diff revision 1)
> >  
> >          mContactService = new ContactService(EventDispatcher.getInstance(), this);
> >  
> >          mPromptService = new PromptService(this);
> >  
> > +        mSpeechSynthesisService = new SpeechSynthesisService();
> 
> Yeah, I'm not thrilled about adding another singleton in GeckoApp. I think
> we should use this as an opportunity to find a better home for this type of
> thing, since it's really not tied to the front-end code at all. My
> suggestion is to manage the lifetimes of these things in GeckoThread.

We will need a singleton in any case for the actual speech service component. I guess maybe it could be a JNI class as well and embody both the Android Java API and the native xpcomy stuff?

To recap, it would be a service component that starts with the "speech-synth-started" category like any other platform. It would also be a JNI class that would call into java methods for the Android speech operations.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8750257 [details]
Bug 1184142 - Support SpeechSynthesis API in Android.

Sounds like this isn't the right approach.

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8750257 [details]
Bug 1184142 - Support SpeechSynthesis API in Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51331/diff/1-2/

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8750257 [details]
Bug 1184142 - Support SpeechSynthesis API in Android.

https://reviewboard.mozilla.org/r/51331/#review65100

Looks ok to me, but would be good to have Jim Chen look as well.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

https://reviewboard.mozilla.org/r/51331/#review66756

::: mobile/android/base/java/org/mozilla/gecko/GeckoThread.java:126
(Diff revision 2)
>  
>      private final String mArgs;
>      private final String mAction;
>      private final boolean mDebugging;
>  
> +    private SpeechSynthesisService mSpeechSynthesisService;

This should not be in GeckoThread. You should start/stop the service from elsewhere (maybe in GeckoApplication).

Comment 14

[Makoto Kato [:m_kato]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #13)
> https://reviewboard.mozilla.org/r/51331/#review66756
> 
> ::: mobile/android/base/java/org/mozilla/gecko/GeckoThread.java:126
> (Diff revision 2)
> >  
> >      private final String mArgs;
> >      private final String mAction;
> >      private final boolean mDebugging;
> >  
> > +    private SpeechSynthesisService mSpeechSynthesisService;
> 
> This should not be in GeckoThread. You should start/stop the service from
> elsewhere (maybe in GeckoApplication).

snorp suggests that I should use on GeckoThread instead by comment #8.  jchen and snorp, which class should I use this singleton??

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8750257 [details]
Bug 1184142 - Support SpeechSynthesis API in Android.

https://reviewboard.mozilla.org/r/51331/#review67222

GeckoThread is part of GeckoView so Fennec components shouldn't be used there. I suggest GeckoApplication because it's not tied to a particular activity like GeckoApp is, so you can just initialize it once in `GeckoApplication.onDelayedStartup()`.

::: mobile/android/base/java/org/mozilla/gecko/SpeechSynthesisService.java:68
(Diff revision 2)
> +    private void registerVoices() {
> +        if (mTTS != null) {
> +            return;
> +        }
> +
> +        mTTS = new TextToSpeech(GeckoAppShell.getContext(), new TextToSpeech.OnInitListener() {

Also, use `GeckoAppShell.getApplicationContext()` here instead of `getContext()`

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

I guess I didn't think this speech synthesis stuff was a Fennec feature -- more like platform support, but I'll defer to jchen's wisdom here.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

m_kato are you still planning to finish this feature?

Comment 18

[Eitan Isaacson [:eeejay]]

I tried this patch. A few issues with how it lists available voices:

* The voice.lang field should be a bcp47 tag. Instead it is a localized language name. Easy fix.

* It is a very long list of voices with non-human readable names like "de-de-x-nfh#female_2-local". On my phone I get over 200 voices like these. In comparison[1], I get 30 voices in Chrome which are better named. I think it may be worth doing registerVoicesByLocale() no matter what the version of Android is. In that way we can offer 1 voice for any supported language and let the platform pick which specific voice to use. This will result in a shorter list of voices similar to Chrome. We can use the Locale API to create a human readable localized name.

1. https://docs.google.com/spreadsheets/d/1vPIA5o7fFJ8OrN0xkmGx09HqXhDvoiQ9Cff22wvBG9Q/pubhtml#

Comment 19

[Makoto Kato [:m_kato]]

I will handle this into 53 train...

Comment 20

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1184142 - Android speech patch update.

Review commit: https://reviewboard.mozilla.org/r/94906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94906/

Comment 21

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1184142 - Support SpeechSynthesis API in Android.

Review commit: https://reviewboard.mozilla.org/r/94908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94908/

Comment 22

[Eitan Isaacson [:eeejay]]

Comment on attachment 8813489 [details]
Bug 1184142 - Android speech patch update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94906/diff/1-2/

Comment 23

[Eitan Isaacson [:eeejay]]

This is confusing as hell. I'm sorry about that.

Submitted two patches, one is a rebased version of m_kato's patch. The other are the nits addressed from above, and changes to how we list voices to be more like Chrome (but better :)

I will be taking leave very soon, and I don't know if I will be able to see this through. Not taking the bug. Feel free to incorporate my changes and rebase into your own.

Comment 24

[Eitan Isaacson [:eeejay]]

Comment on attachment 8813490 [details]
Bug 1184142 - Support SpeechSynthesis API in Android.

https://reviewboard.mozilla.org/r/94908/#review95104

Comment 25

[Eitan Isaacson [:eeejay]]

Attachment: Support WebSpeech in GeckoView. r?snorp r?smaug

I put this together in the last couple of days. It brings us into parity with Chrome's TTS, and adds word boundary events when available.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8958627 [details] [diff] [review]
Support WebSpeech in GeckoView. r?snorp r?smaug


>+#include "mozilla/ModuleUtils.h"
>+#include "nsIClassInfoImpl.h"
Is there any reason for nsIClassInfoImpl.h?
>+SpeechSynthesisService::Setup()
>+{
>+  ALOG("SpeechSynthesisService::Init");
Clearly this isn't ::Init method.

>+
>+  if (!Preferences::GetBool("media.webspeech.synth.enabled") ||
>+      Preferences::GetBool("media.webspeech.synth.test")) {
>+    return;
>+  }
>+
>+  if (!jni::IsAvailable()) {
>+    NS_WARNING("Failed to initialize speech synthesis");
>+    return;
>+  }
>+
>+  Init();
>+  java::SpeechSynthesisService::GetVoices();
Is this synchronous? Does it block the main thread badly?

>+SpeechSynthesisService::Observe(nsISupports* aSubject,
>+                                const char* aTopic,
>+                                const char16_t* aData)
Why all these services (I mean also non-Android services) have empty Observe? Does something require the service to be nsIObserver?

>+SpeechSynthesisService::DispatchEnd(jni::String::Param aUtteranceId,
>+                                    int64_t aElapsedTime,
Since elapsed time is later converted to some different unit, better to document what elapsedTime is here and
elsewhere. Is it ms? μs?

>+  static SpeechSynthesisService* GetInstance(bool create = true);
argument names should be in form aName

>+  static already_AddRefed<SpeechSynthesisService> GetInstanceForService();
>+
>+  static StaticRefPtr<SpeechSynthesisService> sSingleton;
>+
>+private:
>+  virtual ~SpeechSynthesisService(){};
>+
>+  nsInterfaceHashtable<nsCStringHashKey, nsISpeechTask> mTasks;
So entries are added to mTasks hashtable, but never removed

Comment 27

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8958627 [details] [diff] [review]
> Support WebSpeech in GeckoView. r?snorp r?smaug
> 
> 
> >+#include "mozilla/ModuleUtils.h"
> >+#include "nsIClassInfoImpl.h"
> Is there any reason for nsIClassInfoImpl.h?

No, fixed.

> >+SpeechSynthesisService::Setup()
> >+{
> >+  ALOG("SpeechSynthesisService::Init");
> Clearly this isn't ::Init method.
> 

Fixed.

> >+
> >+  if (!Preferences::GetBool("media.webspeech.synth.enabled") ||
> >+      Preferences::GetBool("media.webspeech.synth.test")) {
> >+    return;
> >+  }
> >+
> >+  if (!jni::IsAvailable()) {
> >+    NS_WARNING("Failed to initialize speech synthesis");
> >+    return;
> >+  }
> >+
> >+  Init();
> >+  java::SpeechSynthesisService::GetVoices();
> Is this synchronous? Does it block the main thread badly?
> 

It creates a TextToSpeech object in java that is initialized asynchronously, and calls a callback when done on the UI thread (not the main gecko thread). I don't know if getting the voices on the UI thread creates and jank, so I changed that part to be on the background thread.

> >+SpeechSynthesisService::Observe(nsISupports* aSubject,
> >+                                const char* aTopic,
> >+                                const char16_t* aData)
> Why all these services (I mean also non-Android services) have empty
> Observe? Does something require the service to be nsIObserver?
> 

Good question! I think we used to use it, before I introduced the "speech-synth-started" category a while ago.

I recall that for some reason modules needed to implement an observer even for that, but I just removed it and it work..

Might need to go back and do that in other platforms as well.

> >+SpeechSynthesisService::DispatchEnd(jni::String::Param aUtteranceId,
> >+                                    int64_t aElapsedTime,
> Since elapsed time is later converted to some different unit, better to
> document what elapsedTime is here and
> elsewhere. Is it ms? μs?

ms, i'll add it to the name.

> 
> >+  static SpeechSynthesisService* GetInstance(bool create = true);
> argument names should be in form aName

Done.

> 
> >+  static already_AddRefed<SpeechSynthesisService> GetInstanceForService();
> >+
> >+  static StaticRefPtr<SpeechSynthesisService> sSingleton;
> >+
> >+private:
> >+  virtual ~SpeechSynthesisService(){};
> >+
> >+  nsInterfaceHashtable<nsCStringHashKey, nsISpeechTask> mTasks;
> So entries are added to mTasks hashtable, but never removed

Oy, that's embarrassing.

Comment 28

[Eitan Isaacson [:eeejay]]

Attachment: Support WebSpeech in GeckoView. r?snorp r?smaug

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8959268 [details] [diff] [review]
Support WebSpeech in GeckoView. r?snorp r?smaug

Not really reviewing the java side.

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8959268 [details] [diff] [review]
Support WebSpeech in GeckoView. r?snorp r?smaug

Review of attachment 8959268 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webspeech/synth/android/SpeechSynthesisService.h
@@ +58,5 @@
> +
> +private:
> +  virtual ~SpeechSynthesisService(){};
> +
> +  nsInterfaceHashtable<nsCStringHashKey, nsISpeechTask> mTasks;

This hash seems to indicate that we can have multiple utterances occurring. If that's not that case then just make this a single nsISpeech and nsCString and assert that id in dispatch* matches?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/SpeechSynthesisService.java
@@ +28,5 @@
> +    private static TextToSpeech sTTS;
> +    private static String sCurrentUtteranceId;
> +
> +    @WrapForJNI(calledFrom = "gecko")
> +    public static void getVoices() {

This is called getVoices() but does not return anything. Maybe initVoices() or just init()?

@@ +103,5 @@
> +        sCurrentUtteranceId = utteranceId;
> +        return utteranceId;
> +    }
> +
> +    private static void setUtteranceListener(final String text) {

The callbacks all have a specific utteranceId, but this text is from one specific speak() invocation. If speak() can be called again before the previous utterance is issued we'll be in trouble. Is this a possibility? It seems like we need more tracking of the text in this case.

Comment 31

[Eitan Isaacson [:eeejay]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #30)
> Comment on attachment 8959268 [details] [diff] [review]
> Support WebSpeech in GeckoView. r?snorp r?smaug
> 
> Review of attachment 8959268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webspeech/synth/android/SpeechSynthesisService.h
> @@ +58,5 @@
> > +
> > +private:
> > +  virtual ~SpeechSynthesisService(){};
> > +
> > +  nsInterfaceHashtable<nsCStringHashKey, nsISpeechTask> mTasks;
> 
> This hash seems to indicate that we can have multiple utterances occurring.
> If that's not that case then just make this a single nsISpeech and nsCString
> and assert that id in dispatch* matches?

More than one utterance is theoretically possible. Although we limit it to one on platforms that have global queues like Android. I would prefer to keep this map here because we may decide in the future to queue the utterances in the platform service and not in the browser. The work I did for utterance queue management is in bug 1188099.

> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/
> SpeechSynthesisService.java
> @@ +28,5 @@
> > +    private static TextToSpeech sTTS;
> > +    private static String sCurrentUtteranceId;
> > +
> > +    @WrapForJNI(calledFrom = "gecko")
> > +    public static void getVoices() {
> 
> This is called getVoices() but does not return anything. Maybe initVoices()
> or just init()?

Sure

> 
> @@ +103,5 @@
> > +        sCurrentUtteranceId = utteranceId;
> > +        return utteranceId;
> > +    }
> > +
> > +    private static void setUtteranceListener(final String text) {
> 
> The callbacks all have a specific utteranceId, but this text is from one
> specific speak() invocation. If speak() can be called again before the
> previous utterance is issued we'll be in trouble. Is this a possibility? It
> seems like we need more tracking of the text in this case.

aah. I was treating it like a closure thinking that a new listener needs to be set for each utterance. I read the API docs as the listener is for a "given utterance", when really, the callbacks are called with the argument being a "given utterance" id.

I'm going to implement a hash map here not that different than the native one with the tasks. The platform can't have concurrent utterances, but I want to keep the behavior of the service "correct" in the sense that there may be multiple pending utterances.

Comment 32

[Eitan Isaacson [:eeejay]]

On second thought, I'm going to keep this initial version simple, get rid of maps, and assert on multiple utterances.

Comment 33

[Eitan Isaacson [:eeejay]]

Attachment: Support WebSpeech in GeckoView. r?snorp r?smaug

Sorry for pinging smaug again for a review. I simplified this a bit, and made it only accept one speak() at a time. I also moved the entire state to the native code, because that is where the speech task lives.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8959395 [details] [diff] [review]
Support WebSpeech in GeckoView. r?snorp r?smaug


+class SpeechSynthesisService final
+  : public nsISpeechService
+  , public java::SpeechSynthesisService::Natives<SpeechSynthesisService>
+{
+public:
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSISPEECHSERVICE
+
+  SpeechSynthesisService(){};
+
+  void Setup();
+
+  static void DoneRegisteringVoices();
+
+  static void RegisterVoice(jni::String::Param aUri,
+                            jni::String::Param aName,
+                            jni::String::Param aLocale,
+                            bool aIsNetwork,
+                            bool aIsDefault);
+
+  static void DispatchStart(jni::String::Param aUtteranceId);
+
+  static void DispatchEnd(jni::String::Param aUtteranceId);
+
+  static void DispatchError(jni::String::Param aUtteranceId);
+
+  static void DispatchBoundary(jni::String::Param aUtteranceId,
+                               int32_t aStart,
+                               int32_t aEnd);
+
+  static SpeechSynthesisService* GetInstance(bool aCreate = true);
+  static already_AddRefed<SpeechSynthesisService> GetInstanceForService();
+
+  static StaticRefPtr<SpeechSynthesisService> sSingleton;
+
+private:
+  virtual ~SpeechSynthesisService(){};
+
+  nsCOMPtr<nsISpeechTask> mTask;
+
+  nsCString mUtteranceId;
+
+  nsString mText;
You only ever assign value to mText but never use it. drop this.

+
+  TimeStamp mStartTime;
mStartTime is a tad odd member variable in the service, when it really is about task.
At least change the variable name and add a comment that it is about the start time of mTask

+
+  uint32_t mCharOffset;
This needs a comment too

+};

>+
>+  auto utteranceId =
>+    java::SpeechSynthesisService::Speak(aUri, aText, aRate, aPitch, aVolume);
Please don't use auto in context where it isn't clear what the type is

>+SpeechSynthesisService::DispatchStart(jni::String::Param aUtteranceId)
>+{
>+  if (sSingleton) {
>+    MOZ_ASSERT(sSingleton->mUtteranceId.Equals(aUtteranceId->ToCString()));
>+    nsCOMPtr<nsISpeechTask> task = sSingleton->mTask;
>+    if (task) {
>+      sSingleton->mStartTime = TimeStamp::Now();
>+      DebugOnly<nsresult> rv = task->DispatchStart();
>+      NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Unable to dispatch start");
if DispatchStart fails, we leak, since task keeps callback alive, and callback keeps this service alive

Comment 35

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8959395 [details] [diff] [review]
Support WebSpeech in GeckoView. r?snorp r?smaug

Review of attachment 8959395 [details] [diff] [review]:
-----------------------------------------------------------------

The java bits look ok to me modulo the one comment which should be a quick fix

::: dom/media/webspeech/synth/android/SpeechSynthesisService.cpp
@@ +77,5 @@
> +
> +// nsISpeechService
> +
> +NS_IMETHODIMP
> +SpeechSynthesisService::Speak(const nsAString& aText,

I think we need to check if the prior init worked, or is still in progress

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/SpeechSynthesisService.java
@@ +37,5 @@
> +
> +        sTTS = new TextToSpeech(ctx, new TextToSpeech.OnInitListener() {
> +            @Override
> +            public void onInit(int status) {
> +                if (status != TextToSpeech.SUCCESS) {

Seems like we should notify SpeechSynthesisService if this fails? Maybe doneRegisteringVoices() should be initResult(boolean)? Or I guess maybe we just need to call doneRegisteringVoices() here with no voices registered.

If there are no registered voices, will SpeechSynthesisService never be asked to utter?

Comment 36

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 8959395 [details] [diff] [review]
> Support WebSpeech in GeckoView. r?snorp r?smaug
> 
> 
> +class SpeechSynthesisService final
> +  : public nsISpeechService
> +  , public java::SpeechSynthesisService::Natives<SpeechSynthesisService>
> +{
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSISPEECHSERVICE
> +
> +  SpeechSynthesisService(){};
> +
> +  void Setup();
> +
> +  static void DoneRegisteringVoices();
> +
> +  static void RegisterVoice(jni::String::Param aUri,
> +                            jni::String::Param aName,
> +                            jni::String::Param aLocale,
> +                            bool aIsNetwork,
> +                            bool aIsDefault);
> +
> +  static void DispatchStart(jni::String::Param aUtteranceId);
> +
> +  static void DispatchEnd(jni::String::Param aUtteranceId);
> +
> +  static void DispatchError(jni::String::Param aUtteranceId);
> +
> +  static void DispatchBoundary(jni::String::Param aUtteranceId,
> +                               int32_t aStart,
> +                               int32_t aEnd);
> +
> +  static SpeechSynthesisService* GetInstance(bool aCreate = true);
> +  static already_AddRefed<SpeechSynthesisService> GetInstanceForService();
> +
> +  static StaticRefPtr<SpeechSynthesisService> sSingleton;
> +
> +private:
> +  virtual ~SpeechSynthesisService(){};
> +
> +  nsCOMPtr<nsISpeechTask> mTask;
> +
> +  nsCString mUtteranceId;
> +
> +  nsString mText;
> You only ever assign value to mText but never use it. drop this.
> 

Do'h! I was going to use the length for the end event. I'll store that instead. Rename, add comment, etc.

> >+SpeechSynthesisService::DispatchStart(jni::String::Param aUtteranceId)
> >+{
> >+  if (sSingleton) {
> >+    MOZ_ASSERT(sSingleton->mUtteranceId.Equals(aUtteranceId->ToCString()));
> >+    nsCOMPtr<nsISpeechTask> task = sSingleton->mTask;
> >+    if (task) {
> >+      sSingleton->mStartTime = TimeStamp::Now();
> >+      DebugOnly<nsresult> rv = task->DispatchStart();
> >+      NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Unable to dispatch start");
> if DispatchStart fails, we leak, since task keeps callback alive, and
> callback keeps this service alive

Are you assuming that the other dispatch methods (end/error) won't be called after we fail? They should still be called. An error in the task would indicate possibly that the underlying window went away, the system should still followup with DispatchEnd/DispatchError and release the task.

Comment 37

[Eitan Isaacson [:eeejay]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #35)
> Comment on attachment 8959395 [details] [diff] [review]
> Support WebSpeech in GeckoView. r?snorp r?smaug
> 
> Review of attachment 8959395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The java bits look ok to me modulo the one comment which should be a quick
> fix
> 
> ::: dom/media/webspeech/synth/android/SpeechSynthesisService.cpp
> @@ +77,5 @@
> > +
> > +// nsISpeechService
> > +
> > +NS_IMETHODIMP
> > +SpeechSynthesisService::Speak(const nsAString& aText,
> 
> I think we need to check if the prior init worked, or is still in progress
> 

As I mention below, the only way Speak() would be called on this service is if it registered a voice. If the init fails, we never add voices to the registry, so it would not be called.

> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/
> SpeechSynthesisService.java
> @@ +37,5 @@
> > +
> > +        sTTS = new TextToSpeech(ctx, new TextToSpeech.OnInitListener() {
> > +            @Override
> > +            public void onInit(int status) {
> > +                if (status != TextToSpeech.SUCCESS) {
> 
> Seems like we should notify SpeechSynthesisService if this fails? Maybe
> doneRegisteringVoices() should be initResult(boolean)? Or I guess maybe we
> just need to call doneRegisteringVoices() here with no voices registered.
> 

doneRegisteringVoices() sends a DOM event to content telling it that the available voices have changed. If there are no additional voices, it shouldn't be fired.

> If there are no registered voices, will SpeechSynthesisService never be
> asked to utter?

Right. Gecko stores a list of voices with associated services. If there are no voices, this service will not be used. We can delete the service instance, but it is singleton, and I don't think it is worth the bother.

Comment 38

[Eitan Isaacson [:eeejay]]

Attachment: Support WebSpeech in GeckoView. r=snorp r?smaug

Addressed comments. With the exception of the memory leak issue smaug mentioned above. I hope I clarified why I don't think it's a leak. I'm concerned about dropping the task before dispatching end/error to it because that might actually trigger a real leak if it isn't terminated correctly.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8959634 [details] [diff] [review]
Support WebSpeech in GeckoView. r=snorp r?smaug

Could you drop mService from AndroidSpeechCallback. It isn't used ever

Comment 40

[Eitan Isaacson [:eeejay]]

Attachment: Support WebSpeech in GeckoView. r=snorp r?smaug

Removed service reference in callback.

Regarding what we discussed on IRC with the nsIObserver interface, since this is a special, one-time, category, I can remove the supplied topic sent to NS_CreateServicesFromCategory. I'll post a followup.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00ca51f4fafc
Support WebSpeech in GeckoView. r=snorp r=smaug

Comment 43

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/00ca51f4fafc

Comment 44

[Eric Shepherd [:sheppy]]

Can I please get clarification: is this going to ship in 61 or 62? The bug here says 61 but the Intent to Ship email said 62.

Comment 45

[Florian Scholz (Open Web Docs)]

Updated compat data on these reference pages to say behind a pref in Firefox 61 and enabled by default in 62 
(https://github.com/mdn/browser-compat-data/pull/2144):

https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/cancel
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/getVoices
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/onvoiceschanged
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/pause
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/paused
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/pending
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/resume
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/speak
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesis/speaking

https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisErrorEvent
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisErrorEvent/error

https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisEvent
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisEvent/charIndex
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisEvent/elapsedTime
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisEvent/name
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisEvent/utterance

https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/SpeechSynthesisUtterance
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/lang
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/onboundary
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/onend
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/onerror
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/onmark
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/onpause
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/onresume
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/onstart
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/pitch
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/rate
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/text
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/voice
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisUtterance/volume

https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisVoice
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisVoice/default
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisVoice/lang
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisVoice/localService
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisVoice/name
https://developer.mozilla.org/en-US/docs/Web/API/SpeechSynthesisVoice/voiceURI

It's also announced on Firefox 62 for developers:
https://developer.mozilla.org/en-US/Firefox/Releases/62#APIs