Bug 224278 - Media Session action should default to the MediaElement's default when no MediaSession handler are set
Summary: Media Session action should default to the MediaElement's default when no Med...
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:
Blocks:
 
Reported: 2021-04-07 05:51 PDT by Jean-Yves Avenard [:jya]
Modified: 2021-04-09 17:45 PDT (History)
13 users (show)

See Also:


Attachments
wip (9.09 KB, patch)
2021-04-07 06:08 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (42.00 KB, patch)
2021-04-09 04:06 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff
Patch (40.74 KB, patch)
2021-04-09 08:24 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-04-07 05:51:20 PDT
Currently, when no media session handler are set; the media session coordinator actions will be ignored.

We should default to the HTMLMediaElement default action (play/pause/seek etc)
Comment 1 Radar WebKit Bug Importer 2021-04-07 05:59:22 PDT
<rdar://problem/76339841>
Comment 2 Jean-Yves Avenard [:jya] 2021-04-07 06:08:15 PDT
Created attachment 425388 [details]
wip
Comment 3 youenn fablet 2021-04-07 06:32:55 PDT
Comment on attachment 425388 [details]
wip

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

> Source/WebCore/ChangeLog:7
> +

Do we have a way to test these changes?

> Source/WebCore/Modules/mediasession/MediaSession.cpp:261
> +    default:

We do not tend to add default so that, if there is a new enumeration value, we will have to fix it.

> Source/WebCore/Modules/mediasession/MediaSession.cpp:266
> +        return;

Maybe we could return in the enumeration for better clarity.
It seems this will only happen in MediaSessionAction::Settrack and Skipad cases.

> Source/WebCore/Modules/mediasession/MediaSession.cpp:378
> +    return nullptr;

Why not return HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(*doc, MediaElementSession::PlaybackControlsPurpose::NowPlaying) directly?

> Source/WebCore/Modules/mediasession/MediaSession.h:74
> +    RefPtr<HTMLMediaElement> activeMediaElement() const;

Does it need to be public?

> Source/WebCore/html/HTMLMediaElement.cpp:594
> +RefPtr<HTMLMediaElement> HTMLMediaElement::bestMediaElementForShowingPlaybackControlsManager(Document* document, MediaElementSession::PlaybackControlsPurpose purpose)

const Document*?
Maybe we could just have one single bestMediaElementForShowingPlaybackControlsManager taking a const Document* with a default value of nullptr?
Comment 4 Eric Carlson 2021-04-07 10:04:21 PDT
Comment on attachment 425388 [details]
wip

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

>> Source/WebCore/ChangeLog:7
>> +
> 
> Do we have a way to test these changes?

Yes, please add a test.

> Source/WebCore/Modules/mediasession/MediaSession.cpp:226
> +    PlatformMediaSession::RemoteControlCommandType command = PlatformMediaSession::NoCommand;
> +    PlatformMediaSession::RemoteCommandArgument argument;
> +
> +    switch (actionDetails.action) {

I would put this into a static free function like `platformCommandForMediaSessionAction`.

> Source/WebCore/Modules/mediasession/MediaSession.cpp:267
> +    static_cast<PlatformMediaSessionClient*>(element.get())->didReceiveRemoteControlCommand(command, argument);

This static_cast won't be necessary if you make `HTMLMediaElement::didReceiveRemoteControlCommand` public

> Source/WebCore/html/MediaElementSession.cpp:1143
>      if (auto handler = session->handlerForAction(actionDetails.action))
>          handler->handleEvent(actionDetails);
> -    else
> -        ALWAYS_LOG(LOGIDENTIFIER, "Ignoring command, no action handler registered for ", actionDetails.action);
> +    else {
> +        // FIXME: MediaElementSession expected to call the default action if MediaSession is set and no handler for the action are set?
> +        PlatformMediaSession::didReceiveRemoteControlCommand(commandType, argument);
> +    }

If you change this to `session->callActionHandler(...)`, won't this "just work"?
Comment 5 Jean-Yves Avenard [:jya] 2021-04-07 21:15:03 PDT
Comment on attachment 425388 [details]
wip

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

>> Source/WebCore/Modules/mediasession/MediaSession.h:74
>> +    RefPtr<HTMLMediaElement> activeMediaElement() const;
> 
> Does it need to be public?

Not at this stage. But the active media element is a concept you find in the MediaSession spec, so I figured we will need it sooner or later elsewhere.
Comment 6 Jean-Yves Avenard [:jya] 2021-04-09 02:27:16 PDT
Comment on attachment 425388 [details]
wip

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

>>> Source/WebCore/ChangeLog:7
>>> +
>> 
>> Do we have a way to test these changes?
> 
> Yes, please add a test.

that was wip, just to get answers on the questions added in the code. incoming test
Comment 7 Jean-Yves Avenard [:jya] 2021-04-09 04:06:36 PDT
Created attachment 425605 [details]
Patch
Comment 8 youenn fablet 2021-04-09 06:45:11 PDT
Comment on attachment 425605 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSession.cpp:124
> +    return std::pair<PlatformMediaSession::RemoteControlCommandType, PlatformMediaSession::RemoteCommandArgument>(command, argument);

make_pair?

> Source/WebCore/Modules/mediasession/MediaSession.h:74
> +    RefPtr<HTMLMediaElement> activeMediaElement() const;

I would put it as private for now, we can always move it to public later.

> Source/WebCore/html/HTMLMediaElement.h:571
> +    // PlatformMediaSessionClient

Why moving them to public?
In general we prefer private by default and public for those needed.

> Source/WebCore/html/HTMLMediaElement.h:577
> +    void mayResumePlayback(bool shouldResume) override;

If we modify these, we could look at final/override.
It seems these two could be marked final, maybe others as well.

> LayoutTests/media/media-session/default-actionHandlers.html:43
> +        });

Can we add a few more tests, for instance if paused is overriden.
Or if we do setActionHandler("play", null) after having set it with a function.

> LayoutTests/media/media-session/default-actionHandlers.html:45
> +        // Playback shouldn't have started if a handler for this action was defined and it did nothing.

This is not a comment for this patch but to the spec in general.
It might be convenient that the JS handler could decide to be a no-op and let the browser do its default action, whatever it is.
Something similar to preventDefault but the inverse.
Comment 9 Jean-Yves Avenard [:jya] 2021-04-09 07:03:22 PDT
Comment on attachment 425605 [details]
Patch

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

>> Source/WebCore/Modules/mediasession/MediaSession.cpp:124
>> +    return std::pair<PlatformMediaSession::RemoteControlCommandType, PlatformMediaSession::RemoteCommandArgument>(command, argument);
> 
> make_pair?

+1

>> Source/WebCore/html/HTMLMediaElement.h:571
>> +    // PlatformMediaSessionClient
> 
> Why moving them to public?
> In general we prefer private by default and public for those needed.

Actioning Eric's earlier comment to avoid static_cast. I question on why they were made provide in the first case when HTMLMediaElement inherit from public PlatformMediaSessionClientand and all those methods are public in the base class.

>> Source/WebCore/html/HTMLMediaElement.h:577
>> +    void mayResumePlayback(bool shouldResume) override;
> 
> If we modify these, we could look at final/override.
> It seems these two could be marked final, maybe others as well.

I believe they all can be made final.

But this seems out of scope for this patch.

>> LayoutTests/media/media-session/default-actionHandlers.html:45
>> +        // Playback shouldn't have started if a handler for this action was defined and it did nothing.
> 
> This is not a comment for this patch but to the spec in general.
> It might be convenient that the JS handler could decide to be a no-op and let the browser do its default action, whatever it is.
> Something similar to preventDefault but the inverse.

You should open a spec bug with the WG. In particular, the more I look into it, the more I think it's lacking details for edge cases. Like there's no way to determine which media element caused the action handler to be called. So you can't manually run the command play like you would etc.
Comment 10 Jean-Yves Avenard [:jya] 2021-04-09 07:04:36 PDT
Comment on attachment 425605 [details]
Patch

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

>>> Source/WebCore/html/HTMLMediaElement.h:571
>>> +    // PlatformMediaSessionClient
>> 
>> Why moving them to public?
>> In general we prefer private by default and public for those needed.
> 
> Actioning Eric's earlier comment to avoid static_cast. I question on why they were made provide in the first case when HTMLMediaElement inherit from public PlatformMediaSessionClientand and all those methods are public in the base class.

I meant "why they were made private" above
Comment 11 Jean-Yves Avenard [:jya] 2021-04-09 07:05:38 PDT
Comment on attachment 425605 [details]
Patch

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

>>>> Source/WebCore/html/HTMLMediaElement.h:571
>>>> +    // PlatformMediaSessionClient
>>> 
>>> Why moving them to public?
>>> In general we prefer private by default and public for those needed.
>> 
>> Actioning Eric's earlier comment to avoid static_cast. I question on why they were made provide in the first case when HTMLMediaElement inherit from public PlatformMediaSessionClientand and all those methods are public in the base class.
> 
> I meant "why they were made private" above

I could of course could only make the only one used public, I thought it was more consistent to have them all public
Comment 12 Jean-Yves Avenard [:jya] 2021-04-09 08:24:55 PDT
Created attachment 425619 [details]
Patch

Apply review comments
Comment 13 youenn fablet 2021-04-09 09:35:27 PDT
(In reply to Jean-Yves Avenard [:jya] from comment #12)
> Created attachment 425619 [details]
> Patch
> 
> Apply review comments

LGTM
Comment 14 youenn fablet 2021-04-09 09:36:29 PDT
> I could of course could only make the only one used public, I thought it was
> more consistent to have them all public

I believe this is what is preferred these days.
Comment 15 youenn fablet 2021-04-09 09:44:58 PDT
> > This is not a comment for this patch but to the spec in general.
> > It might be convenient that the JS handler could decide to be a no-op and let the browser do its default action, whatever it is.
> > Something similar to preventDefault but the inverse.
> 
> You should open a spec bug with the WG. In particular, the more I look into
> it, the more I think it's lacking details for edge cases. Like there's no
> way to determine which media element caused the action handler to be called.
> So you can't manually run the command play like you would etc.

I filed https://github.com/w3c/mediasession/issues/268
Comment 16 EWS 2021-04-09 17:45:45 PDT
Committed r275787 (236358@main): <https://commits.webkit.org/236358@main>

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