Bug 224278 – Media Session action should default to the MediaElement's default when no MediaSession handler are set

• Component: Media
• Status: RESOLVED FIXED

[Jean-Yves Avenard [:jya]]

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)

[Radar WebKit Bug Importer]

<rdar://problem/76339841>

[Jean-Yves Avenard [:jya]]

Created attachment 425388 [details]
wip

[youenn fablet]

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?

[Eric Carlson]

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"?

[Jean-Yves Avenard [:jya]]

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.

[Jean-Yves Avenard [:jya]]

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

[Jean-Yves Avenard [:jya]]

Created attachment 425605 [details]
Patch

[youenn fablet]

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.

[Jean-Yves Avenard [:jya]]

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.

[Jean-Yves Avenard [:jya]]

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

[Jean-Yves Avenard [:jya]]

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

[Jean-Yves Avenard [:jya]]

Created attachment 425619 [details]
Patch

Apply review comments

[youenn fablet]

(In reply to Jean-Yves Avenard [:jya] from comment #12)
> Created attachment 425619 [details]
> Patch
> 
> Apply review comments

LGTM

[youenn fablet]

> 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.

[youenn fablet]

> > 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

[EWS]

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].