Bug 202616

Summary:

[Picture-in-Picture Web API] Implement EnterPictureInPictureEvent support

Product:

WebKit

Reporter:

Peng Liu <peng.liu6>

Component:

Media

Assignee:

Peng Liu <peng.liu6>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cadubentzen, calvaris, cdumez, commit-queue, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, peng.liu6, philipj, sergio, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

Safari 12

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

182688, 202615

Attachments:

Description	Flags
Patch	none
Update the patch to fix build errors on iOS, GTK and WPE	none
Try to fix build failures on GTK and WPE.	none
Patch	eric.carlson: review+
Patch	none

Peng Liu

Reported 2019-10-05 22:24:11 PDT

Implement EnterPictureInPictureEvent support.

Attachments
Patch (14.91 KB, patch) 2019-10-19 17:42 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Update the patch to fix build errors on iOS, GTK and WPE (14.95 KB, patch) 2019-10-19 23:57 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Try to fix build failures on GTK and WPE. (14.98 KB, patch) 2019-10-20 10:00 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Patch (16.05 KB, patch) 2019-10-23 11:33 PDT, Peng Liu	eric.carlson: review+	Details Formatted Diff Diff
Patch (15.90 KB, patch) 2019-10-23 15:49 PDT, Peng Liu	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-10-14 15:54:11 PDT

<rdar://problem/56267916>

Peng Liu

Comment 2 2019-10-19 17:42:19 PDT

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

Comment 3 2019-10-19 23:57:38 PDT

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

Peng Liu

Comment 4 2019-10-20 10:00:56 PDT

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

Peng Liu

Comment 5 2019-10-23 11:33:57 PDT

Created attachment 381706 [details] Patch Update the patch to fix API test failures

Eric Carlson

Comment 6 2019-10-23 15:18:42 PDT

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 7 2019-10-23 15:48:16 PDT

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

Comment 8 2019-10-23 15:49:14 PDT

Created attachment 381746 [details] Patch Patch for landing

WebKit Commit Bot

Comment 9 2019-10-23 23:34:14 PDT

Comment on attachment 381746 [details] Patch Clearing flags on attachment: 381746 Committed r251530: <https://trac.webkit.org/changeset/251530>

Note You need to log in before you can comment on or make changes to this bug.