Bug 157008 - Set WebVideoFullscreenInterfaceMac up as a client of WebPlaybackSessionInterfaceMac to listen for playback state changes
Summary: Set WebVideoFullscreenInterfaceMac up as a client of WebPlaybackSessionInterf...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ada Chan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-04-25 17:27 PDT by Ada Chan
Modified: 2016-04-27 16:26 PDT (History)
2 users (show)

See Also:


Attachments
Patch (16.34 KB, patch)
2016-04-25 17:57 PDT, Ada Chan
no flags Details | Formatted Diff | Diff
Patch (17.35 KB, patch)
2016-04-25 21:21 PDT, Ada Chan
no flags Details | Formatted Diff | Diff
Patch (17.07 KB, patch)
2016-04-26 16:34 PDT, Ada Chan
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ada Chan 2016-04-25 17:27:18 PDT
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.
Comment 1 Ada Chan 2016-04-25 17:29:02 PDT
Related to <rdar://problem/25840183> and <rdar://problem/25841037>
Comment 2 Ada Chan 2016-04-25 17:57:04 PDT
Created attachment 277298 [details]
Patch
Comment 3 Ada Chan 2016-04-25 21:18:54 PDT
Need to make sure WebPlaybackSessionModelMediaElement's media element is set in WebVideoFullscreenManager::enterVideoFullscreenForVideoElement().  Updated patch coming.
Comment 4 Ada Chan 2016-04-25 21:21:40 PDT
Created attachment 277328 [details]
Patch
Comment 5 Jer Noble 2016-04-26 10:45:32 PDT
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.
Comment 6 Ada Chan 2016-04-26 16:19:16 PDT
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
Comment 7 Ada Chan 2016-04-26 16:34:17 PDT
Created attachment 277420 [details]
Patch
Comment 8 Ada Chan 2016-04-27 16:26:34 PDT
Committed:
http://trac.webkit.org/changeset/200157