Prevent race condition during Worker shutdown
Created attachment 184031 [details]
Comment on attachment 184031 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=184031&action=review
Patch looks fine, but there are a few stylistic issues worth fixing. review- to fix those
No need to have me be the one who reviews next time around.
> + (MessageQueue):
> + (WTF):
> + (WTF::::appendAndKill):
All three of these lines look bogus. Please fix them if the script generates something that looks like nonsense. Per-function comments are even better.
> + (WorkerRunLoop):
Please remove this bogus line.
> + void postTask(PassOwnPtr<ScriptExecutionContext::Task>, bool terminate = false);
> + void postTaskForMode(PassOwnPtr<ScriptExecutionContext::Task>, const String& mode, bool terminate = false);
I’m not sure that a boolean or enum here is superior to a separate postTaskAndTerminate function. It seems that a separate function will work just fine:
void postTaskAndTerminate(PassOwnPtr<ScriptExecutionContext::Task> task)
No reason to touch these existing functions at all.
> + const bool terminate = true;
> + m_runLoop.postTask(WorkerThreadShutdownStartTask::create(), terminate);
This named boolean argument idiom is not what we use for things like this in WebKit; it’s too easy for it to be wrong or get out of sync with the function we are calling. Instead we use enums, which is a perfect fit here. We can and should define an enum in WorkerRunLoop.h.
Thanks, darin@. Seemingly contradictory advice...
(In reply to comment #2)
> No reason to touch these existing functions at all.
> We can and should define an enum in WorkerRunLoop.h.
.. but I went with adding postTaskAndTerminate() since that seems the clearest.
Created attachment 184052 [details]
Comment on attachment 184052 [details]
Clearing flags on attachment: 184052
Committed r140483: <http://trac.webkit.org/changeset/140483>
All reviewed patches have been landed. Closing bug.