Bug 212191 – Fix issues of the Picture-in-Picture API under stress tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Peng Liu]

Fix issues of the Picture-in-Picture API under stress test

[Peng Liu]

Also, we need to refactor the layout tests for the Picture-in-Picture API with the Mock VideoPresentation Mode.

[Peng Liu]

Created attachment 399941 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/63483308>

[Eric Carlson]

Comment on attachment 399941 [details]
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?

[Peng Liu]

Comment on attachment 399941 [details]
Patch

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

>> Source/WebCore/html/HTMLVideoElement.cpp:458
>> +}
> 
> 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.

Filed a bug webkit.org/b/212229 for it.

>> Source/WebCore/html/HTMLVideoElement.cpp:472
>> +#if ENABLE(PICTURE_IN_PICTURE_API)
> 
> Do we still need ENABLE_PICTURE_IN_PICTURE_API?

No, the change is for the Picture-in-Picture feature in general, not only for the API. So I will remove it.

>> 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?

Make sense! Will fix it.

[Peng Liu]

Created attachment 399991 [details]
Patch for landing

[EWS]

Committed r262038: <https://trac.webkit.org/changeset/262038>

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