[webkit-reviews] review granted: [Bug 215305] Add the support to return to element fullscreen from picture-in-picture : [Attachment 406450] patch - revise change logs

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 215305: Add the support to return to element fullscreen from
picture-in-picture
https://bugs.webkit.org/show_bug.cgi?id=215305

Attachment 406450: patch - revise change logs

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




--- Comment #8 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 406450
  --> https://bugs.webkit.org/attachment.cgi?id=406450
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())
...
```