Closed
Bug 1350953
Opened 8 years ago
Closed 5 years ago
Label service worker runnables
Categories
(Core :: DOM: Service Workers, enhancement, P3)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: smaug, Unassigned)
References
Details
(Whiteboard: [QDL][BACKLOG][DOM])
See Bug 1321812
I have no idea how easy it is to get reasonable Tab/DocGroup to dispatch.
Comment 1•8 years ago
|
||
As I know, WorkerMainThreadRunnable and WorkerPrivate::DispatchToMainThread() use EventTargetFor(TaskCategory::Worker) for main-thread dispatching:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/workers/WorkerPrivate.cpp#4479
Comment 2•8 years ago
|
||
Note, most of the ServiceWorkerManager and its associated runnables will be moving to parent process or worker process in the near future. Labeling the current runnables may not be worth prioritizing at the moment.
Comment 3•8 years ago
|
||
Also, I anticipate that we may start using MozPromise more in service worker code. I'm not sure how that impacts labeling efforts.
Updated•8 years ago
|
Priority: -- → P3
Reporter | ||
Comment 4•8 years ago
|
||
MozPromise uses still normal event loop somewhere underneath, so they should be labeled.
Reporter | ||
Comment 5•8 years ago
|
||
But if all that serviceworker stuff in content processes will just go away, nothing to label.
(New code of course should be labeled)
Comment 6•8 years ago
|
||
Olli, Do we need a new bug to track the labeling of the new code? Or will labeling be implemented as part of new development?
Flags: needinfo?(bugs)
Reporter | ||
Comment 7•8 years ago
|
||
I think bevis is better person to ask labeling questions.
But in general I'd prefer if all the new code was labeled properly from the beginning.
Flags: needinfo?(bugs) → needinfo?(btseng)
Comment 8•8 years ago
|
||
Yes, labeling shall be part of new development.
For the use of MozPromise, it's more related to which AbstractThread instance shall be used.
For MozPromises on the main thread, SchedulerGroup::AbstractMainThreadFor() is the option for labeling:
http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/xpcom/threads/SchedulerGroup.h#74
For the use of MozPromise off the main thread,
it's still TBD to get the default AbstractThread instance.
AFAIK, AbstractThread::GetCurrent() is the current option but it's a wrong option and is abused in several places.
In short, current implementation of AbstractThread::GetCurrent() is wrong.
The default TLS value of AbstractThread::sCurrentThreadTLS is set to AbstractThread::MainThread() on main thread and set to MessageLoopAbstractThreadWrapper off main thread which is incorrect!
According to the comment in its declaration,
The return value of AbstractThread::GetCurrent() shall always be nullptr unless the caller is in a runnable that was dispatched to an associated AbstractThread:
http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/xpcom/threads/AbstractThread.h#40-42
:jwwang might have better suggestion of how AbstractThread shall be accessed by default in the future for MozPromise.
Flags: needinfo?(btseng) → needinfo?(jwwang)
Comment 9•8 years ago
|
||
I think having different APIs for main thread vs other code makes it harder to build features that need to work on both window and worker. Shouldn't we be striving to have less "if main thread do special logic" stuff in the tree?
Comment 10•8 years ago
|
||
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #8)
> :jwwang might have better suggestion of how AbstractThread shall be accessed
> by default in the future for MozPromise.
I am not sure I follow. MozPromise never assumes a particular AbstractThread to use. It is up to the caller to supply a target thread to MozPromise::Then(...).
Flags: needinfo?(jwwang)
Updated•7 years ago
|
Whiteboard: [QDL][DOM][BACKLOG]
Updated•7 years ago
|
Whiteboard: [QDL][DOM][BACKLOG] → [QDL][BACKLOG][DOM]
Comment 11•5 years ago
|
||
:smaug, this blocks an already resolved issue. Can I then assume, we do not need/want this any more?
Flags: needinfo?(bugs)
Comment 12•5 years ago
|
||
(In reply to Jens Stutte [:jstutte] from comment #11)
:smaug, this blocks an already resolved issue. Can I then assume, we do not need/want this any more?
Yes.
Flags: needinfo?(bugs)
Updated•5 years ago
|
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•