Closed Bug 1350937 Opened 3 years ago Closed 3 years ago

Label dom/notifications

Categories

(Core :: DOM: Core & HTML, enhancement)

50 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: smaug, Assigned: Lina)

References

Details

Attachments

(1 file, 2 obsolete files)

See Bug 1321812.

Kit, would you have time to take a look?
Or do you know who might be familiar with this code?
Flags: needinfo?(kit)
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?
Flags: needinfo?(kit) → needinfo?(bugs)
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
Flags: needinfo?(bugs)
Depends on: 1351772
Attached patch label.patch (obsolete) — Splinter Review
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.
Assignee: nobody → kit
Status: NEW → ASSIGNED
Attached patch label.patch (obsolete) — Splinter Review
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.
Attachment #8852663 - Attachment is obsolete: true
Attachment #8853566 - Flags: review?(wmccloskey)
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.
Attachment #8853566 - Flags: review?(wmccloskey) → feedback+
Attached patch label.patchSplinter Review
Thanks very much, Bill! I incorporated your feedback; this patch is a lot simpler. :-)
Attachment #8853566 - Attachment is obsolete: true
Attachment #8854915 - Flags: review?(wmccloskey)
Attachment #8854915 - Flags: review?(wmccloskey) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/840274759aba
Label web notification runnables. r=billm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/840274759aba
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.