[webkit-reviews] review granted: [Bug 220660] PiP video subtitles stop updating when Safari is backgrounded : [Attachment 418018] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 220660: PiP video subtitles stop updating when Safari is backgrounded
https://bugs.webkit.org/show_bug.cgi?id=220660

Attachment 418018: Patch

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




--- Comment #11 from Darin Adler <darin at apple.com> ---
Comment on attachment 418018
  --> https://bugs.webkit.org/attachment.cgi?id=418018
Patch

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

> Source/WebKit/ChangeLog:11
> +	   Subtitles in the picture-in-picture window will stop updating when
the browser is
> +	   in the background because we freeze the layer tree when a browser is
in the background.
> +	   This patch fixes this issue by avoiding freezing the layer tree if a
video is playing
> +	   in picture-in-picture when the browser is in the background.

Can we construct a regression test?

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.h:121
> +    bool videoInPictureInPicture() const;

This sounds like a function that would return a "video", not a function that
would return a boolean answer to a question.

Given the comment you wrote above, it seems that this function’s name should be
hasVideoPlayingInPictureInPicture().

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:274
> +    if (mode == HTMLMediaElementEnums::VideoFullscreenModePictureInPicture)
> +	   m_videoElementInPictureInPicture = makeWeakPtr(videoElement);

Seems like an omission that if the mode is something else, we don’t null out
m_videoElementInPictureInPicture. Do we have an iron clad guarantee it will be
null in that case? Could we maybe just set to null? Or is there some chance
that we can have a PiP lingering while we start some other full-screen, perhaps
on a different element?