See Bug 1321812. Kit, would you have time to take a look?

Or do you know who might be familiar with this code?

I'm happy to take a look. William knows this code a lot better than I do, though, and can likely get the conversion done a lot quicker. :-) This is the first I've heard about labeling runnables, so I'm sorry for the basic questions. I read through bug 1321812, :billm's blog post, and the wiki, and I'm confused about a few things: * We have several classes that inherit from `MainThreadWorkerRunnable`. IIUC, those are dispatched from the main thread to the worker thread. Bug 1321812, comment 9 says "Only the runnables to the main thread of the content process has to be labeled", so I assume they don't need to be labeled? If they do, how do I convert calls to `WorkerMainThreadRunnable::Dispatch` (http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/workers/WorkerRunnable.cpp#571) with two arguments? Do I need to have the subclass implement `nsINamed`? Should I use `mWorkerPrivate->GetDocument()->GetDocGroup()` on the main thread, then call `docGroup->EventTargetFor(...)` on the worker thread? https://wiki.mozilla.org/Quantum/DOM suggests this is safe, but I'm not sure. * To convert the `NS_DispatchToMainThread` calls, it looks like I'll need to have the runnable hold an `nsCOMPtr<nsIEventTarget>`. Is that safe to use off the main thread? I looked at the script loader example on https://wiki.mozilla.org/Quantum/DOM, but I see `NotifyOffThreadScriptLoadCompletedRunnable` is created on the main thread, and it looks like our runnables (`NotificationGetRunnable`, `FocusWindowRunnable`) are not? In that example, it's also not clear to me which thread `static void Dispatch(already_AddRefed<NotifyOffThreadScriptLoadCompletedRunnable>&& aSelf)` is called on. If it's not safe, how do I have the worker runnables use the correct `DocGroup` when posting back to the main thread?

this is only for stuff dispatched to main thread. nsIEventTargets tend to be thread safe (main thread itself being an nsIEventTarget). You can get nsIEventTarget from DocGroup or TabGroup and pass the nsIEventTarget around other threads. DocGroup and TabGroup themselves are threadsafe too

I think this should do it, but it's blocked on bug 1351772. As written, this patch will fail to dispatch runnables from workers to the main thread.

Bill, can you take a look, please? Ben pointed me to `WorkerPrivate::MainThreadEventTarget` and `WorkerPrivate::DispatchToMainThread` in bug 1351772, and I *think* I'm getting the correct `DocGroup` on the main thread.

Comment on attachment 8853566 [details] [diff] [review] label.patch Review of attachment 8853566 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/notification/Notification.cpp @@ +630,5 @@ > { > + nsCOMPtr<nsIRunnable> resolver = > + NewRunnableMethod(this, &NotificationPermissionRequest::ResolvePromise); > + return mDocGroup->Dispatch( > + "NotificationPermissionRequest::DispatchResolvePromise", I guess I could use `__func__` here, too. Or is it better to use strings?

Comment on attachment 8853566 [details] [diff] [review] label.patch Review of attachment 8853566 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Kit. I suggested a couple simplifications below, so I'd like to see another revision. It looks good though. ::: dom/notification/Notification.cpp @@ +630,5 @@ > { > + nsCOMPtr<nsIRunnable> resolver = > + NewRunnableMethod(this, &NotificationPermissionRequest::ResolvePromise); > + return mDocGroup->Dispatch( > + "NotificationPermissionRequest::DispatchResolvePromise", Please use strings everywhere. It makes it easier to search for them. @@ +1869,4 @@ > if (aCallback.WasPassed()) { > permissionCallback = &aCallback.Value(); > } > + DocGroup* docGroup = window->GetDocGroup(); Instead of getting the DocGroup, it's easier to do: global->Dispatch(<name>, <category>, <event>); since nsIGlobalObject implements DispatcherTrait. That will work even if the DocGroup is null for some reason. @@ +2074,5 @@ > RefPtr<NotificationGetRunnable> r = > new NotificationGetRunnable(origin, aFilter.mTag, callback); > > + nsIEventTarget* target = nullptr; > + if (DocGroup* docGroup = doc->GetDocGroup()) { Same deal here about going through global. @@ +2302,2 @@ > nsCOMPtr<nsIRunnable> closeNotificationTask = > + new NotificationTask(__func__, Move(ref), NotificationTask::eClose); I'd rather write the function name out here so that it's more searchable. @@ +2748,2 @@ > nsCOMPtr<nsIRunnable> showNotificationTask = > + new NotificationTask(__func__, Move(ref), NotificationTask::eShow); Same thing about __func___. @@ +2808,5 @@ > return NS_OK; > } > > +nsIEventTarget* > +Notification::MainThreadEventTarget() const { Please put the brace on its own line. @@ +2811,5 @@ > +nsIEventTarget* > +Notification::MainThreadEventTarget() const { > + nsIEventTarget* target = nullptr; > + if (NS_IsMainThread()) { > + if (nsCOMPtr<nsPIDOMWindowInner> owner = GetOwner()) { I think you can use GetOwnerGlobal()->EventTargetFor(...). Although you'll have to null check GetOwnerGlobal(). @@ +2821,5 @@ > + } > + } else if (mWorkerPrivate) { > + target = mWorkerPrivate->MainThreadEventTarget(); > + } > + return target; I'm concerned that this can return null. It might be good to return do_GetMainThread() if target is null here. Then you can return the null checks in callers.

Thanks very much, Bill! I incorporated your feedback; this patch is a lot simpler. :-)

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/840274759aba Label web notification runnables. r=billm