Summary: | Have ScrollingThread use a RunLoop rather than rolling its own | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Component: | Scrolling | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cmarcelo, darin, ews-watchlist, fred.wang, ggaren, jamesr, luiz, simon.fraser, thorton, tonikitoo, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2020-05-11 10:28:14 PDT
Created attachment 399025 [details]
Patch
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? Created attachment 399029 [details]
Patch
I can't imagine why, but the EWS Mac-wk2 crash in swipe/pushState-cached-back-swipe.html is reproducible and new. 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! (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. Created attachment 399052 [details]
Patch
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. Committed r261494: <https://trac.webkit.org/changeset/261494> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399052 [details]. |