Closed Bug 1282366 Opened 8 years ago Closed 8 years ago

Improve WorkerHolder use in Runnables

Categories

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

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file)

No description provided.
Attached patch holder.patch (deleted) — Splinter Review
There are a couple of runnables (in Console and in BindingUtils) that use WorkerHolder just to keep the worker alive. With this patch I want to unify these runnables implementing a common base class and use ModifyBusyCount instead using WorkerHolder. This is a good first step to make WorkerHolder a thread-only class so that we can add assertions in the CTOR and in the DTOR.
Attachment #8765362 - Flags: review?(khuey)
Comment on attachment 8765362 [details] [diff] [review] holder.patch Review of attachment 8765362 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/ActorsChild.cpp @@ +927,5 @@ > + if (NS_WARN_IF(!mWorkerPrivate->ModifyBusyCountFromWorker(true))) { > + return false; > + } > + > + if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) { here too ::: dom/workers/WorkerRunnable.cpp @@ +721,5 @@ > + RunBackOnWorkerThread(); > + return false; > + } > + > + if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(this)))) { NS_DispatchToMainThread can't fail until after xpcom-shutdown-threads, at which point the worker has to be gone. So you can MOZ_ALWAYS_SUCCEEDS this. ::: dom/workers/WorkerRunnable.h @@ +410,5 @@ > > +// This runnable is an helper class for dispatching something from a worker > +// thread to the main-thread and back to the worker-thread. During this > +// operation, this class will keep the worker alive. > +class WorkerToMainThreadRunnable : public Runnable I have to admit that I don't care for this name, but I can't think of anything much better. WorkerProxyToMainThreadRunnable? WorkerPingPongToMainThreadRunnable? :P I think the name needs to indicate that it comes back ...
Attachment #8765362 - Flags: review?(khuey) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1286487
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: