Bug 214771

Summary: WebCoreResourceHandleAsOperationQueueDelegate can use RunLoop::dispatch
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
darin: review+
Patch for landing none

Description Geoffrey Garen 2020-07-24 17:10:26 PDT
WebCoreResourceHandleAsOperationQueueDelegate can use RunLoop::dispatch
Comment 1 Geoffrey Garen 2020-07-24 17:35:24 PDT
Created attachment 405207 [details]
Patch
Comment 2 Alex Christensen 2020-07-24 19:55:54 PDT
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?
Comment 3 Geoffrey Garen 2020-07-25 15:00:07 PDT
> 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.
Comment 4 Geoffrey Garen 2020-07-25 15:38:18 PDT
Created attachment 405233 [details]
Patch
Comment 5 Darin Adler 2020-07-25 22:10:30 PDT
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.
Comment 6 Geoffrey Garen 2020-07-27 11:06:48 PDT
Created attachment 405295 [details]
Patch for landing
Comment 7 Geoffrey Garen 2020-07-27 16:45:44 PDT
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.
Comment 8 EWS 2020-07-27 16:48:21 PDT
Committed r264955: <https://trac.webkit.org/changeset/264955>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405295 [details].
Comment 9 Radar WebKit Bug Importer 2020-07-27 16:49:27 PDT
<rdar://problem/66189145>
Comment 10 Aakash Jain 2020-07-28 03:46:10 PDT
(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
Comment 11 Karl Rackler 2020-07-28 10:03:07 PDT
Reverted r264955 for reason:

Reverting because this commit may have caused issues with tests. 

Committed r264993: <https://trac.webkit.org/changeset/264993>