[webkit-reviews] review granted: [Bug 224278] Media Session action should default to the MediaElement's default when no MediaSession handler are set : [Attachment 425605] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Jean-Yves Avenard [:jya]
<jya at apple.com>'s request for review:
Bug 224278: Media Session action should default to the MediaElement's default
when no MediaSession handler are set
https://bugs.webkit.org/show_bug.cgi?id=224278

Attachment 425605: Patch

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




--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 425605
  --> https://bugs.webkit.org/attachment.cgi?id=425605
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.