Open Bug 1350910 Opened 3 years ago Updated 2 years ago

WebSpeech labeling


(Core :: Web Speech, defect)

50 Branch
Not set




(Reporter: smaug, Assigned: eeejay)




(1 file, 1 obsolete file)

At least in recognition side most of the labeling should be easy, since dispatching tend to happen in main thread. There is also some none-main-thread case where one may need to be careful to not touch main thread objects unsafely.
eeejay, do you think you could take a look?
Shouldn't take much time.

See bug 1321812
eeejay is on parental leave ATM, so please expect delays in response. :-)
Flags: needinfo?(eitan)
I think most of this has already been done (perhaps in a scripted fashion?) in bug 1372405. There are a few outlying ones that I can followup with.

Also, recognition is preffed off, and I don't think there is a plan to enable that soon.
Flags: needinfo?(eitan)
I think this was mostly taken care of, just cleaned it up a bit, and labeled some runnables that were missed.
Attachment #8881909 - Flags: review?(bugs)
Comment on attachment 8881909 [details] [diff] [review]
Clean up the runnable labeling in webspeech. r?smaug

Sorry if this bug wasn't clear enough.
Labeling means also dispatching to the right nsIEventTarget, not just to main thread.
So I assume need to dispatch using something like
Attachment #8881909 - Flags: review?(bugs) → review-
Assignee: nobody → eitan
Still need to verify this works on OSX, but at least it builds..

I assume the category should be "other", and because this is top-level
stuff that is not associated with a document or tab it should be
Attachment #8886241 - Flags: review?(bugs)
Attachment #8881909 - Attachment is obsolete: true
Comment on attachment 8886241 [details] [diff] [review]
Label more runnables and dispatch them to correct group/category. r?smaug

>   RefPtr<RegisterVoicesRunnable> runnable = new RegisterVoicesRunnable(mSpeechService, list);
>-  NS_DispatchToMainThread(runnable, NS_DISPATCH_SYNC);
>+  SystemGroup::EventTargetFor(TaskCategory::Other)->Dispatch(runnable, NS_DISPATCH_SYNC);
This is parent process only, right? In that case SystemGroup is fine

>   SpeechDispatcherService* service = SpeechDispatcherService::GetInstance(false);
>   if (service) {
>-    NS_DispatchToMainThread(NewRunnableMethod<uint32_t, SPDNotificationType>(
>-      "dom::SpeechDispatcherService::EventNotify",
>-      service,
>-      &SpeechDispatcherService::EventNotify,
>-      static_cast<uint32_t>(msg_id),
>-      state));
>+    SystemGroup::EventTargetFor(TaskCategory::Other)->Dispatch(
>+      NewRunnableMethod<uint32_t, SPDNotificationType>(
>+        "dom::SpeechDispatcherService::EventNotify",
>+        service,
>+        &SpeechDispatcherService::EventNotify,
>+        static_cast<uint32_t>(msg_id),
>+        state));
same here

>@@ -418,18 +420,17 @@ SpeechDispatcherService::Setup()
>       uri.Append(NS_ConvertUTF8toUTF16(lang));
>       mVoices.Put(uri, new SpeechDispatcherVoice(
>                     NS_ConvertUTF8toUTF16(list[i]->name),
>                     NS_ConvertUTF8toUTF16(lang)));
>     }
>   }
>-  NS_DispatchToMainThread(
>+  SystemGroup::EventTargetFor(TaskCategory::Other)->Dispatch(
>     NewRunnableMethod("dom::SpeechDispatcherService::RegisterVoices",
>                       this,
>                       &SpeechDispatcherService::RegisterVoices));
This doesn't look right. On child process we end up dispatching "voiceschanged" event synchronously from
RegisterVoices call, right? And that even goes to the web page. So it shouldn't use SystemGroup but dispatched using the window or document of the page.

>@@ -528,23 +529,25 @@ SpeechDispatcherService::Speak(const nsAString& aText, const nsAString& aUri,
>       return NS_ERROR_FAILURE;
>     }
>     mCallbacks.Put(msg_id, callback);
>   } else {
>     // Speech dispatcher does not work well with empty strings.
>     // In that case, don't send empty string to speechd,
>     // and just emulate a speechd start and end event.
>-    NS_DispatchToMainThread(NewRunnableMethod<SPDNotificationType>(
>+    nsCOMPtr<nsIEventTarget> target = SystemGroup::EventTargetFor(TaskCategory::Other);
>+    target->Dispatch(NewRunnableMethod<SPDNotificationType>(
>       "dom::SpeechDispatcherCallback::OnSpeechEvent",
>       callback,
>       &SpeechDispatcherCallback::OnSpeechEvent,
>       SPD_EVENT_BEGIN));
>-    NS_DispatchToMainThread(NewRunnableMethod<SPDNotificationType>(
>+    target->Dispatch(NewRunnableMethod<SPDNotificationType>(
>       "dom::SpeechDispatcherCallback::OnSpeechEvent",
>       callback,
>       &SpeechDispatcherCallback::OnSpeechEvent,
>       SPD_EVENT_END));
>   }
Why are these right? Somehow we should end up using event target of the page, and not system group when dispatching "start" or so.
Attachment #8886241 - Flags: review?(bugs) → review-
You need to log in before you can comment on or make changes to this bug.