[webkit-reviews] review granted: [Bug 212191] Fix issues of the Picture-in-Picture API under stress tests : [Attachment 399941] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Peng Liu
<peng.liu6 at apple.com>'s request for review:
Bug 212191: Fix issues of the Picture-in-Picture API under stress tests
https://bugs.webkit.org/show_bug.cgi?id=212191

Attachment 399941: Patch

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




--- Comment #4 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 399941
  --> https://bugs.webkit.org/attachment.cgi?id=399941
Patch

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

> Source/WebCore/html/HTMLVideoElement.cpp:458
> +static inline HTMLVideoElement::VideoPresentationMode
toPresentationMode(HTMLMediaElementEnums::VideoFullscreenMode mode)
> +{
> +    if (mode == HTMLMediaElementEnums::VideoFullscreenModeStandard)
> +	   return HTMLVideoElement::VideoPresentationMode::Fullscreen;
> +
> +    if (mode & HTMLMediaElementEnums::VideoFullscreenModePictureInPicture)
> +	   return HTMLVideoElement::VideoPresentationMode::PictureInPicture;
> +
> +    if (mode == HTMLMediaElementEnums::VideoFullscreenModeNone)
> +	   return HTMLVideoElement::VideoPresentationMode::Inline;
> +
> +    ASSERT_NOT_REACHED();
> +    return HTMLVideoElement::VideoPresentationMode::Inline;
> +}

Do we need both enums? VideoFullscreenMode is a bitfield, but it doesn't look
like it needs to be so it would be nice to just use VideoPresentationMode if
possible. 

If that works, it is likely to be a big change so another patch is fine.

> Source/WebCore/html/HTMLVideoElement.cpp:472
> +#if ENABLE(PICTURE_IN_PICTURE_API)

Do we still need ENABLE_PICTURE_IN_PICTURE_API?

> Source/WebCore/html/HTMLVideoElement.cpp:474
> +		   m_exitingPictureInPicture = true;

Since you can't be entering and exiting PiP at the same time (I hope!), can you
replace m_enteringPictureInPicture and m_exitingPictureInPicture with a single
enum?