Bug 211730 – Have ScrollingThread use a RunLoop rather than rolling its own

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Simon Fraser (smfr)]

Have ScrollingThread use a RunLoop rather than rolling its own

[Simon Fraser (smfr)]

Created attachment 399025 [details]
Patch

[Geoffrey Garen]

Comment on attachment 399025 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingThread.cpp:58
> +    static std::once_flag onceFlag;
> +    std::call_once(onceFlag, [] {
> +        auto& scrollingThread = ScrollingThread::singleton();
> +        scrollingThread.createThreadIfNeeded();
> +    });

Should this code just go into the singleton() function?

[Simon Fraser (smfr)]

Created attachment 399029 [details]
Patch

[Geoffrey Garen]

I can't imagine why, but the EWS Mac-wk2 crash in swipe/pushState-cached-back-swipe.html is reproducible and new.

[Darin Adler]

Comment on attachment 399029 [details]
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!

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #5)
> Comment on attachment 399029 [details]

> 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!

Even better, I think I can just use a BinarySemaphore.

[Simon Fraser (smfr)]

Created attachment 399052 [details]
Patch

[Darin Adler]

Comment on attachment 399052 [details]
Patch

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

Sure does look nice

> Source/WebCore/page/scrolling/ScrollingThread.cpp:58
> +

I’d leave this blank line out.

[EWS]

Committed r261494: <https://trac.webkit.org/changeset/261494>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399052 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63106926>