Bug 215305 - Add the support to return to element fullscreen from picture-in-picture
Summary: Add the support to return to element fullscreen from picture-in-picture
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: 218419
  Show dependency tree / graph
 
Reported: 2020-08-07 21:38 PDT by Peng Liu
Modified: 2020-11-06 14:26 PST (History)
13 users (show)

See Also:

Attachments
WIP patch (34.23 KB, patch)
2020-08-07 21:42 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP patch (fix build failures) (34.14 KB, patch)
2020-08-07 22:02 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP patch v2 (40.77 KB, patch)
2020-08-10 09:44 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
WIP patch v2 (fix macOS build failures) (40.80 KB, patch)
2020-08-10 10:27 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
Patch (52.70 KB, patch)
2020-08-11 17:11 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
patch - revise change logs (53.01 KB, patch)
2020-08-12 00:08 PDT, Peng Liu 		jer.noble: review+
Details | Formatted Diff | Diff
patch for landing (52.97 KB, patch)
2020-08-12 11:21 PDT, Peng Liu 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) 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-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].