Bug 229068 - nexttrack and previoustrack MediaSession handlers not working
Summary: nexttrack and previoustrack MediaSession handlers not working
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on: 229096
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-12 20:54 PDT by Jean-Yves Avenard [:jya]
Modified: 2021-08-13 14:17 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.29 KB, patch)
2021-08-12 21:18 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jean-Yves Avenard [:jya] 2021-08-12 20:54:17 PDT
nexttrack and previoustrack MediaSession handlers not working
Comment 1 Jean-Yves Avenard [:jya] 2021-08-12 20:55:38 PDT
rdar://80100092
Comment 2 Jean-Yves Avenard [:jya] 2021-08-12 21:18:13 PDT
Created attachment 435471 [details]
Patch
Comment 3 youenn fablet 2021-08-13 00:07:39 PDT
Comment on attachment 435471 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        used with the MediaRemote backend, which prevents automating the test.

Could we do the following:
- Use MediaSession API to trigger adding handlers for nexttrack.
- Add an Internals.idl API to get the supported commands, which would generate a string. Validate 'nexttrack' is in the commands?

Another approach would be to add a mock backend, which sounds feasible as well.
Comment 4 Jean-Yves Avenard [:jya] 2021-08-13 01:36:35 PDT
Comment on attachment 435471 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        used with the MediaRemote backend, which prevents automating the test.
> 
> Could we do the following:
> - Use MediaSession API to trigger adding handlers for nexttrack.
> - Add an Internals.idl API to get the supported commands, which would generate a string. Validate 'nexttrack' is in the commands?
> 
> Another approach would be to add a mock backend, which sounds feasible as well.

What this bug caused was that if you registered a nexttrack action handler it would have registered the MR previous track message that makes Now Playing display the wrong button. 

This was the only thing wrong. If you had pressed the previous button, it would have correctly called the previoustrack handler. The mapping from MR to MediaSession is correct. 

If we had a test, all we would test is the test itself, there would be no guarantee that if that test pass the right information would reach MR.
Comment 5 youenn fablet 2021-08-13 01:45:40 PDT
> If we had a test, all we would test is the test itself, there would be no
> guarantee that if that test pass the right information would reach MR.

It depends how you implement the Internals API (it would need to get the information from the MR) or how you implement the mock (it would need to replace the session manager nowPlaying manager and then query what the mock is receiving, should not be too difficult with mock in web process, but harder in GPU process).
Comment 6 Jean-Yves Avenard [:jya] 2021-08-13 04:33:42 PDT
I will take this in another bug. The scope to implement such test will be way more difficult than the fix itself
Comment 7 EWS 2021-08-13 04:57:28 PDT
Committed r281013 (240503@main): <https://commits.webkit.org/240503@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435471 [details].
Comment 8 Eric Carlson 2021-08-13 08:50:26 PDT
Comment on attachment 435471 [details]
Patch

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

>>> Source/WebCore/ChangeLog:12
>>> +        used with the MediaRemote backend, which prevents automating the test.
>> 
>> Could we do the following:
>> - Use MediaSession API to trigger adding handlers for nexttrack.
>> - Add an Internals.idl API to get the supported commands, which would generate a string. Validate 'nexttrack' is in the commands?
>> 
>> Another approach would be to add a mock backend, which sounds feasible as well.
> 
> What this bug caused was that if you registered a nexttrack action handler it would have registered the MR previous track message that makes Now Playing display the wrong button. 
> 
> This was the only thing wrong. If you had pressed the previous button, it would have correctly called the previoustrack handler. The mapping from MR to MediaSession is correct. 
> 
> If we had a test, all we would test is the test itself, there would be no guarantee that if that test pass the right information would reach MR.

Making this testable could be as simple as adding `PlatformMediaSessionManager::getSupportedCommands()`, plumbing it down to `RemoteCommandListener`, and exposing that through Internals.idl.
Comment 9 Aakash Jain 2021-08-13 14:09:56 PDT
Sees like this broke windows build
https://build.webkit.org/#/builders/67/builds/4559 failed with 240502@main (r281012)
https://build.webkit.org/#/builders/67/builds/4558 passed with 240501@main (r281011)
Comment 10 WebKit Commit Bot 2021-08-13 14:10:36 PDT
Re-opened since this is blocked by bug 229096
Comment 11 Aakash Jain 2021-08-13 14:17:48 PDT
Commented on wrong bug, please ignore.