Cleanup state variables related to the picture-in-picture implementation
Created attachment 407526 [details] WIP patch
Created attachment 407527 [details] WIP patch - fix build errors
Created attachment 407529 [details] WIP patch - fix build errors v2
Created attachment 407541 [details] WIP - revert a change leading to test failures
Created attachment 407542 [details] WIP - fix an API test failure
Created attachment 407550 [details] WIP - fix some layout test failures
Created attachment 407552 [details] WIP - fix an issue of picture-in-picture support on Mac
Created attachment 407615 [details] WIP - add changelogs
<rdar://problem/68098202>
Comment on attachment 407615 [details] WIP - add changelogs View in context: https://bugs.webkit.org/attachment.cgi?id=407615&action=review I didn’t get to really review, but I did look over the patch and have a few style comments. > Source/WebCore/html/HTMLMediaElement.cpp:6085 > + downcast<HTMLVideoElement>(this)->exitToFullscreenModeWithoutAnimationIfPossible(oldVideoFullscreenMode, VideoFullscreenModeStandard); It would be better like this: downcast<HTMLVideoElement>(*this). Matches the line above. > Source/WebCore/html/HTMLMediaElement.cpp:7914 > + ASSERT(m_videoFullscreenMode != mode); > if (m_videoFullscreenMode == mode) > return; Why is this assertion a good idea? > Source/WebCore/html/HTMLVideoElement.cpp:480 > + VideoFullscreenMode videoFullscreenMode = toFullscreenMode(mode); auto Also, it seems like in the context of this function we should use a shorter name than videoFullscreenMode. Maybe fullscreenMode. > Source/WebCore/html/HTMLVideoElement.cpp:517 > + m_pictureInPictureObserver->didEnterPictureInPicture(IntSize(size)); How did we select "casting to IntSize" over expandedIntSize, flooredIntSize (I think this does the same thing casting with better handling for edge cases), and roundedIntSize? > Source/WebCore/html/HTMLVideoElement.h:85 > enum class VideoPresentationMode { Inline, Fullscreen, PictureInPicture}; Missing space before the "}". > Source/WebCore/html/HTMLVideoElement.h:99 > + // HTMLMediaElement I don’t think this comment is valuable. > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1125 > + WebAVPictureInPicturePlayerLayerView *pipView = (WebAVPictureInPicturePlayerLayerView *)[m_playerLayerView pictureInPicturePlayerLayerView]; auto > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1126 > + WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer]; auto > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1455 > + WebAVPictureInPicturePlayerLayerView *pipView = (WebAVPictureInPicturePlayerLayerView *)[m_playerLayerView pictureInPicturePlayerLayerView]; auto > Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1456 > + WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer]; auto > Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm:258 > + // FIXME(rdar://problem/42250952) This is not a great comment for people who can’t follow a rdar: link. Can we write words here? > Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:601 > + didEnterFullscreen(contextId, { m_mockPictureInPictureWindowSize.width(), m_mockPictureInPictureWindowSize.height() }); Why do we have to write out separate width and height in braces instead of just m_mockPictureInPictureWindowSize? > Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:781 > - m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId)); > + if (size.isEmpty()) > + m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, WTF::nullopt)); > + else > + m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, size)); I’d rather use a local variable rather than repeating everything twice: Optional<FloatSize> optionalSize; if (!size.isEmpty()) optionalSize = size; m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, optionalSize)); > Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:449 > - videoElement->didBecomeFullscreenElement(); > + if (size) > + videoElement->didEnterFullscreenOrPictureInPicture(size.value()); > + else > + videoElement->didEnterFullscreenOrPictureInPicture({ }); There’s a function called Optional::valueOr that we should use in case like this so this can be one line of code instead of four.
Created attachment 407745 [details] WIP - revise the patch according to Darin's comments and fix some issues found in stress test
Comment on attachment 407745 [details] WIP - revise the patch according to Darin's comments and fix some issues found in stress test View in context: https://bugs.webkit.org/attachment.cgi?id=407745&action=review > Source/WebCore/ChangeLog:23 > + the request. The motivation to do that is to guarantee correct events to be fired. s/guarantee correct events to be fired/guarantee the correct events are fired/ > Source/WebCore/ChangeLog:25 > + have a good way to create regression tests for the scenarios that presentation mode change s/scenarios that presentation mode change/scenarios where presentation mode change/
Created attachment 407785 [details] Revise the patch according to Eric's comments and fix some issues found in stress tests
Created attachment 407786 [details] Revise the patch according to Eric's comments and fix some issues found in stress tests
Comment on attachment 407615 [details] WIP - add changelogs View in context: https://bugs.webkit.org/attachment.cgi?id=407615&action=review Thanks for the review! I have revised the patch accordingly. >> Source/WebCore/html/HTMLMediaElement.cpp:6085 >> + downcast<HTMLVideoElement>(this)->exitToFullscreenModeWithoutAnimationIfPossible(oldVideoFullscreenMode, VideoFullscreenModeStandard); > > It would be better like this: > > downcast<HTMLVideoElement>(*this). > > Matches the line above. Fixed. >> Source/WebCore/html/HTMLMediaElement.cpp:7914 >> return; > > Why is this assertion a good idea? Actually, both the assertion and checking need to be removed. HTMLMediaElement::setFullscreenMode is a private function and it expects callers to do the checking. >> Source/WebCore/html/HTMLVideoElement.cpp:480 >> + VideoFullscreenMode videoFullscreenMode = toFullscreenMode(mode); > > auto > > Also, it seems like in the context of this function we should use a shorter name than videoFullscreenMode. Maybe fullscreenMode. Make sense. But "fullscreenMode" is a function name of HTMLMediaElement. >> Source/WebCore/html/HTMLVideoElement.cpp:517 >> + m_pictureInPictureObserver->didEnterPictureInPicture(IntSize(size)); > > How did we select "casting to IntSize" over expandedIntSize, flooredIntSize (I think this does the same thing casting with better handling for edge cases), and roundedIntSize? The size will be used by JS code, and an error/offset of 1 is not a big issue. However, I think using flooredIntSize here is better. >> Source/WebCore/html/HTMLVideoElement.h:85 >> enum class VideoPresentationMode { Inline, Fullscreen, PictureInPicture}; > > Missing space before the "}". Good catch! Fixed. >> Source/WebCore/html/HTMLVideoElement.h:99 >> + // HTMLMediaElement > > I don’t think this comment is valuable. Agree. Removed. >> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1125 >> + WebAVPictureInPicturePlayerLayerView *pipView = (WebAVPictureInPicturePlayerLayerView *)[m_playerLayerView pictureInPicturePlayerLayerView]; > > auto Fixed. >> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1126 >> + WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer]; > > auto Fixed. >> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1455 >> + WebAVPictureInPicturePlayerLayerView *pipView = (WebAVPictureInPicturePlayerLayerView *)[m_playerLayerView pictureInPicturePlayerLayerView]; > > auto Fixed. >> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1456 >> + WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer]; > > auto Fixed. >> Source/WebCore/platform/mac/VideoFullscreenInterfaceMac.mm:258 >> + // FIXME(rdar://problem/42250952) > > This is not a great comment for people who can’t follow a rdar: link. Can we write words here? Agree. Updated the comment. >> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:601 >> + didEnterFullscreen(contextId, { m_mockPictureInPictureWindowSize.width(), m_mockPictureInPictureWindowSize.height() }); > > Why do we have to write out separate width and height in braces instead of just m_mockPictureInPictureWindowSize? It is a mistake. Fixed. >> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:781 >> + m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, size)); > > I’d rather use a local variable rather than repeating everything twice: > > Optional<FloatSize> optionalSize; > if (!size.isEmpty()) > optionalSize = size; > m_page->send(Messages::VideoFullscreenManager::DidEnterFullscreen(contextId, optionalSize)); Good idea! Fixed. >> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:449 >> + videoElement->didEnterFullscreenOrPictureInPicture({ }); > > There’s a function called Optional::valueOr that we should use in case like this so this can be one line of code instead of four. Great idea! Fixed. Thanks!
Comment on attachment 407745 [details] WIP - revise the patch according to Darin's comments and fix some issues found in stress test View in context: https://bugs.webkit.org/attachment.cgi?id=407745&action=review >> Source/WebCore/ChangeLog:23 >> + the request. The motivation to do that is to guarantee correct events to be fired. > > s/guarantee correct events to be fired/guarantee the correct events are fired/ Fixed. >> Source/WebCore/ChangeLog:25 >> + have a good way to create regression tests for the scenarios that presentation mode change > > s/scenarios that presentation mode change/scenarios where presentation mode change/ Fixed.
*** Bug 191629 has been marked as a duplicate of this bug. ***
Committed r266728: <https://trac.webkit.org/changeset/266728> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407786 [details].