Bug 215305 – Add the support to return to element fullscreen from picture-in-picture

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

Add the support to return to element fullscreen from picture-in-picture

[Peng Liu]

<rdar://problem/66424852>

[Peng Liu]

Created attachment 406244 [details]
WIP patch

[Peng Liu]

Created attachment 406245 [details]
WIP patch (fix build failures)

[Peng Liu]

Created attachment 406303 [details]
WIP patch v2

[Peng Liu]

Created attachment 406309 [details]
WIP patch v2 (fix macOS build failures)

[Peng Liu]

Created attachment 406438 [details]
Patch

[Peng Liu]

Created attachment 406450 [details]
patch - revise change logs

[Jer Noble]

Comment on attachment 406450 [details]
patch - revise change logs

View in context: https://bugs.webkit.org/attachment.cgi?id=406450&action=review

r=me, with only a couple minor nits.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:296
> +void VideoFullscreenModelContext::prepareToExitFullscreen()
> +{
> +    for (auto& client : m_clients)
> +        client->prepareToExitPictureInPicture();
> +}
> +

I realize this is an existing pattern, but we should replace `HashSet<WebCore::VideoFullscreenModelClient*>` with `WeakHashSet<WebCore::VideoFullscreenModelClient>` and update all the call sites. This may be a source of latent bugs, if the m_clients set is mutated while calling callbacks. This can be cleaned up in a future patch though.

> Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.cpp:143
> +    auto* currentPlaybackControlsElement = m_page->playbackSessionManager().currentPlaybackControlsElement();
> +    if (currentPlaybackControlsElement)

Nit: could be:

```
if (auto* currentPlaybackControlsElement = m_page->playbackSessionManager().currentPlaybackControlsElement())
...
```

[Peng Liu]

Created attachment 406465 [details]
patch for landing

[Peng Liu]

Comment on attachment 406450 [details]
patch - revise change logs

View in context: https://bugs.webkit.org/attachment.cgi?id=406450&action=review

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:296
>> +
> 
> I realize this is an existing pattern, but we should replace `HashSet<WebCore::VideoFullscreenModelClient*>` with `WeakHashSet<WebCore::VideoFullscreenModelClient>` and update all the call sites. This may be a source of latent bugs, if the m_clients set is mutated while calling callbacks. This can be cleaned up in a future patch though.

Good point! Filed Bug 215420 to track that.

>> Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.cpp:143
>> +    if (currentPlaybackControlsElement)
> 
> Nit: could be:
> 
> ```
> if (auto* currentPlaybackControlsElement = m_page->playbackSessionManager().currentPlaybackControlsElement())
> ...
> ```

Fixed.

[EWS]

Committed r265562: <https://trac.webkit.org/changeset/265562>

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