Allow media element to supply the video layer after fullscreen transition has already begun.
Created attachment 226110 [details] Patch
Comment on attachment 226110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:758 > + void MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenGravity(MediaPlayer::VideoGravity gravity) Nit: you don't need this indentation. > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:135 > + exitFullscreenForNode(m_node.get()); "exitFullscreenForNode" called from didEnterFullscreen? > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:156 > + enterFullscreenForNode(m_node.get()); "enterFullscreenForNode" called from didExitFullscreen? > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:166 > + Nit: extra blank line.
Comment on attachment 226110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review > Source/WebCore/ChangeLog:35 > + Stash the gravity propety, pass it along to the media player if possible. property* > Source/WebCore/html/HTMLMediaElement.h:800 > + RetainPtr<PlatformLayer> m_videoFullscreenLayer; Should these variables be moved after the flags? > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:501 > + { Brace on same line as "if" >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:758 >> + void MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenGravity(MediaPlayer::VideoGravity gravity) > > Nit: you don't need this indentation. Remove excess indentation > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:764 > + NSString* videoGravity = AVLayerVideoGravityResizeAspect; NSString *videoGravity > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:768 > + videoGravity = AVLayerVideoGravityResize; It seems this might be slightly better: switch (gravity) { case VideoGravityResizeAspectFill: videoGravity = AVLayerVideoGravityResizeAspectFill; break; … default: ASSERT_NOT_REACHED(); } only in that any additional modes would trigger an assert. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:201 > + _avPlayerController = (WebAVPlayerController*)playerController; missing space: (WebAVPlayerController *)playerController > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:215 > + { There's enough of the consequent clause here to return early to avoid the indentation. > Source/WebCore/platform/ios/WebVideoFullscreenModel.h:45 > + enum VideoGravity { VideoGravityReize, VideoGravityResizeAspect, VideoGravityResizeAspectFill}; Missing space separating last enum from brace > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:94 > + else Our style puts the concluding brace and "else" on the same line. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:176 > + [m_videoFullscreenLayer setFrame:CGRectMake(rect.x(), rect.y(), rect.width(), rect.height())]; You can use CGRect(rect) here. (See FloatRectCG.cpp) > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:186 > + videoGravity = MediaPlayer::VideoGravityResizeAspectFill; same thing here re: switch statement > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:108 > + Remove excess space. > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:113 > + Ditto > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:70 > + return isHTMLVideoElement(node); Would be clearer to write this simply as return Settings::avKitEnabled() && isHTMLVideoElement(node);
<rdar://problem/16207968>
(In reply to comment #2) > (From update of attachment 226110 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:758 > > + void MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenGravity(MediaPlayer::VideoGravity gravity) > > Nit: you don't need this indentation. Done. > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:135 > > + exitFullscreenForNode(m_node.get()); > > "exitFullscreenForNode" called from didEnterFullscreen? Surprising but intended. This handles the case the exitFullScreen was called while animating into fullscreen. > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:156 > > + enterFullscreenForNode(m_node.get()); > > "enterFullscreenForNode" called from didExitFullscreen? Ditto. > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:166 > > + > > Nit: extra blank line. Done.
(In reply to comment #3) > (From update of attachment 226110 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review > > > Source/WebCore/ChangeLog:35 > > + Stash the gravity propety, pass it along to the media player if possible. > > property* Done. > > > Source/WebCore/html/HTMLMediaElement.h:800 > > + RetainPtr<PlatformLayer> m_videoFullscreenLayer; > > Should these variables be moved after the flags? I moved this and moved the initializer to match the order. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:501 > > + { > > Brace on same line as "if" Done. > > >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:758 > >> + void MediaPlayerPrivateAVFoundationObjC::setVideoFullscreenGravity(MediaPlayer::VideoGravity gravity) > > > > Nit: you don't need this indentation. > > Remove excess indentation Done. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:764 > > + NSString* videoGravity = AVLayerVideoGravityResizeAspect; > > NSString *videoGravity Done. > > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:768 > > + videoGravity = AVLayerVideoGravityResize; > > It seems this might be slightly better: > > switch (gravity) { > case VideoGravityResizeAspectFill: > videoGravity = AVLayerVideoGravityResizeAspectFill; > break; > … > default: > ASSERT_NOT_REACHED(); > } Done, using an else clause. > > only in that any additional modes would trigger an assert. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:201 > > + _avPlayerController = (WebAVPlayerController*)playerController; > > missing space: (WebAVPlayerController *)playerController Done. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:215 > > + { > > There's enough of the consequent clause here to return early to avoid the indentation. Done. > > > Source/WebCore/platform/ios/WebVideoFullscreenModel.h:45 > > + enum VideoGravity { VideoGravityReize, VideoGravityResizeAspect, VideoGravityResizeAspectFill}; > > Missing space separating last enum from brace Done. And fixed another occurrence of this in MediaPlayer. And fixed typo VideoGravityResize > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:94 > > + else > > Our style puts the concluding brace and "else" on the same line. Done. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:176 > > + [m_videoFullscreenLayer setFrame:CGRectMake(rect.x(), rect.y(), rect.width(), rect.height())]; > > You can use CGRect(rect) here. (See FloatRectCG.cpp) Done. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:186 > > + videoGravity = MediaPlayer::VideoGravityResizeAspectFill; > > same thing here re: switch statement Done, using an else. Also fixed same pattern in WebAVVideoLayer. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:108 > > + > > Remove excess space. Done. > > > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:113 > > + > > Ditto Ditto. > > > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:70 > > + return isHTMLVideoElement(node); > > Would be clearer to write this simply as > > return Settings::avKitEnabled() && isHTMLVideoElement(node); Done.
Comment on attachment 226110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review >>> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:135 >>> + exitFullscreenForNode(m_node.get()); >> >> "exitFullscreenForNode" called from didEnterFullscreen? > > Surprising but intended. This handles the case the exitFullScreen was called while animating into fullscreen. It probably deserves an explanatory comment, others will probably misunderstand this as well. >>> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:156 >>> + enterFullscreenForNode(m_node.get()); >> >> "enterFullscreenForNode" called from didExitFullscreen? > > Ditto. Ditto.
Created attachment 226144 [details] Patch
r+ (bugzilla is broken)
Test
(In reply to comment #7) > (From update of attachment 226110 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226110&action=review > > >>> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:135 > >>> + exitFullscreenForNode(m_node.get()); > >> > >> "exitFullscreenForNode" called from didEnterFullscreen? > > > > Surprising but intended. This handles the case the exitFullScreen was called while animating into fullscreen. > > It probably deserves an explanatory comment, others will probably misunderstand this as well. > > >>> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:156 > >>> + enterFullscreenForNode(m_node.get()); > >> > >> "enterFullscreenForNode" called from didExitFullscreen? > > > > Ditto. > > Ditto. Added comments.
(In reply to comment #9) > r+ (bugzilla is broken) I'm submitting a rebased version to prevent a merge conflict.
Created attachment 226178 [details] Patch
Comment on attachment 226178 [details] Patch Marking r+ based on Simon's earlier review.
Comment on attachment 226178 [details] Patch Clearing flags on attachment: 226178 Committed r165344: <http://trac.webkit.org/changeset/165344>
All reviewed patches have been landed. Closing bug.