Bug 202616 – [Picture-in-Picture Web API] Implement EnterPictureInPictureEvent support

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

Implement EnterPictureInPictureEvent support.

[Radar WebKit Bug Importer]

<rdar://problem/56267916>

[Peng Liu]

Created attachment 381379 [details]
Patch

With this patch, the enterpictureinpicture event is fired after the transition is completed and the picture-in-picture window size information is available. But, the leavepictureinpicture event is fired before the transition is completed.

[Peng Liu]

Created attachment 381391 [details]
Update the patch to fix build errors on iOS, GTK and WPE

[Peng Liu]

Created attachment 381394 [details]
Try to fix build failures on GTK and WPE.

[Peng Liu]

Created attachment 381706 [details]
Patch

Update the patch to fix API test failures

[Eric Carlson]

Comment on attachment 381706 [details]
Patch

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

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:153
> +        EnterPictureInPictureEvent::Init init;
> +        init.bubbles = true;
> +        init.pictureInPictureWindow = PictureInPictureWindow::create(m_videoElement.document(), windowSize.width(), windowSize.height());

Nit: I think "initializer" would be slightly better variable name.

> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:168
> +        auto leavePictureInPictureEvent = Event::create(eventNames().leavepictureinpictureEvent, Event::CanBubble::Yes, Event::IsCancelable::No);
> +        m_videoElement.scheduleEvent(WTFMove(leavePictureInPictureEvent));

Nit: this could be one line.

> Source/WebCore/html/HTMLVideoElement.cpp:537
> +        if (m_waitingForPictureInPictureWindowFrame) {
> +            m_waitingForPictureInPictureWindowFrame = false;
> +            if (m_pictureInPictureObserver)
> +                m_pictureInPictureObserver->didEnterPictureInPicture(IntSize(frame.size()));
> +        } else {
> +            if (m_pictureInPictureObserver)
> +                m_pictureInPictureObserver->pictureInPictureWindowResized(IntSize(frame.size()));
> +        }

Nit: WebKit style guidelines prefer early return when possible, and I think that would make the slightly easier to read:

if (toPresentationMode(fullscreenMode()) != VideoPresentationMode::PictureInPicture)
    return;

if (m_waitingForPictureInPictureWindowFrame) {
    m_waitingForPictureInPictureWindowFrame = false;
     if (m_pictureInPictureObserver)
         m_pictureInPictureObserver->didEnterPictureInPicture(IntSize(frame.size()));

    return;
}

if (m_pictureInPictureObserver)
    m_pictureInPictureObserver->pictureInPictureWindowResized(IntSize(frame.size()));

[Peng Liu]

Comment on attachment 381706 [details]
Patch

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

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:153
>> +        init.pictureInPictureWindow = PictureInPictureWindow::create(m_videoElement.document(), windowSize.width(), windowSize.height());
> 
> Nit: I think "initializer" would be slightly better variable name.

Agree.

>> Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:168
>> +        m_videoElement.scheduleEvent(WTFMove(leavePictureInPictureEvent));
> 
> Nit: this could be one line.

Right, and the same for the enterPictureInPictureEvent.

>> Source/WebCore/html/HTMLVideoElement.cpp:537
>> +        }
> 
> Nit: WebKit style guidelines prefer early return when possible, and I think that would make the slightly easier to read:
> 
> if (toPresentationMode(fullscreenMode()) != VideoPresentationMode::PictureInPicture)
>     return;
> 
> if (m_waitingForPictureInPictureWindowFrame) {
>     m_waitingForPictureInPictureWindowFrame = false;
>      if (m_pictureInPictureObserver)
>          m_pictureInPictureObserver->didEnterPictureInPicture(IntSize(frame.size()));
> 
>     return;
> }
> 
> if (m_pictureInPictureObserver)
>     m_pictureInPictureObserver->pictureInPictureWindowResized(IntSize(frame.size()));

Agree.

[Peng Liu]

Created attachment 381746 [details]
Patch

Patch for landing

[WebKit Commit Bot]

Comment on attachment 381746 [details]
Patch

Clearing flags on attachment: 381746

Committed r251530: <https://trac.webkit.org/changeset/251530>