Summary: | Set WebVideoFullscreenInterfaceMac up as a client of WebPlaybackSessionInterfaceMac to listen for playback state changes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ada Chan <adachan> | ||||||||
Component: | Media | Assignee: | Ada Chan <adachan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric.carlson, jer.noble | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ada Chan
2016-04-25 17:27:18 PDT
Related to <rdar://problem/25840183> and <rdar://problem/25841037> Created attachment 277298 [details]
Patch
Need to make sure WebPlaybackSessionModelMediaElement's media element is set in WebVideoFullscreenManager::enterVideoFullscreenForVideoElement(). Updated patch coming. Created attachment 277328 [details]
Patch
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. 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]
Patch
Committed: http://trac.webkit.org/changeset/200157 |