This tracks the work required to support the airplay picker on WK2. <rdar://problem/15349859>
Created attachment 227970 [details] Patch
Comment on attachment 227970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227970&action=review r=me on the WebCore changes > Source/WebCore/html/HTMLMediaSession.cpp:213 > - MediaSessionManager::sharedManager().showPlaybackTargetPicker(); > +#if PLATFORM(IOS) > + element.document().frame()->page()->chrome().client().showAirPlayRoutePicker(element.hasVideo()); > +#endif MediaSessionManager::showPlaybackTargetPicker doesn't have any purpose now, it should be removed. > Source/WebCore/page/ChromeClient.h:260 > + virtual void showAirPlayRoutePicker(bool hasVideo) = 0; This name shouldn't be be AirPlay specific, maybe showPlaybackTargetPicker? > Source/WebCore/platform/audio/MediaSessionManager.cpp:280 > + // FIXME: Add implementation. It would be good to have a call to notImplemented() here. > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:76 > +SOFT_LINK_CLASS(MediaPlayer, MPVolumeView) Nit: we usually group all of the SOFT_LINK_CLASS() macros together. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2077 > - m_allowsWirelessVideoPlayback = ![m_avPlayer.get() allowsExternalPlayback]; > + m_allowsWirelessVideoPlayback = [m_avPlayer.get() allowsExternalPlayback]; :-O
thanks for the review. > MediaSessionManager::showPlaybackTargetPicker doesn't have any purpose now, it should be removed. I'll remove it. > > > Source/WebCore/page/ChromeClient.h:260 > > + virtual void showAirPlayRoutePicker(bool hasVideo) = 0; > > This name shouldn't be be AirPlay specific, maybe showPlaybackTargetPicker? Good idea, I'll change it. > > > Source/WebCore/platform/audio/MediaSessionManager.cpp:280 > > + // FIXME: Add implementation. > > It would be good to have a call to notImplemented() here. Will do. > > > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:76 > > +SOFT_LINK_CLASS(MediaPlayer, MPVolumeView) > > Nit: we usually group all of the SOFT_LINK_CLASS() macros together. Ok.
Comment on attachment 227970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227970&action=review >> Source/WebCore/page/ChromeClient.h:260 >> + virtual void showAirPlayRoutePicker(bool hasVideo) = 0; > > This name shouldn't be be AirPlay specific, maybe showPlaybackTargetPicker? Does this need to be pure virtual? Can we provide a stub empty implementation so ports not interested do not need to include it? I would also expect, because this is in PLATFORM(IOS) that it be behind ENABLE(IOS_AIRPLAY) as well. But maybe that ENABLE flag is not as important now. > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:257 > + _airPlayPresenceRoutingController = [[getMPAVRoutingControllerClass() alloc] initWithName:@"WebCore - HTML media element checking for AirPlay route presence"]; I think this should adoptNS(...), otherwise we will leak. > Source/WebKit2/Configurations/WebKit2.xcconfig:33 > +FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator = -lobjc -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CorePDF -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework MediaPlayer -framework OpenGLES -framework MobileAsset -framework WebKit -lMobileGestalt -lassertion_extension; Would it help performance to soft link MediaPlayer framework? > Source/WebKit2/UIProcess/ios/PageClientImplIOS.h:116 > + virtual void showAirPlayRoutePicker(bool hasVideo, const WebCore::IntRect& elementRect); Nit: override. In fact, these should all probably have override but at least add it for the new functions. > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:39 > +#import <MediaPlayer/MPAudioVideoRoutingPopoverController.h> > +#import <MediaPlayer/MPAudioVideoRoutingActionSheet.h> > +#import <MediaPlayer/MPAudioVideoRoutingPopoverController.h> > +#import <MediaPlayer/MPAudioVideoRoutingActionSheet.h> Duplicate includes. > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:47 > + RetainPtr<id<WKFormControl>> _control; This does not appear to be used. Can we remove it? > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:50 > + RetainPtr<MPAVRoutingController> _airPlayPickerRoutingController; > + RetainPtr<MPAudioVideoRoutingPopoverController> _airPlayPopoverController; // iPad > + RetainPtr<MPAudioVideoRoutingActionSheet> _airPlayActionSheet; // iPhone These all have "_airPlay" in the name, but that is redundant considering the class name. Maybe you can drop the prefix, and just have _popoverController, _actionSheet, and _routingController. Much easier to distinguish down below. > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:96 > + [self _dismissAirPlayRoutePickerIPad]; Shouldn't this re-present the picker (-show:fromRect:), instead of dismissing? > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:162 > +#endif // PLATFORM(IOS) Weird double space here. > Source/WebKit/ios/WebCoreSupport/WebChromeClientIOS.h:86 > + virtual void showAirPlayRoutePicker(bool hasVideo) override; If this is stubbed in ChromeClient.h, we don't even need to update WebKit1. Unless we really will need to fix this later, then I would suggest not stubbing it.
> > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:257 > > + _airPlayPresenceRoutingController = [[getMPAVRoutingControllerClass() alloc] initWithName:@"WebCore - HTML media element checking for AirPlay route presence"]; > > I think this should adoptNS(...), otherwise we will leak. OOPS! good catch! > > > Source/WebKit2/Configurations/WebKit2.xcconfig:33 > > +FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator = -lobjc -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CorePDF -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework MediaPlayer -framework OpenGLES -framework MobileAsset -framework WebKit -lMobileGestalt -lassertion_extension; > > Would it help performance to soft link MediaPlayer framework? I don't know. > > > Source/WebKit2/UIProcess/ios/PageClientImplIOS.h:116 > > + virtual void showAirPlayRoutePicker(bool hasVideo, const WebCore::IntRect& elementRect); > > Nit: override. In fact, these should all probably have override but at least add it for the new functions. > I'll fix it. > > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:39 > > +#import <MediaPlayer/MPAudioVideoRoutingPopoverController.h> > > +#import <MediaPlayer/MPAudioVideoRoutingActionSheet.h> > > +#import <MediaPlayer/MPAudioVideoRoutingPopoverController.h> > > +#import <MediaPlayer/MPAudioVideoRoutingActionSheet.h> > > Duplicate includes. > > > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:47 > > + RetainPtr<id<WKFormControl>> _control; > > This does not appear to be used. Can we remove it? > OOPS leftover from cut&paste :-) > These all have "_airPlay" in the name, but that is redundant considering the class name. Maybe you can drop the prefix, and just have _popoverController, _actionSheet, and _routingController. Much easier to distinguish down below. > Sure. > > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:96 > > + [self _dismissAirPlayRoutePickerIPad]; > No, we decided to remove it upon rotation. > Shouldn't this re-present the picker (-show:fromRect:), instead of dismissing? > > > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:162 > > +#endif // PLATFORM(IOS) > > Weird double space here. > > > Source/WebKit/ios/WebCoreSupport/WebChromeClientIOS.h:86 > > + virtual void showAirPlayRoutePicker(bool hasVideo) override; > > If this is stubbed in ChromeClient.h, we don't even need to update WebKit1. Unless we really will need to fix this later, then I would suggest not stubbing it. I'll post the patch for WK1 shortly. thanks for the comments!
Comment on attachment 227970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227970&action=review > Source/WebCore/html/HTMLMediaSession.cpp:212 > + element.document().frame()->page()->chrome().client().showAirPlayRoutePicker(element.hasVideo()); Is it safe to assume the element has a document/frame etc? Can't the JavaScript use the API from a detached element? Or an element into a detached frame? > Source/WebKit2/Configurations/WebKit2.xcconfig:33 > +FRAMEWORK_AND_LIBRARY_LDFLAGS_iphonesimulator = -lobjc -framework CFNetwork -framework CoreFoundation -framework CoreGraphics -framework CorePDF -framework CoreText -framework Foundation -framework GraphicsServices -framework ImageIO -framework UIKit -framework MediaPlayer -framework OpenGLES -framework MobileAsset -framework WebKit -lMobileGestalt -lassertion_extension; Maybe we should soft link? > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.h:34 > +- (void)show:(BOOL)hasVideo fromRect:(CGRect)elementRect; This is a little confusing. For audio, you would call show:NO, which makes it looks like you want to hide the picker. Probably better to used a type enum and do something like showMedia:(typed enum) fromRect:()... > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:51 > + WKContentView* _view; // Weak reference. Would __weak work? > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:65 > + if (_airPlayActionSheet) { You don't need the if(). > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:70 > + if (_airPlayPopoverController) Maybe ditto here. > Source/WebKit2/UIProcess/ios/forms/WKAirPlayRoutePicker.mm:118 > + if (_airPlayPopoverController) > + return; Shouldn't we check for _airPlayPickerRoutingController in show:fromRect: instead? > Source/WebKit2/WebProcess/WebPage/WebPage.h:1129 > + WebCore::IntPoint m_interactionLocation; Maybe m_lastInteractionLocation? > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:307 > + m_interactionLocation = point; I think this should use the roundedAdjustedPoint.
(In reply to comment #6) > (From update of attachment 227970 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227970&action=review > > > Source/WebCore/html/HTMLMediaSession.cpp:212 > > + element.document().frame()->page()->chrome().client().showAirPlayRoutePicker(element.hasVideo()); > > Is it safe to assume the element has a document/frame etc? > Can't the JavaScript use the API from a detached element? Or an element into a detached frame? > I believe the answer is no, but I'd like Eric to comment on this. > This is a little confusing. For audio, you would call show:NO, which makes it looks like you want to hide the picker. > Probably better to used a type enum and do something like showMedia:(typed enum) fromRect:()... > At the call site this looks like show:hasVideo, I think it is ok. I've addressed all your other comments. thanks!
Committed revision 166384.
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 227970 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=227970&action=review > > > > > Source/WebCore/html/HTMLMediaSession.cpp:212 > > > + element.document().frame()->page()->chrome().client().showAirPlayRoutePicker(element.hasVideo()); > > > > Is it safe to assume the element has a document/frame etc? > > Can't the JavaScript use the API from a detached element? Or an element into a detached frame? > > > I believe the answer is no, but I'd like Eric to comment on this. > Oops, it looks like I forgot to save my comment about guarding against a NULL frame! It is safe to assume there is a document because there is an element. showingPlaybackTargetPickerPermitted (called earlier in the method) returns false if there isn't a page, but it is probably worth adding something like ASSERT(element.document().page()).