[webkit-reviews] review denied: [Bug 107577] Prevent race condition during Worker shutdown : [Attachment 184031] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 22 14:29:45 PST 2013
Darin Adler <darin at apple.com> has denied Joshua Bell <jsbell at chromium.org>'s
request for review:
Bug 107577: Prevent race condition during Worker shutdown
https://bugs.webkit.org/show_bug.cgi?id=107577
Attachment 184031: Patch
https://bugs.webkit.org/attachment.cgi?id=184031&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
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.
> Source/WTF/ChangeLog:14
> + (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.
> Source/WebCore/ChangeLog:19
> + (WorkerRunLoop):
Please remove this bogus line.
> Source/WebCore/workers/WorkerRunLoop.h:64
> + 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)
{
m_messageQueue.appendAndKill(Task::create(task,
defaultMode().isolatedCopy()));
}
No reason to touch these existing functions at all.
> Source/WebCore/workers/WorkerThread.cpp:275
> + 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.
More information about the webkit-reviews
mailing list