Bug 223435 – Move management of RemoteCommandListener from MediaSessionManagerCocoa into NowPlayingManager

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jean-Yves Avenard [:jya]]

sub-task of bug 223298

The NowPlayingManager already does almost the same work as done by MediaSessionManagerCocoa: 
1- Setup and control RemoteCommandListener
2- Allow to set NowPlayingInfo to RemoteMedia instance.

It does 2) via calling back the MediaSessionManagerCocoa::setNowPlayingInfo/clearPlayingInfo static method.

NowPlayingManager is currently only used in the GPU process and acts as a proxy to MediaSessionManagerCocoa in the WebContent process.

The flow currently being:

with GPU process:
MediaSessionManagerCocoa -> WebPlatformStrategies -> XPC to GPU -> GPUConnectionToWebProcess -> NowPlayingInfoManager -> MediaSessionManagerCocoa(static) -> MediaRemote
without GPU process:
MediaSessionManagerCocoa -> WebPlatformStrategies -> MediaSessionManagerCocoa(static) -> MediaRemote

in bug 223298 we want to add special handling so that the MediaSessionManagerCocoa can set the NowPlayingInfo's artwork without having to always send the image.

By doing:
MediaSessionManagerCocoa -> NowPlayingInfoManager -> WebPlatformStrategies [-> XPC to GPU -> GPUConnectionToWebProcess] -> NowPlayingInfoManager -> MediaRemote

We can simplify and centralise the code so that NowPlayingInfoManager will be responsible in calling the web strategies and the caching at both end of the XPC channel.

[Radar WebKit Bug Importer]

<rdar://problem/75567198>

[Jean-Yves Avenard [:jya]]

Created attachment 423809 [details]
Patch

[Jean-Yves Avenard [:jya]]

Created attachment 423814 [details]
Patch

[Eric Carlson]

Comment on attachment 423814 [details]
Patch

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

> Source/WebCore/platform/NowPlayingManager.cpp:65
> +    m_setAsNowPlayingApplication = false;

Do we need both `m_nowPlayingInfo` and `m_setAsNowPlayingApplication` now - couldn't we clear `m_nowPlayingInfo` here and check `!m_nowPlayingInfo` where we currently check `m_setAsNowPlayingApplication`?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:390
> +    // FIXME: Eric, why is this call needed? This will reset the seek information from the one set in setNowPlayingInfo
>      updateSupportedRemoteCommands();

It is needed if we are just switched to a different element, which may have a different set of supported commands.

[Jean-Yves Avenard [:jya]]

Comment on attachment 423814 [details]
Patch

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

>> Source/WebCore/platform/NowPlayingManager.cpp:65
>> +    m_setAsNowPlayingApplication = false;
> 
> Do we need both `m_nowPlayingInfo` and `m_setAsNowPlayingApplication` now - couldn't we clear `m_nowPlayingInfo` here and check `!m_nowPlayingInfo` where we currently check `m_setAsNowPlayingApplication`?

Not with the logic as-is

m_setAsNowPlayingApplication is only set after the first call to setNowPlayingInfoPrivate, if we used to just check if m_nowPlayingInfo is set, then we would always pass true to MediaSessionManagerCocoa::setNowPlayingInfo

And we need to set m_nowPlayingInfo before calling it as in bug 223298 as the parameter is const.

So while we could probably combined the two members, I do feel like it would make the code harder to read. As such, I prefer to leave it as-is.

[Jean-Yves Avenard [:jya]]

Created attachment 423960 [details]
Patch

remove FIXME question:

[EWS]

Committed r274829: <https://commits.webkit.org/r274829>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423960 [details].