Summary: | WebCoreResourceHandleAsOperationQueueDelegate can use RunLoop::dispatch | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||||||
Component: | New Bugs | Assignee: | Geoffrey Garen <ggaren> | ||||||||
Status: | REOPENED --- | ||||||||||
Severity: | Normal | CC: | aakash_jain, achristensen, ap, benjamin, cdumez, cmarcelo, darin, ews-watchlist, rackler, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Geoffrey Garen
2020-07-24 17:10:26 PDT
Created attachment 405207 [details]
Patch
Comment on attachment 405207 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405207&action=review The code in WebCoreResourceHandleAsOperationQueueDelegate.mm took several iterations to get just right, and I don't think the code in WTF is equivalent. Could you include a radar with a list of clients? > Source/WTF/wtf/cf/RunLoopCF.cpp:91 > + if (schedulePair->mode() == kCFRunLoopCommonModes What about kCFRunLoopDefaultMode? > Could you include a radar with a list of clients? I can assemble a list. But what would the Radar say? > > Source/WTF/wtf/cf/RunLoopCF.cpp:91 > > + if (schedulePair->mode() == kCFRunLoopCommonModes > > What about kCFRunLoopDefaultMode? The kCFRunLoopDefaultMode was subtly incorrect. dispatch() dispatches in kCFRunLoopCommonModes, so if a client took care to first unschedule kCFRunLoopCommonModes (which WebKit schedules by default) and then schedule kCFRunLoopDefaultMode, applying this optimization would sometimes dispatch in the wrong modes. Created attachment 405233 [details]
Patch
Comment on attachment 405233 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405233&action=review > Source/WTF/wtf/cf/RunLoopCF.cpp:92 > + if (schedulePair->mode() == kCFRunLoopCommonModes > + && schedulePair->runLoop() == RunLoop::main().m_runLoop) Easier to read as a long line. Created attachment 405295 [details]
Patch for landing
Those failing API tests pass locally (Debug and Release), and the flakiness dashboard also reports that they fail regularly on bots. Not sure how to interpret these contradictory reasons to exonerate this patch. I guess I'll try landing and see what happens. Committed r264955: <https://trac.webkit.org/changeset/264955> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405295 [details]. (In reply to EWS from comment #8) > Committed r264955: <https://trac.webkit.org/changeset/264955> TestWebKitAPI.WKWebViewCloseAllMediaPresentations.PictureInPicture seems to have started to consistently timeout after this change. History: https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WKWebViewCloseAllMediaPresentations.PictureInPicture Reverted r264955 for reason: Reverting because this commit may have caused issues with tests. Committed r264993: <https://trac.webkit.org/changeset/264993> |