Bug 157008 – Set WebVideoFullscreenInterfaceMac up as a client of WebPlaybackSessionInterfaceMac to listen for playback state changes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ada Chan]

Right now both WebVideoFullscreenInterfaceMac and WebPlaybackSessionInterfaceMac keep track of the playback rate separately. Since WebVideoFullscreenInterfaceMac holds a reference to WebPlaybackSessionInterfaceMac anyway, it should just get its playback rate from WebPlaybackSessionInterfaceMac.

Also, since WebVideoFullscreenInterfaceMac holds onto WebPlaybackSessionInterfaceMac, we can't let WebPlaybackSessionManagerProxy unregister that WebPlaybackSessionInterfaceMac while WebVideoFullscreenInterfaceMac is still using it. WebVideoFullscreenInterfaceMac should add itself as a client for that WebPlaybackSession context.

[Ada Chan]

Related to <rdar://problem/25840183> and <rdar://problem/25841037>

[Ada Chan]

Created attachment 277298 [details]
Patch

[Ada Chan]

Need to make sure WebPlaybackSessionModelMediaElement's media element is set in WebVideoFullscreenManager::enterVideoFullscreenForVideoElement().  Updated patch coming.

[Ada Chan]

Created attachment 277328 [details]
Patch

[Jer Noble]

Comment on attachment 277328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277328&action=review

> Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.h:47
> +    virtual void playbackRateDidChange() = 0;

this should probably be "void rateChanged(bool isPlaying, float playbackRate)" to match the "setRate(bool, float)" call.  Passing the changed values in the client method would also make the "playbackRate()" getter unnecessary.

> Source/WebCore/platform/mac/WebPlaybackSessionInterfaceMac.h:64
> +    float playbackRate();

Having a getter here is very "model"-y.  I don't think this is necessary.

> Source/WebCore/platform/mac/WebVideoFullscreenInterfaceMac.mm:120
> +void WebVideoFullscreenInterfaceMac::playbackRateDidChange()
> +{
>  #if USE(APPLE_INTERNAL_SDK)
> -    [videoFullscreenInterfaceObjC() setRate:isPlaying ? playbackRate : 0.];
> +    [videoFullscreenInterfaceObjC() updatePlaybackRate];
>  #endif
>  }

If this was "::rateChanged(bool isPlaying, float playbackRate)", then the contents of this method wouldn't need to change.  It's also not obvious that the updatePlaybackRate method will do the correct thing here, since setRate() actually passes in two values, and there is only a getter for the second value. Essentially, you're tying the definition of playbackRate() to the implementation details in the playbackSessionInterface.

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.mm:274
> +    m_playbackSessionManager->ensureModel(contextId).setMediaElement(&videoElement);

I think that it would be better to do this in WebPlaybackSessionManager::createModelAndInterface().  The new media element model would get the videoElement from m_mediaElements.

[Ada Chan]

Thanks for your feedback, Jer.

I'm reworking the patch to address your feedback.  I'll be removing that playbackRate() getter in WebPlaybackSessionInterfaceMac. Since I won't exactly make WebVideoFullscreenInterfaceMac get the playback rate from WebPlaybackSessionInterfaceMac, I'll retitle this bug to:

Set WebVideoFullscreenInterfaceMac up as a client of WebPlaybackSessionInterfaceMac to listen for playback state changes

[Ada Chan]

Created attachment 277420 [details]
Patch

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/200157