Bug 133366 - send external playback properties to fullscreen.
Summary: send external playback properties to fullscreen.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-28 17:10 PDT by Jeremy Jones
Modified: 2014-10-21 23:30 PDT (History)
11 users (show)

See Also:


Attachments
Patch (18.72 KB, patch)
2014-05-28 17:27 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (18.10 KB, patch)
2014-05-29 11:00 PDT, Jeremy Jones
simon.fraser: review+
Details | Formatted Diff | Diff
Patch for landing (22.13 KB, patch)
2014-05-30 15:51 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2014-05-28 17:10:54 PDT
send external playback properties to fullscreen.
Comment 1 Jeremy Jones 2014-05-28 17:27:12 PDT
Created attachment 232224 [details]
Patch
Comment 2 Jeremy Jones 2014-05-28 17:28:58 PDT
radar://16733415
Comment 3 Eric Carlson 2014-05-28 18:12:03 PDT
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.
Comment 4 Jeremy Jones 2014-05-29 11:00:51 PDT
Created attachment 232254 [details]
Patch
Comment 5 Jeremy Jones 2014-05-29 11:01:51 PDT
smfr, please review the WK2 parts.
Comment 6 Simon Fraser (smfr) 2014-05-29 11:40:15 PDT
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.
Comment 7 Jeremy Jones 2014-05-30 14:58:34 PDT
(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()
Comment 8 Jeremy Jones 2014-05-30 15:51:03 PDT
Created attachment 232307 [details]
Patch for landing
Comment 9 Jeremy Jones 2014-05-30 17:11:05 PDT
(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 10 Jeremy Jones 2014-05-30 18:52:16 PDT
Comment on attachment 232307 [details]
Patch for landing

Attempting to cancel the commit.
Comment 11 WebKit Commit Bot 2014-06-02 14:13:43 PDT
Comment on attachment 232307 [details]
Patch for landing

Clearing flags on attachment: 232307

Committed r169545: <http://trac.webkit.org/changeset/169545>