The current setup with speech services in mochitest JS is inflexible. Testing with e10s is impossible, and the ipc tests are awkward and not stable. If we have a fake speech service we could have plain content mochitests with no special powers that should run on any platform and setup.

Also, remove ipc tests since we now can enable these tests in e10s.

Made this a seperate patch so the actual changes in the tests in the previous patch are easier to read.

Added SPeechSynthesis* to test_interfaces.html

Comment on attachment 8603425 [details] [diff] [review] [1/2] Replace mochitest test synth services with global services to simplify tests. >-NS_IMPL_CYCLE_COLLECTION(nsSpeechTask, mSpeechSynthesis, mUtterance); >+NS_IMPL_CYCLE_COLLECTION(nsSpeechTask, mSpeechSynthesis, mUtterance, mCallback); Oh, the meaning of callback changes a bit. It isn't anymore just something implemented by a service. Please fix the .idl to not say that nsISpeechTaskCallback is something implemented by a service. >+++ b/dom/media/webspeech/synth/test/FakeSynthModule.cpp >@@ -0,0 +1,58 @@ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#include "mozilla/ModuleUtils.h" >+#include "nsIClassInfoImpl.h" >+ >+#ifdef MOZ_WEBRTC This ifdef sure needs a comment. >+ >+#include "nsFakeSynthServices.h" Why not include all the .h files here after ifdef? >+FakeDirectAudioSynth::Speak(const nsAString& aText, const nsAString& aUri, >+ float aVolume, float aRate, float aPitch, >+ nsISpeechTask* aTask) >+{ >+ class Runnable final : public nsRunnable >+ { >+ public: >+ Runnable(nsISpeechTask* aTask, const nsAString& aText) : >+ mTask(aTask), mText(aText) { >+ } >+ >+ NS_IMETHOD Run() override >+ { >+ NS_ENSURE_TRUE(mTask, NS_ERROR_FAILURE); >+ >+ nsRefPtr<FakeSynthCallback> cb = new FakeSynthCallback(nullptr); >+ mTask->Setup(cb, CHANNELS, SAMPLERATE, 2); >+ >+ uint32_t frames_length = (SAMPLERATE / 40) * mText.Length(); / 40 sure needs some comment >+ int16_t* frames = new int16_t[frames_length](); >+ mTask->SendAudioNative(frames, frames_length); >+ free(frames); Please use nsAutoArrayPtr or UniquePtr. No manual free. >+ void Revoke() >+ { >+ mTask = nullptr; >+ } You define Revoke for many runnables, but that isn't used ever. Also you then null check mTask in those runnables, but is it actually ever possible that mTask is null? >+nsFakeSynthServices::Observe(nsISupports* aSubject, const char* aTopic, >+ const char16_t* aData) >+{ >+ MOZ_ASSERT(NS_IsMainThread()); >+ NS_ENSURE_TRUE(!strcmp(aTopic, "profile-after-change"), NS_ERROR_UNEXPECTED); >+ /* >+ if (!Preferences::GetBool("media.webspeech.synth.test")) { >+ return NS_OK; >+ } >+ */ So the commented out code shouldn't be needed. We shouldn't even initialize the service without the pref, right?

or if the fakeservice is always instantiated, then remove /* and */

(In reply to Olli Pettay [:smaug] from comment #4) > >+ void Revoke() > >+ { > >+ mTask = nullptr; > >+ } > You define Revoke for many runnables, but that isn't used ever. > Also you then null check mTask in those runnables, but is it actually ever > possible that mTask is null? > I'm not familiar enough with the runnable api. I was assuming that it can be removed from the queue somewhere else, and revoke() would be called. Then in Run() we check to see if mTask is null, and don't perform the action if it is. I got the idea from here: https://dxr.mozilla.org/mozilla-central/source/accessible/generic/Accessible.cpp#1726 Am I perpetuating a bad idea?

Revoke is for the case when you keep nsRevocableEventPtr<> reference to the runnable. http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#879

Comment on attachment 8603524 [details] [diff] [review] [2/2] Enable synth in mochitest profile and flatten synth tests. I think we don't want the change to test_interfaces.html. We aren't exposing the interfaces on release builds yet, right? At least not on desktop. So media.webspeech.synth.enabled should be set in each webspeech test, basically what Bug 1159768 tries to do to web components tests.

Also, remove ipc tests since we now can enable these tests in e10s. Also, make utterances very long in cancel test so that we actually interrupt them. Sorry for the delay, worked to make try happy with this and the dependent bugs.

Comment on attachment 8604903 [details] [diff] [review] Replace mochitest test synth services with global services to simplify tests. >+enum VoiceFlags >+{ >+ eSupressEvents = 1, >+ eSupressEnd = 2 Isn't it suppress, not supress? >+ Runnable(nsISpeechTask* aTask, const nsAString& aText) : >+ mTask(aTask), mText(aText) { { to its own line >+ explicit DispatchStart(nsISpeechTask* aTask) : >+ mTask(aTask) { ditto >+ DispatchEnd(nsISpeechTask* aTask, const nsAString& aText) : >+ mTask(aTask), mText(aText) { ditto But I don't still understand the profile-after-change setup here. Is nsFakeSynthServices::Observe( ever called? And if so, why? See the order here http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#854 Or do we rely on http://mxr.mozilla.org/mozilla-central/source/profile/dirserviceprovider/nsProfileDirServiceProvider.cpp#103 there? Could you explain?

Oh, I see http://mxr.mozilla.org/mozilla-central/source/xpcom/components/nsCategoryManager.cpp#875 Weird setup/API.

(In reply to Olli Pettay [:smaug] from comment #11) > Oh, I see > http://mxr.mozilla.org/mozilla-central/source/xpcom/components/ > nsCategoryManager.cpp#875 > Weird setup/API. Yeah, it is not great.

Comment on attachment 8604903 [details] [diff] [review] Replace mochitest test synth services with global services to simplify tests. >+class FakeSynthCallback : public nsISpeechTaskCallback [...] >+ NS_IMETHOD OnPause() >+ { [...] >+ NS_IMETHOD OnResume() >+ { [...] >+ NS_IMETHOD OnCancel() >+ { [...] These were missing "override" annotations, which causes clang 3.6 & newer to spam a "-Winconsistent-missing-override" build warning, which busts --enable-warnings-as-errors builds (with clang 3.6+). Once the tree reopens, I'll push a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2.