Closed
Bug 1366089
Opened 8 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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Assignee: wmccloskey → bkelly
Assignee | ||
Comment 6•7 years ago
|
||
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
|
||
Attachment #8880927 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8880953 -
Attachment is obsolete: true
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8881239 -
Attachment is obsolete: true
Assignee | ||
Comment 11•7 years ago
|
||
Assignee | ||
Comment 12•7 years ago
|
||
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 |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•