Currently, external synth services are hit on Firefox startup. We should only touch external services when speech synthesis is required. Doing otherwise can potentially cause processes to spawn that are not required by the user, and generally slow down startup. Ideally, SpeechSynthesis.getVoices() would be async, and we would lazily initialize the voice registry and query for voices when it is first called. In reality, getVoices() is sync, and an errata was added[1] to the spec that gives SpeechSynthesis a "voiceschanged" event that is dispatched when voices are added to the internal registry. Chrome already implements this. We need to change our internal speech service API to allow lazy voice loading. An ideal API change would: - Support the "voiceschanged" event. - Be future-proof and support a potential async getVoices() method. 1. https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi-errata.html (see E11)

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

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

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

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

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

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

Asked Margret for review since Gijs is away. The changes should be easy enough to understand without too much catching up on the narrate feature.

Comment on attachment 8735947 [details] MozReview Request: Bug 1254378 - Make SpeechSynthesis an event target with a "voiceschanged" event. r=smaug https://reviewboard.mozilla.org/r/43003/#review39655 Those fixed, r+ ::: dom/media/webspeech/synth/SpeechSynthesis.cpp:82 (Diff revision 1) > SpeechSynthesis::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) > { > return SpeechSynthesisBinding::Wrap(aCx, this, aGivenProto); > } > > nsPIDOMWindowInner* I would just remove GetParentObject from SpeechSynthesis since DOMEventTargetHelper has it already. Though, perhaps you want to pass the inner window to some other object? If so, that code could use GetOwner(); ::: dom/media/webspeech/synth/SpeechSynthesis.cpp:157 (Diff revision 1) > } > > RefPtr<SpeechSynthesisUtterance> utterance = mSpeechQueue.ElementAt(0); > > nsAutoString docLang; > - nsIDocument* doc = mParent->GetExtantDoc(); > + nsIDocument* doc = GetOwner()->GetExtantDoc(); Get* methods may return null. This needs a null pointer check. (GetOwner() starts to return null once a window is closed, yet JS may still have references to it)

Comment on attachment 8735948 [details] MozReview Request: Bug 1254378 - Make new 'speech-synth-started' component service category. r=smaug https://reviewboard.mozilla.org/r/43005/#review39659 ::: dom/media/webspeech/synth/nsSynthVoiceRegistry.cpp:195 (Diff revision 1) > if (!gSynthVoiceRegistry) { > gSynthVoiceRegistry = new nsSynthVoiceRegistry(); > Preferences::AddBoolVarCache(&sForceGlobalQueue, > "media.webspeech.synth.force_global_queue"); > + if (XRE_IsParentProcess()) { > + // Start up all speech synth services. "all ... services"? Isn't there just one (though different implementations in different OSes)

Comment on attachment 8735949 [details] MozReview Request: Bug 1254378 - Implement nsISynthVoiceRegistry.notifyVoicesChanged. r=smaug https://reviewboard.mozilla.org/r/43007/#review39663 That fixed, r+ ::: dom/media/webspeech/synth/speechd/SpeechDispatcherService.cpp:425 (Diff revision 1) > // private methods > > void > SpeechDispatcherService::RegisterVoices() > { > nsSynthVoiceRegistry* registry = nsSynthVoiceRegistry::GetInstance(); This looks a bit scary, at least in theory. Raw pointer to registry and then calling a method on it which may call JS. Make registry a strong pointer.

Comment on attachment 8735950 [details] MozReview Request: Bug 1254378 - Dispatch 'voiceschanged' when notified. r=smaug https://reviewboard.mozilla.org/r/43009/#review39661 If that raw pointer -> strong pointer is done in that other patch, this should be fine. ::: dom/media/webspeech/synth/SpeechSynthesis.cpp:329 (Diff revision 1) > - obs->RemoveObserver(this, "inner-window-destroyed"); > + obs->RemoveObserver(this, "inner-window-destroyed"); > - } > + } > - } > + } > + } else if (strcmp(aTopic, "synth-voices-changed") == 0) { > + LOG(LogLevel::Debug, ("SpeechSynthesis::onvoiceschanged")); > + DispatchTrustedEvent(NS_LITERAL_STRING("voiceschanged")); Is it really safe to dispatch the event and call AdvanceQueue() synchronously here? Or should this all happen asynchronously?

Comment on attachment 8735951 [details] MozReview Request: Bug 1254378 - Update synth tests and introduce no voiceschanged test. r=smaug https://reviewboard.mozilla.org/r/43011/#review39665 All those explained and/or fixed, r+ ::: dom/media/webspeech/synth/moz.build:9 (Diff revision 1) > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > if CONFIG['MOZ_WEBSPEECH']: > MOCHITEST_MANIFESTS += [ > 'test/mochitest.ini', > + 'test/startup/mochitest.ini', Not sure why we're adding a new directory. I'd prefer just having test file name such that it is clear what it is testing. ::: dom/media/webspeech/synth/test/common.js:70 (Diff revision 1) > +function waitForVoices(win) { > + return new Promise(resolve => { > + function resolver() { > + if (win.speechSynthesis.getVoices().length) { > + resolve(); > + win.speechSynthesis.removeEventListener('voiceschanged', resolver); Nit, might be good to first remove event listener and then call resolve() ::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:321 (Diff revision 1) > } > > static void > AddVoices(nsISpeechService* aService, const VoiceDetails* aVoices, uint32_t aLength) > { > nsSynthVoiceRegistry* registry = nsSynthVoiceRegistry::GetInstance(); Please make registry a strong pointer. ::: dom/media/webspeech/synth/test/startup/file_voiceschanged.html:29 (Diff revision 1) > + SimpleTest.finish(); > +} > + > +speechSynthesis.addEventListener("voiceschanged", onVoicesChanged); > + > +is(speechSynthesis.getVoices().length, 0, "No voices added initially"); Please explain what guarantees this is the first time we initialize this stuff?

Comment on attachment 8735952 [details] MozReview Request: Bug 1254378 - Update Narrate to work with "voiceschanged" event. r=margaret https://reviewboard.mozilla.org/r/43013/#review39793 Without being familiar with this code, these changes look reasonable to me. ::: toolkit/components/narrate/VoiceSelect.jsm (Diff revision 1) > - > - for (let option of options) { > - this.add(option.label, option.value); > - } > - > - this.selectedIndex = 0; With this line removed, it looks like the `selectedIndex` getter/setter are now unused. ::: toolkit/components/narrate/VoiceSelect.jsm:50 (Diff revision 1) > option.setAttribute("role", "option"); > option.textContent = label; > this.listbox.appendChild(option); > }, > > + addOptions: function(options, selectedIndex = 0) { This method is never called with a `selectedIndex` parameter. Are there plans to use this in the future, or could you just remove this and just always select the first item?

https://reviewboard.mozilla.org/r/43005/#review39659 > "all ... services"? Isn't there just one (though different implementations in different OSes) There can theoretically be more than one. For example, addons.

https://reviewboard.mozilla.org/r/43009/#review39661 > Is it really safe to dispatch the event and call AdvanceQueue() synchronously here? Or should this all happen asynchronously? I made it a strong pointer in the other patch. So I am assuming this is fine now?

https://reviewboard.mozilla.org/r/43011/#review39665 > Not sure why we're adding a new directory. I'd prefer just having test file name such that it is clear what it is testing. A new directory gaurentees that this test is run on a fresh instance of the browser. Apparently the mochitest order is not determined by mochitest.ini, and I didn't want to play with filename ordering. > Please explain what guarantees this is the first time we initialize this stuff? This test should always run on a fresh browser (as explained above) before synth was initialized. This is the first synth-related function call, so it should return 0 voices and should also kickoff synth initialization.

(In reply to Eitan Isaacson [:eeejay] from comment #15) > I made it a strong pointer in the other patch. So I am assuming this is fine > now? Yeah, should be fine, if it is strong ptr.

(In reply to Eitan Isaacson [:eeejay] from comment #16) > A new directory gaurentees that this test is run on a fresh instance of the > browser. Oh, I didn't know having a new directory has that kind of side effect. Is that side effect documented somewhere. But anyhow, if that is the case, fine.

Comment on attachment 8735947 [details] MozReview Request: Bug 1254378 - Make SpeechSynthesis an event target with a "voiceschanged" event. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43003/diff/1-2/

Comment on attachment 8735948 [details] MozReview Request: Bug 1254378 - Make new 'speech-synth-started' component service category. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43005/diff/1-2/

Comment on attachment 8735949 [details] MozReview Request: Bug 1254378 - Implement nsISynthVoiceRegistry.notifyVoicesChanged. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43007/diff/1-2/

Comment on attachment 8735950 [details] MozReview Request: Bug 1254378 - Dispatch 'voiceschanged' when notified. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43009/diff/1-2/

Comment on attachment 8735951 [details] MozReview Request: Bug 1254378 - Update synth tests and introduce no voiceschanged test. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43011/diff/1-2/

Comment on attachment 8735952 [details] MozReview Request: Bug 1254378 - Update Narrate to work with "voiceschanged" event. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43013/diff/1-2/

Comment on attachment 8735952 [details] MozReview Request: Bug 1254378 - Update Narrate to work with "voiceschanged" event. r=margaret Margaret, I'm flagging you for review again because you uncovered something I forgot to complete: When voices change the value should be pre-set to the user's previous preference.

Comment on attachment 8735952 [details] MozReview Request: Bug 1254378 - Update Narrate to work with "voiceschanged" event. r=margaret https://reviewboard.mozilla.org/r/43013/#review40079

Comment on attachment 8735947 [details] MozReview Request: Bug 1254378 - Make SpeechSynthesis an event target with a "voiceschanged" event. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43003/diff/2-3/

Comment on attachment 8735948 [details] MozReview Request: Bug 1254378 - Make new 'speech-synth-started' component service category. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43005/diff/2-3/

Comment on attachment 8735949 [details] MozReview Request: Bug 1254378 - Implement nsISynthVoiceRegistry.notifyVoicesChanged. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43007/diff/2-3/

Comment on attachment 8735950 [details] MozReview Request: Bug 1254378 - Dispatch 'voiceschanged' when notified. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43009/diff/2-3/

Comment on attachment 8735951 [details] MozReview Request: Bug 1254378 - Update synth tests and introduce no voiceschanged test. r=smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43011/diff/2-3/

Comment on attachment 8735952 [details] MozReview Request: Bug 1254378 - Update Narrate to work with "voiceschanged" event. r=margaret Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43013/diff/2-3/

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8bd64aaadd https://hg.mozilla.org/integration/mozilla-inbound/rev/3ace54af7a05 https://hg.mozilla.org/integration/mozilla-inbound/rev/47390c18d9b1 https://hg.mozilla.org/integration/mozilla-inbound/rev/70ead871eea8 https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a5b2b77a77 https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c661c123c0

https://hg.mozilla.org/mozilla-central/rev/7f8bd64aaadd https://hg.mozilla.org/mozilla-central/rev/3ace54af7a05 https://hg.mozilla.org/mozilla-central/rev/47390c18d9b1 https://hg.mozilla.org/mozilla-central/rev/70ead871eea8 https://hg.mozilla.org/mozilla-central/rev/f7a5b2b77a77 https://hg.mozilla.org/mozilla-central/rev/f2c661c123c0

yay,this has a perf win for main_rss memory measured.