Bug 215594

Summary: A PiP window is closed when the video element is removed from DOM
Product: WebKit Reporter: Peng Liu <peng.liu6>
Component: MediaAssignee: Peng Liu <peng.liu6>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, pdr, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=232125
Attachments:
Description Flags
Patch
none
fix build failures
none
fix build failures again
eric.carlson: review+
patch for landing none

Peng Liu
Reported 2020-08-17 16:31:41 PDT
A PiP window will be closed when the video element is removed from DOM
Attachments
Patch (6.48 KB, patch)
2020-08-19 10:57 PDT, Peng Liu 		no flags
DetailsFormatted DiffDiff
fix build failures (6.61 KB, patch)
2020-08-19 11:12 PDT, Peng Liu 		no flags
DetailsFormatted DiffDiff
fix build failures again (6.57 KB, patch)
2020-08-19 11:16 PDT, Peng Liu 		eric.carlson: review+
DetailsFormatted DiffDiff
patch for landing (9.13 KB, patch)
2020-08-19 14:05 PDT, Peng Liu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Peng Liu
Comment 1 2020-08-17 16:33:29 PDT
<rdar://problem/66633670>
Peng Liu
Comment 2 2020-08-19 10:57:28 PDT
Created attachment 406854 [details] Patch
Peng Liu
Comment 3 2020-08-19 11:12:23 PDT
Created attachment 406858 [details] fix build failures
Peng Liu
Comment 4 2020-08-19 11:16:09 PDT
Created attachment 406859 [details] fix build failures again
Eric Carlson
Comment 5 2020-08-19 11:44:17 PDT
Comment on attachment 406859 [details] fix build failures again View in context: https://bugs.webkit.org/attachment.cgi?id=406859&action=review > Source/WebCore/html/HTMLMediaElement.cpp:4931 > + if (m_videoFullscreenMode == VideoFullscreenModePictureInPicture) > + return true; > + It took me a minute to figure out why this is necessary, so I think it would be useful to have a short comment here. > LayoutTests/media/remove-video-element-in-pip-from-document.html:31 > + setTimeout(endTest, 4000); We shouldn't use a timeout to end tests if at all possible. Four seconds is a very long time to wait for most test runs, but will probably be too short on a very heavily loaded bot. Luckily, I think you can do what you need with a MutationObserver instead.
Peng Liu
Comment 6 2020-08-19 14:05:57 PDT
Created attachment 406871 [details] patch for landing
Peng Liu
Comment 7 2020-08-19 14:17:51 PDT
Comment on attachment 406859 [details] fix build failures again View in context: https://bugs.webkit.org/attachment.cgi?id=406859&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:4931 >> + > > It took me a minute to figure out why this is necessary, so I think it would be useful to have a short comment here. Good suggestion! Fixed. >> LayoutTests/media/remove-video-element-in-pip-from-document.html:31 >> + setTimeout(endTest, 4000); > > We shouldn't use a timeout to end tests if at all possible. Four seconds is a very long time to wait for most test runs, but will probably be too short on a very heavily loaded bot. > > Luckily, I think you can do what you need with a MutationObserver instead. Fixed. The new patch uses a MutationObserver to observe the event that the video element has been removed from DOM, and set up a one-second timer to end the test.
EWS
Comment 8 2020-08-19 15:36:42 PDT
Committed r265904: <https://trac.webkit.org/changeset/265904> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406871 [details].
Note You need to log in before you can comment on or make changes to this bug.