Summary: | Fix issues of the Picture-in-Picture API under stress tests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peng Liu <peng.liu6> | ||||||
Component: | New Bugs | Assignee: | Peng Liu <peng.liu6> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, philipj, sergio, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Peng Liu
2020-05-21 00:35:07 PDT
Also, we need to refactor the layout tests for the Picture-in-Picture API with the Mock VideoPresentation Mode. Created attachment 399941 [details]
Patch
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? 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. Created attachment 399991 [details]
Patch for landing
Committed r262038: <https://trac.webkit.org/changeset/262038> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399991 [details]. |