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

Attachment 381986: Patch

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




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

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

>
Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:10
1
> +	   auto pictureInPictureWindow =
videoElementPictureInPicture->m_pictureInPictureWindow;
> +	  
promise->resolve<IDLInterface<PictureInPictureWindow>>(*pictureInPictureWindow)
;

Nit: I'm not sure the local variable adds anything.

>
Source/WebCore/Modules/pictureinpicture/HTMLVideoElementPictureInPicture.cpp:16
9
>      INFO_LOG(LOGIDENTIFIER);
> +    m_pictureInPictureWindow->close();
>      m_videoElement.document().setPictureInPictureElement(nullptr);

Step 2 of the "exit Picture-in-Picture algorithm" is "run the close window
algorithm", and the "close window algorithm" says you must:

    ... set the window state is set to closed.
    ... set the width attribute to 0.
    ... set the height attribute to 0.
    ... if the size of the Picture-in-Picture window changes, queue a task to
fire a 'resize' event

So if this is called when the window has not already been set to { 0, 0 }, you
need to fire a 'resize' event.

> Source/WebCore/Modules/pictureinpicture/PictureInPictureWindow.cpp:58
> +    m_size.setWidth(0);
> +    m_size.setHeight(0);

Nit: I think you can simplify this to something like "m_size = { 0, 0 }"

> Source/WebCore/html/HTMLVideoElement.cpp:532
> +    // fullscreenMode() does not always provide the correct fullscreen mode
> +    // when mode changing is happening

Nit: it would be good to have a bug filed for the real fix for this problem so
we don't lose track of it. Please include the bug number here.