Right now, nsIGlobalObject::EventTargetFor returns the main thread for all non-nsGlobalWindow globals. It would make sense for it to return the worker thread for any worker globals.
I started to work on a patch for this, but I'm not sure what event target to return. Can I just return mWorkerPrivate->mThread? Will that work if the worker hasn't started up yet or something (is that possible once we have a global for it)? Also, how should I deal with mThread being private? Can I make an accessor, or should I make WorkerGlobalScope a friend class?
Try WorkerPrivate::GetEventTarget(): http://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#3001 It can return null, though. I guess the nsIGlobalObject method should return a failure code in that case?
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #2) > I guess the nsIGlobalObject method should > return a failure code in that case? Also Dispatch() can return failure if you hold the event target past the lifetime of the thread. I think these are just the reality of working with an nsIEventTarget representing an ephemeral thread.
There were a couple annoying issues here: 1. AbstractMainThreadFor doesn't make a lot of sense for workers, so I just made it assert. 2. EventTargetFor is never supposed to return null (based on the name), but mWorkerPrivate->GetEventTarget() can return null. So I invented this FakeEventTarget thing that always returns an error when you dispatch to it. I'm not very happy with the name. 3. EventTargetFor doesn't return an already_AddRefed, so someone else needs to own the FakeEventTarget. So I created it in the WorkerGlobalScope constructor. Happy to take suggestions about better ways to do this.
Comment on attachment 8870136 [details] [diff] [review] patch So, I think we should probably just make WorkerPrivate::GetEventTarget() an infallible EventTarget() method instead. It can do the Dispatch() failing if the thread is not active. I'll steal this bug to make that change.
Attachment #8870136 - Attachment is obsolete: true
Attachment #8880927 - Attachment is obsolete: true
Attachment #8880953 - Attachment is obsolete: true
Attachment #8881239 - Attachment is obsolete: true
Andrea, this patch does a few things: 1) Make WorkerPrivate::GetEventTarget() always return an nsISerialEvent target. If we would have returned nullptr before we now just Disable() the event target instead. So we have an event target, but any Dispatch() will return an error. 2) Drop the parent status check in GetEventTarget(). This makes the check match the WorkerHolder checks. I don't think the parent check is really necessary or makes sense here. 3) Make the WorkerGlobalScope::EventTargetFor() return the WorkerPrivate::GetEventTarget().
Comment on attachment 8881859 [details] [diff] [review] Make EventTargetFor() support worker threads. r=baku Review of attachment 8881859 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerScope.cpp @@ +493,5 @@ > } > } > > +nsresult > +WorkerGlobalScope::Dispatch(const char* aname, TaskCategory aCategory, aName @@ +1077,5 @@ > } > } > > +nsresult > +WorkerDebuggerGlobalScope::Dispatch(const char* aname, TaskCategory aCategory, aName ::: dom/workers/WorkerScope.h @@ +208,5 @@ > + // Override DispatchTrait API to target the worker thread. Dispatch may > + // return failure if the worker thread is not alive. > + nsresult > + Dispatch(const char* aname, TaskCategory aCategory, > + already_AddRefed<nsIRunnable>&& aRunnable) override; aName @@ +420,5 @@ > > + // Override DispatchTrait API to target the worker thread. Dispatch may > + // return failure if the worker thread is not alive. > + nsresult > + Dispatch(const char* aname, TaskCategory aCategory, aName
Attachment #8881859 - Flags: review?(amarchesini) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/f83eb621e672 Make EventTargetFor() support worker threads. r=baku
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.