Bug 223435

Summary: Move management of RemoteCommandListener from MediaSessionManagerCocoa into NowPlayingManager
Product: WebKit Reporter: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Component: MediaAssignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 223298    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Jean-Yves Avenard [:jya] 2021-03-18 03:37:27 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-03-18 03:37:49 PDT
<rdar://problem/75567198>
Comment 2 Jean-Yves Avenard [:jya] 2021-03-20 03:41:08 PDT
Created attachment 423809 [details]
Patch
Comment 3 Jean-Yves Avenard [:jya] 2021-03-20 05:13:42 PDT
Created attachment 423814 [details]
Patch
Comment 4 Eric Carlson 2021-03-22 14:31:15 PDT
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.
Comment 5 Jean-Yves Avenard [:jya] 2021-03-22 16:34:21 PDT
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.
Comment 6 Jean-Yves Avenard [:jya] 2021-03-22 16:45:58 PDT
Created attachment 423960 [details]
Patch

remove FIXME question:
Comment 7 EWS 2021-03-22 17:43:16 PDT
Committed r274829: <https://commits.webkit.org/r274829>

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