Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve flakiness in VideoControlsManager tests and re-enable them #4843

Closed

Conversation

heycam
Copy link
Contributor

@heycam heycam commented Sep 30, 2022

f83459d

Resolve flakiness in VideoControlsManager tests and re-enable them
https://bugs.webkit.org/show_bug.cgi?id=175909
rdar://problem/42793000

Reviewed by Eric Carlson.

The primary issue with the flakiness of these tests is that they run
some script, e.g. "muteFirstVideoAndScrollToSecondVideo()", and then
immediately add a handler for a message that the page sends in response
to running that script. Depending on timing, the Web process can end up
sending the message before the UI process has added the handler.

This patch adds a new expectControlsManager function that takes a setup
function to run after the message handler is added but before we start
waiting for the message.

A secondary issue affects
VideoControlsManagerMultipleVideosSwitchControlledVideoWhenScrolling:
the setTimeout callback in the test file that sends the "scrolled"
message after performing the scroll to the second video uses a timeout
of 0ms. Since bug 221124, setTimeout(..., 0) callbacks get run first the
next time around the event loop, and this means the UI process does not
get a chance to be informed about the change in active
PlaybackSessionContextIdentifier (through a SetUpPlaybackControlsManagerWithID
message) before it continues to the request the controlled element ID.
That request is made using the old PlaybackSessionContextIdentifier. To
resolve this, we bump the timeout to 1ms (the effective minimum before
bug 221124) so that the SetUpPlaybackControlsManagerWithID message is
sent first.

* Tools/TestWebKitAPI/Tests/WebKitCocoa/VideoControlsManager.mm:
(-[VideoControlsManagerTestWebView expectControlsManager:afterReceivingMessage:]):
(-[VideoControlsManagerTestWebView expectControlsManager:afterReceivingMessage:withSetup:]):
(TestWebKitAPI::TEST):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/large-videos-autoplaying-scroll-to-video.html:

Canonical link: https://commits.webkit.org/255057@main

c2113f2

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@heycam heycam self-assigned this Sep 30, 2022
@heycam heycam added Media Bugs related to the HTML 5 Media elements. WebKit Nightly Build labels Sep 30, 2022
Copy link
Contributor

@eric-carlson eric-carlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@heycam heycam added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Oct 1, 2022
@webkit-commit-queue
Copy link
Collaborator

Committed 255057@main (f83459d): https://commits.webkit.org/255057@main

Reviewed commits have been landed. Closing PR #4843 and removing active labels.

@webkit-commit-queue webkit-commit-queue force-pushed the video-controls-manager-test-flakiness branch from c2113f2 to f83459d Compare October 1, 2022 01:55
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
4 participants