[webkit-reviews] review granted: [Bug 202616] [Picture-in-Picture Web API] Implement EnterPictureInPictureEvent support : [Attachment 381706] 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 202616: [Picture-in-Picture Web API] Implement EnterPictureInPictureEvent
support
https://bugs.webkit.org/show_bug.cgi?id=202616

Attachment 381706: Patch

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




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

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

>
Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:15
3
> +	   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:16
8
> +	   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())
);