[webkit-reviews] review granted: [Bug 188246] Add the support for micro tasks in workers : [Attachment 346561] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 188246: Add the support for micro tasks in workers
https://bugs.webkit.org/show_bug.cgi?id=188246

Attachment 346561: Patch

https://bugs.webkit.org/attachment.cgi?id=346561&action=review




--- Comment #34 from Darin Adler <darin at apple.com> ---
Comment on attachment 346561
  --> https://bugs.webkit.org/attachment.cgi?id=346561
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346561&action=review

> Source/WebCore/ChangeLog:3
> +	   Add the support for micro tasks in workers

Grammar error here: the word "the" should be omitted

> Source/WebCore/ChangeLog:10
> +	   This patch adds microtask mechanism to workers. To adopt the
existing microtask mechanism in main thread,
> +	   we extends JSMainThreadExecState for non-main-threads. We rename it
to JSExecState, and store stacked
> +	   ExecState* data in thread local storage in ThreadGlobalData instead
of a static variable s_mainThreadState.

Some other minor grammar errors here. Should read:

	This patch adds the microtask mechanism to workers. To adopt the
existing microtask mechanism from the main thread,
	we extend JSMainThreadExecState for non-main-threads. We rename it to
JSExecState, and store stacked
	ExecState* data in thread local storage in ThreadGlobalData instead of
a static variable s_mainThreadState.

> Source/WebCore/bindings/js/JSMicrotaskCallback.h:42
> +	   Ref<JSMicrotaskCallback> protectedThis(*this);

Would it be nicer to just protect m_task, not the entire JSMicrotaskCallback?

Another way to write this in modern code is:

    auto protectedThis { makeRef(*this) };

> Source/WebCore/bindings/js/JSMicrotaskCallback.h:49
> +	   ExecState* exec = m_globalObject->globalExec();
> +
> +	   JSExecState::runTask(exec, m_task);

I’d be tempted to do this in a single line instead of using a local variable.