Bug 159580 - Refactor WebPlaybackSessionModelMediaElement to be client based.
Summary: Refactor WebPlaybackSessionModelMediaElement to be client based.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-08 13:34 PDT by Jeremy Jones
Modified: 2016-09-28 14:30 PDT (History)
6 users (show)

See Also:


Attachments
Patch (185.13 KB, patch)
2016-07-08 16:46 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (185.16 KB, patch)
2016-07-08 17:19 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (184.91 KB, patch)
2016-08-29 11:24 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (184.92 KB, patch)
2016-09-02 12:11 PDT, Jer Noble
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 2016-07-08 13:34:16 PDT
WebPlaybackSessionModelMediaElement currently only supports a single interface client. This should be refactored so that it supports multiple clients that can each register and unregister for state changes.
Comment 1 Jer Noble 2016-07-08 16:46:45 PDT
Created attachment 283223 [details]
Patch
Comment 2 Jer Noble 2016-07-08 17:19:52 PDT
Created attachment 283227 [details]
Patch
Comment 3 Jer Noble 2016-08-29 11:24:41 PDT
Created attachment 287287 [details]
Patch
Comment 4 Eric Carlson 2016-08-30 14:00:43 PDT
Comment on attachment 287287 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        can have multiple clients, and that the object will both store current values and also notify those clients

Nit: "and that the object" -> "and so the object"

> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:96
> +        for (auto* client : m_clients)

Nit" auto& because m_clients can't have NULL values

> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:111
> +        for (auto* client : m_clients)

Ditto.

> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:122
> +        for (auto* client : m_clients) {

Ditto.

> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:125
> +            // FIXME: 130788 - find a better event to update seekable ranges from.

Nit: "update seekable ranges from" what?

> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:143
> +        for (auto* client : m_clients) {

Nit: auto&

> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:281
> +    for (auto& client : m_clients) {

Nit: "auto" if you don't decide to change the ones above.

> Source/WebCore/platform/cocoa/WebPlaybackSessionModelMediaElement.mm:393
> +    auto host = m_mediaElement->mediaControlsHost();

Nit: if you move this above the test you can avoid calling the method twice.

> Source/WebCore/platform/cocoa/WebVideoFullscreenModelVideoElement.mm:99
> +        setHasVideo(!!m_videoElement);

Nit: "!!" shouldn't be necessary as setHasVideo takes a bool, not a BOOL

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:749
> +    m_playbackModel->setMediaElement(m_videoElement.get());

Nit: not ".get()" necessary if you use the "videoElement" parameter.

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:757
> +    m_fullscreenModel->setVideoElement(m_videoElement.get());
> +
> +    bool allowsPictureInPicture = m_videoElement->mediaSession().allowsPictureInPicture(*m_videoElement.get());
> +
> +    IntRect videoElementClientRect = elementRectInWindow(m_videoElement.get());

Ditto.
Comment 5 Jer Noble 2016-09-02 12:11:28 PDT
Created attachment 287796 [details]
Patch for landing
Comment 6 Jer Noble 2016-09-02 13:08:52 PDT
Committed r205365: <http://trac.webkit.org/changeset/205365>
Comment 7 Jon Lee 2016-09-28 14:30:47 PDT
rdar://problem/28020157