1350910 - WebSpeech labeling

Status: RESOLVED WONTFIX

(Core :: Web Speech, defect)

Description

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

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.

Comment 1

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

eeejay, do you think you could take a look?
Shouldn't take much time.

See bug 1321812

Comment 2

[Marco Zehe (:MarcoZ)]

eeejay is on parental leave ATM, so please expect delays in response. :-)

Comment 3

[Eitan Isaacson [:eeejay]]

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.

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: Clean up the runnable labeling in webspeech. r?smaug

I think this was mostly taken care of, just cleaned it up a bit, and labeled some runnables that were missed.

Comment 5

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

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
window->Document()->EventTargetFor(TaskCategory::Other)->Dispatch(...)

Comment 6

[Eitan Isaacson [:eeejay]]

I have a wip here. Need to test this on OSX.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a11373afc0a9da80969df8aaa599f2d760df0b7e

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Label more runnables and dispatch them to correct group/category. r?smaug

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
SystemGroup.

Comment 8

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

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.