Bug 215594 – A PiP window is closed when the video element is removed from DOM

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

A PiP window will be closed when the video element is removed from DOM

[Peng Liu]

<rdar://problem/66633670>

[Peng Liu]

Created attachment 406854 [details]
Patch

[Peng Liu]

Created attachment 406858 [details]
fix build failures

[Peng Liu]

Created attachment 406859 [details]
fix build failures again

[Eric Carlson]

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]

Created attachment 406871 [details]
patch for landing

[Peng Liu]

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]

Committed r265904: <https://trac.webkit.org/changeset/265904>

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