[webkit-reviews] review granted: [Bug 202615] [Picture-in-Picture Web API] Implement PictureInPictureWindow : [Attachment 382156] 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 382156: Patch

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




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

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

This is much better and really close, but I think the test should be filled out
a bit.

r=me with a more complete test.

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

Nit: this should reference the bug you filed about fixing this.

> LayoutTests/media/picture-in-picture-api-pip-events.html:19
> +	       run('video.src = findMediaFile("video", "content/test")');
> +	       await waitFor(video, 'canplaythrough');
> +
> +	       run('video.play()');
> +	       await waitFor(video, 'playing');
> +
> +	       runWithKeyDown(function() { video.requestPictureInPicture(); });
> +	       await waitFor(video, 'enterpictureinpicture');

This is fine as far as it goes, but you should test the other steps in the
request Picture-in-Picture algorithm: throw if the state is HAVE_NOTHING, throw
if there is no video track, throw if not triggered by a user gesture, check
that document.pictureInPictureElement is set correctly, check that the
'enterpictureinpicture' event's pictureInPictureWindow attribute is set.

> LayoutTests/media/picture-in-picture-api-pip-events.html:22
> +	       runWithKeyDown(function() { document.exitPictureInPicture(); });
> +	       await waitFor(video, 'leavepictureinpicture');

This should check everything mandated by the exit Picture-in-Picture algorithm.