Closed
Bug 1366089
Opened 7 years ago
Closed 7 years ago
nsIGlobalObject::EventTargetFor should return the worker thread for worker globals
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: billm, Assigned: bkelly)
References
Details
Attachments
(1 file, 6 obsolete files)
9.97 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•7 years ago
|
||
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?
Flags: needinfo?(bkelly)
Assignee | ||
Comment 2•7 years ago
|
||
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?
Flags: needinfo?(bkelly)
Assignee | ||
Comment 3•7 years ago
|
||
(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.
Reporter | ||
Comment 4•7 years ago
|
||
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.
Attachment #8870136 -
Flags: review?(bkelly)
Assignee | ||
Comment 5•7 years ago
|
||
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 -
Flags: review?(bkelly)
Assignee | ||
Updated•7 years ago
|
Assignee: wmccloskey → bkelly
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89cc767811f07e6b5ae827988ce297be8a277ec2
Assignee | ||
Updated•7 years ago
|
Attachment #8870136 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8880921 -
Attachment is obsolete: true
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4d961e69eaf9ae2a03e771018944742a596eafb
Attachment #8880927 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=606a44f2447ff118fcc13b1f04d425a81ae5166a
Attachment #8880953 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b7db5c99fe172ff731917cc5922dac9f1807025
Attachment #8881239 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=322736fa59f407f3d9814c50c5311597280d2fb0
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dca9c021db05cd222822be7add029cf970bc96f1
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•7 years ago
|
||
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().
Attachment #8881264 -
Attachment is obsolete: true
Attachment #8881859 -
Flags: review?(amarchesini)
Comment 14•7 years ago
|
||
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+
Comment 15•7 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f83eb621e672 Make EventTargetFor() support worker threads. r=baku
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f83eb621e672
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•