[webkit-reviews] review granted: [Bug 201024] [Picture-in-Picture Web API] Implement HTMLVideoElement.requestPictureInPicture() / Document.exitPictureInPicture() : [Attachment 380464] 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 201024: [Picture-in-Picture Web API] Implement
HTMLVideoElement.requestPictureInPicture() / Document.exitPictureInPicture()
https://bugs.webkit.org/show_bug.cgi?id=201024

Attachment 380464: Patch

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




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

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

r=me with the suggested fixes.

I think this new API *needs* extensive runtime logging. I'm OK with adding that
in a followup patch, but file a bug and add it quickly.

> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.cpp:47
> +    auto element = document.pictureInPictureElement();
> +    if (!element) {

You should also ensure that document.pictureInPictureElement() returns a video
element:

if (!element || !is<HTMLVideoElement>(*element) ) {
    ASSERT(is<HTMLVideoElement>(element))
    ...

> Source/WebCore/Modules/pictureinpicture/DocumentPictureInPicture.h:44
> +    static bool pictureInPictureEnabled(Document&) { return true; }

Ports definitely need a way to disable PiP at runtime. Why not check the
PictureInPictureAPI runtime flag here instead of always returning true?

>
Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:68
> +    if (!videoElement.player() ||
!videoElement.player()->supportsPictureInPicture()) {
> +	   promise->reject(NotSupportedError);
> +	   return;

Nit: all of these rejections should log so it is possible to figure exactly
what caused the failure.

> Source/WebCore/html/HTMLVideoElement.cpp:495
> +	       if (toPresentationMode(mode) ==
HTMLVideoElement::VideoPresentationMode::PictureInPicture &&
toPresentationMode(fullscreenMode()) !=
HTMLVideoElement::VideoPresentationMode::PictureInPicture)
> +		   m_pictureInPictureObserver->didEnterPictureInPicture();
> +	       else if (toPresentationMode(mode) !=
HTMLVideoElement::VideoPresentationMode::PictureInPicture &&
toPresentationMode(fullscreenMode()) ==
HTMLVideoElement::VideoPresentationMode::PictureInPicture)
> +		   m_pictureInPictureObserver->didExitPictureInPicture();

Nit: you might as well cache toPresentationMode() in local variables instead of
calling it multiple times.

> Source/WebCore/platform/Logging.h:85
> +    M(PictureInPictureAPI) \

This is unused, and I think it would be better to use the existing Media
logging channel in any case.

> Source/WebCore/platform/PictureInPictureObserver.h:28
> +#if PLATFORM(IOS_FAMILY) || (PLATFORM(MAC) &&
ENABLE(VIDEO_PRESENTATION_MODE))

Is this the right set of build flags - don't we want "#if
ENABLE(PICTURE_IN_PICTURE_API)"