Bug 159580 – Refactor WebPlaybackSessionModelMediaElement to be client based.

• Component: Media
• Status: RESOLVED FIXED

[Jeremy Jones]

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.

[Jer Noble]

Created attachment 283223 [details]
Patch

[Jer Noble]

Created attachment 283227 [details]
Patch

[Jer Noble]

Created attachment 287287 [details]
Patch

[Eric Carlson]

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.

[Jer Noble]

Created attachment 287796 [details]
Patch for landing

[Jer Noble]

Committed r205365: <http://trac.webkit.org/changeset/205365>

[Jon Lee]

rdar://problem/28020157