Status

()

2 years ago
10 months ago

People

(Reporter: smaug, Assigned: eeejay)

Tracking

(Blocks: 1 bug)

50 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

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

See bug 1321812

Comment 2

2 years ago
eeejay is on parental leave ATM, so please expect delays in response. :-)
(Assignee)

Updated

a year ago
Flags: needinfo?(eitan)
(Assignee)

Comment 3

a year ago
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)
(Assignee)

Comment 4

a year ago
Created attachment 8881909 [details] [diff] [review]
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.
Attachment #8881909 - Flags: review?(bugs)
(Reporter)

Comment 5

a year ago
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(...)
Attachment #8881909 - Flags: review?(bugs) → review-
Assignee: nobody → eitan
(Assignee)

Comment 7

a year ago
Created attachment 8886241 [details] [diff] [review]
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.
Attachment #8886241 - Flags: review?(bugs)
(Assignee)

Updated

a year ago
Attachment #8881909 - Attachment is obsolete: true
(Reporter)

Comment 8

a year ago
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.