Bug 215594 - A PiP window is closed when the video element is removed from DOM
Summary: A PiP window is closed when the video element is removed from DOM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-17 16:31 PDT by Peng Liu
Modified: 2021-10-21 18:19 PDT (History)
14 users (show)

See Also:

Attachments
Patch (6.48 KB, patch)
2020-08-19 10:57 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
fix build failures (6.61 KB, patch)
2020-08-19 11:12 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
fix build failures again (6.57 KB, patch)
2020-08-19 11:16 PDT, Peng Liu 		eric.carlson: review+
Details | Formatted Diff | Diff
patch for landing (9.13 KB, patch)
2020-08-19 14:05 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-08-17 16:31:41 PDT 
A PiP window will be closed when the video element is removed from DOM
Comment 1 Peng Liu 2020-08-17 16:33:29 PDT 
<rdar://problem/66633670>
Comment 2 Peng Liu 2020-08-19 10:57:28 PDT 
Created attachment 406854 [details]
Patch
Comment 3 Peng Liu 2020-08-19 11:12:23 PDT 
Created attachment 406858 [details]
fix build failures
Comment 4 Peng Liu 2020-08-19 11:16:09 PDT 
Created attachment 406859 [details]
fix build failures again
Comment 5 Eric Carlson 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.
Comment 6 Peng Liu 2020-08-19 14:05:57 PDT 
Created attachment 406871 [details]
patch for landing
Comment 7 Peng Liu 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.
Comment 8 EWS 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].