Bug 214771 – WebCoreResourceHandleAsOperationQueueDelegate can use RunLoop::dispatch

• Status: REOPENED
• Component: New Bugs

[Geoffrey Garen]

WebCoreResourceHandleAsOperationQueueDelegate can use RunLoop::dispatch

[Geoffrey Garen]

Created attachment 405207 [details]
Patch

[Alex Christensen]

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?

[Geoffrey Garen]

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

[Geoffrey Garen]

Created attachment 405233 [details]
Patch

[Darin Adler]

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.

[Geoffrey Garen]

Created attachment 405295 [details]
Patch for landing

[Geoffrey Garen]

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.

[EWS]

Committed r264955: <https://trac.webkit.org/changeset/264955>

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

[Radar WebKit Bug Importer]

<rdar://problem/66189145>

[Aakash Jain]

(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

[Karl Rackler]

Reverted r264955 for reason:

Reverting because this commit may have caused issues with tests. 

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