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.
Related to <rdar://problem/25840183> and <rdar://problem/25841037>
Created attachment 277298 [details]
Need to make sure WebPlaybackSessionModelMediaElement's media element is set in WebVideoFullscreenManager::enterVideoFullscreenForVideoElement(). Updated patch coming.
Created attachment 277328 [details]
Comment on attachment 277328 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=277328&action=review
> + 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.
> + float playbackRate();
Having a getter here is very "model"-y. I don't think this is necessary.
> +void WebVideoFullscreenInterfaceMac::playbackRateDidChange()
> #if USE(APPLE_INTERNAL_SDK)
> - [videoFullscreenInterfaceObjC() setRate:isPlaying ? playbackRate : 0.];
> + [videoFullscreenInterfaceObjC() updatePlaybackRate];
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.
> + 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.
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
Created attachment 277420 [details]