[webkit-reviews] review granted: [Bug 204263] Share more code between WindowEventLoop and WorkerEventLoop by introducing EventLoopTaskGroup : [Attachment 383692] Fixed the tests

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 204263: Share more code between WindowEventLoop and WorkerEventLoop by
introducing EventLoopTaskGroup
https://bugs.webkit.org/show_bug.cgi?id=204263

Attachment 383692: Fixed the tests

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




--- Comment #5 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 383692
  --> https://bugs.webkit.org/attachment.cgi?id=383692
Fixed the tests

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

> Source/WebCore/ChangeLog:15
> +	   Each task is now represented as an instance of a concrete subclass
of EventLoopTask, which has
> +	   a pure virtual execute() member function. Its primary purpose is to
know EventLoopTaskGroup to which
> +	   each task belongs. One of subclasss, EventLoopFunctionDispatchTask,
is used to store a callback function
> +	   and another subclass, ActiveDOMObjectEventDispatchTask, is used to
dispatch an async event.

Why all this subclassing? Are there likely be many more subclasses in the
future? What is wrong with 'if'/'switch'/Variant<>?

> Source/WebCore/dom/AbstractEventLoop.h:66
> +class AbstractEventLoop : public RefCounted<AbstractEventLoop>, public
CanMakeWeakPtr<AbstractEventLoop> {

AbstractEventLoop is not very abstract at all anymore. Could it be named into
something more descriptive (like just EventLoop)? Or is this a spec name?

> Source/WebCore/dom/AbstractEventLoop.h:73
> +    void suspendGroup(EventLoopTaskGroup&);

This appears to have no callers.

> Source/WebCore/dom/AbstractEventLoop.h:88
> +    // Use a global queue instead of multiple task queues since HTML5 spec
allows UA to pick arbitrary queue.
> +    Vector<std::unique_ptr<EventLoopTask>> m_tasks;

I don't understand how the comment relates to the line below. Are you
describing some possible alternative implementation strategy?

> Source/WebCore/dom/AbstractEventLoop.h:120
> +	   ASSERT(m_state != State::Stopped);
> +	   m_state = State::Suspended;
> +	   // EventLoop doesn't do anything upon suspension.

Might be better to describe what this does (suspends when trying to run) rather
than what it (obviously) doesn't do.

> Source/WebCore/dom/WindowEventLoop.cpp:81
> +void AbstractEventLoop::resumeGroup(EventLoopTaskGroup& group)
>  {
> -    ASSERT(isMainThread());
> -    if
(!m_documentIdentifiersForSuspendedTasks.contains(document.identifier()))
> +    ASSERT(isContextThread());
> +    if (!m_groupsWithSuspenedTasks.contains(group))
>	   return;
>      scheduleToRunIfNeeded();
>  }

I'm wondering if m_groupsWithSuspenedTasks optimization is even needed. Would
it really hurt to scheduleToRunIfNeeded() unconditionally? Would simplify the
logic...

> Source/WebCore/workers/WorkerGlobalScope.h:70
> -    AbstractEventLoop& eventLoop() final;
> +    EventLoopTaskGroup& eventLoop() final;

Bit weird that eventLoop() doesn't return an event loop, but ok.