Bug 215305

Summary:

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

Product:

WebKit

Reporter:

Peng Liu <peng.liu6>

Component:

Media

Assignee:

Peng Liu <peng.liu6>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kangil.han, philipj, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

218419

Attachments:

Description	Flags
WIP patch	none
WIP patch (fix build failures)	none
WIP patch v2	none
WIP patch v2 (fix macOS build failures)	none
Patch	none
patch - revise change logs	jer.noble: review+
patch for landing	none

Description Peng Liu 2020-08-07 21:38:47 PDT

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

Comment 1 Peng Liu 2020-08-07 21:39:41 PDT

<rdar://problem/66424852>

Comment 2 Peng Liu 2020-08-07 21:42:50 PDT

Created attachment 406244 [details]
WIP patch

Comment 3 Peng Liu 2020-08-07 22:02:20 PDT

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

Comment 4 Peng Liu 2020-08-10 09:44:37 PDT

Created attachment 406303 [details]
WIP patch v2

Comment 5 Peng Liu 2020-08-10 10:27:41 PDT

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

Comment 6 Peng Liu 2020-08-11 17:11:04 PDT

Created attachment 406438 [details]
Patch

Comment 7 Peng Liu 2020-08-12 00:08:25 PDT

Created attachment 406450 [details]
patch - revise change logs

Comment 8 Jer Noble 2020-08-12 10:41:41 PDT

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())
...
```

Comment 9 Peng Liu 2020-08-12 11:21:31 PDT

Created attachment 406465 [details]
patch for landing

Comment 10 Peng Liu 2020-08-12 11:39:07 PDT

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.

Comment 11 EWS 2020-08-12 13:55:40 PDT

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

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