send external playback properties to fullscreen.
Created attachment 232224 [details] Patch
radar://16733415
Comment on attachment 232224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232224&action=review This looks fine to me, but I am not a WK2 reviewer so you will need get someone else to do the formal r+ > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:677 > +void WebVideoFullscreenInterfaceAVKit::setExternalPlayback(bool enabled, String type, String name) Nit: something like "localizedDeviceName" would be a better name. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:125 > + || *eventType == eventNames().pauseEvent > + || *eventType == eventNames().playEvent > + || *eventType == eventNames().ratechangeEvent) Nit: breaking this line and not the others is odd.
Created attachment 232254 [details] Patch
smfr, please review the WK2 parts.
Comment on attachment 232254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232254&action=review > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:683 > + AVPlayerControllerExternalPlaybackType externalPlaybackType = AVPlayerControllerExternalPlaybackTypeNone; > + if (type == "airplay") > + externalPlaybackType = AVPlayerControllerExternalPlaybackTypeAirPlay; > + else if (type == "tvout") > + externalPlaybackType = AVPlayerControllerExternalPlaybackTypeTVOut; Can't we use an enum for these types? > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:686 > + playerController().externalPlaybackAirPlayDeviceLocalizedName = localizedDeviceName; > + playerController().externalPlaybackType = externalPlaybackType; Why do you set these if enabled == false? > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.h:54 > + void updateState(const WTF::AtomicString*); Unclear what the AtomicString represents. States are usually enum values. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:82 > + static NeverDestroyed<Vector<AtomicString>> events; > + > + if (!events.get().size()) { > + events.get().append(eventNames().durationchangeEvent); > + events.get().append(eventNames().pauseEvent); > + events.get().append(eventNames().playEvent); > + events.get().append(eventNames().ratechangeEvent); > + events.get().append(eventNames().timeupdateEvent); > + events.get().append(eventNames().addtrackEvent); > + events.get().append(eventNames().removetrackEvent); > + events.get().append(eventNames().webkitcurrentplaybacktargetiswirelesschangedEvent); > + } You should move this into a getter that inits the static the first time. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:99 > + updateState(nullptr); Very unclear what this means. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:114 > +void WebVideoFullscreenModelMediaElement::updateState(const WTF::AtomicString* eventType) This needs to be called updateFromEvent() or something.
(In reply to comment #6) > (From update of attachment 232254 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232254&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:683 > > + AVPlayerControllerExternalPlaybackType externalPlaybackType = AVPlayerControllerExternalPlaybackTypeNone; > > + if (type == "airplay") > > + externalPlaybackType = AVPlayerControllerExternalPlaybackTypeAirPlay; > > + else if (type == "tvout") > > + externalPlaybackType = AVPlayerControllerExternalPlaybackTypeTVOut; > > Can't we use an enum for these types? Sure. > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:686 > > + playerController().externalPlaybackAirPlayDeviceLocalizedName = localizedDeviceName; > > + playerController().externalPlaybackType = externalPlaybackType; > > Why do you set these if enabled == false? Prevent hysteresis of the properties. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.h:54 > > + void updateState(const WTF::AtomicString*); > > Unclear what the AtomicString represents. States are usually enum values. renamed to updateForEventName() > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:82 > > + static NeverDestroyed<Vector<AtomicString>> events; > > + > > + if (!events.get().size()) { > > + events.get().append(eventNames().durationchangeEvent); > > + events.get().append(eventNames().pauseEvent); > > + events.get().append(eventNames().playEvent); > > + events.get().append(eventNames().ratechangeEvent); > > + events.get().append(eventNames().timeupdateEvent); > > + events.get().append(eventNames().addtrackEvent); > > + events.get().append(eventNames().removetrackEvent); > > + events.get().append(eventNames().webkitcurrentplaybacktargetiswirelesschangedEvent); > > + } > > You should move this into a getter that inits the static the first time. done. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:99 > > + updateState(nullptr); > > Very unclear what this means. now uses a specific "allEvents" name. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:114 > > +void WebVideoFullscreenModelMediaElement::updateState(const WTF::AtomicString* eventType) > > This needs to be called updateFromEvent() or something. ok: updateForEventName()
Created attachment 232307 [details] Patch for landing
(In reply to comment #3) > (From update of attachment 232224 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232224&action=review > > This looks fine to me, but I am not a WK2 reviewer so you will need get someone else to do the formal r+ > > > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:677 > > +void WebVideoFullscreenInterfaceAVKit::setExternalPlayback(bool enabled, String type, String name) > > Nit: something like "localizedDeviceName" would be a better name. done. > > > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:125 > > + || *eventType == eventNames().pauseEvent > > + || *eventType == eventNames().playEvent > > + || *eventType == eventNames().ratechangeEvent) > > Nit: breaking this line and not the others is odd. done.
Comment on attachment 232307 [details] Patch for landing Attempting to cancel the commit.
Comment on attachment 232307 [details] Patch for landing Clearing flags on attachment: 232307 Committed r169545: <http://trac.webkit.org/changeset/169545>