[webkit-reviews] review granted: [Bug 211730] Have ScrollingThread use a RunLoop rather than rolling its own : [Attachment 399029] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 211730: Have ScrollingThread use a RunLoop rather than rolling its own
https://bugs.webkit.org/show_bug.cgi?id=211730

Attachment 399029: Patch

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




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

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

The EWS failure in mac-wk2 tests looks like it *might* be indirectly caused by
this change.

r=me assuming you can resolve that

> Source/WebCore/page/scrolling/ScrollingThread.cpp:41
> +#if PLATFORM(IOS_FAMILY)
> +    ASSERT_NOT_REACHED();
> +#endif

Could we follow the call sites here and instead not even compile this for
IOS_FAMILY?

> Source/WebCore/page/scrolling/ScrollingThread.cpp:63
> +    ScrollingThread::singleton().runLoop()->dispatch(WTFMove(function));

Why does runLoop return a pointer if it’s guaranteed to be non-null? Or, to ask
the same question another way, what guarantees that runLoop() is non-null here
that doesn’t apply elsewhere?

> Source/WebCore/page/scrolling/ScrollingThread.cpp:73
> +void ScrollingThread::createThread()

Why not put all this code inside inside the constructor? Is there a reason we
want to keep this broken out into a separate named function?

> Source/WebCore/page/scrolling/ScrollingThread.cpp:87
> +void ScrollingThread::initializeRunLoop()

Why not put all this code inside the lambda inside createThread? Is there a
reason we want to keep this broken out into a separate named function?

If we moved this into createThread, it would likely make it clear that
m_initializeRunLoopMutex can be a local variable instead of a data member!