Bug 211730 - Have ScrollingThread use a RunLoop rather than rolling its own
Summary: Have ScrollingThread use a RunLoop rather than rolling its own
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-11 10:28 PDT by Simon Fraser (smfr)
Modified: 2020-05-11 14:52 PDT (History)
11 users (show)

See Also:


Attachments
Patch (11.95 KB, patch)
2020-05-11 10:30 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (15.84 KB, patch)
2020-05-11 11:04 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff
Patch (16.80 KB, patch)
2020-05-11 13:44 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2020-05-11 10:28:14 PDT
Have ScrollingThread use a RunLoop rather than rolling its own
Comment 1 Simon Fraser (smfr) 2020-05-11 10:30:10 PDT
Created attachment 399025 [details]
Patch
Comment 2 Geoffrey Garen 2020-05-11 10:34:52 PDT
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?
Comment 3 Simon Fraser (smfr) 2020-05-11 11:04:52 PDT
Created attachment 399029 [details]
Patch
Comment 4 Geoffrey Garen 2020-05-11 12:00:12 PDT
I can't imagine why, but the EWS Mac-wk2 crash in swipe/pushState-cached-back-swipe.html is reproducible and new.
Comment 5 Darin Adler 2020-05-11 12:56:12 PDT
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!
Comment 6 Simon Fraser (smfr) 2020-05-11 13:29:36 PDT
(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.
Comment 7 Simon Fraser (smfr) 2020-05-11 13:44:03 PDT
Created attachment 399052 [details]
Patch
Comment 8 Darin Adler 2020-05-11 13:49:19 PDT
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.
Comment 9 EWS 2020-05-11 14:51:31 PDT
Committed r261494: <https://trac.webkit.org/changeset/261494>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399052 [details].
Comment 10 Radar WebKit Bug Importer 2020-05-11 14:52:17 PDT
<rdar://problem/63106926>