Bug 217963 – [media-session] Basic support for MediaSession.setPositionState() and MediaSession.setActionHandler()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[media-session] Basic support for MediaSession.setPositionState() and MediaSession.setActionHandler()

[Radar WebKit Bug Importer]

<rdar://problem/70488664>

[Jer Noble]

Created attachment 411875 [details]
Patch

[Eric Carlson]

Comment on attachment 411875 [details]
Patch

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

> Source/WebCore/Modules/mediasession/MediaSession.cpp:100
>          && std::isfinite(state->playbackRate)

Only infinite rate is valid?

> Source/WebCore/Modules/mediasession/MediaSession.cpp:105
> +        m_positionState = WTFMove(state);
> +        m_lastReportedPosition = m_positionState->position;
> +        m_timeAtLastPositionUpdate = MonotonicTime::now();
>          return { };

I think you are missing some braces here, but I think an early return for illegal input would be clearer in any case.

[Jer Noble]

Comment on attachment 411875 [details]
Patch

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

>> Source/WebCore/Modules/mediasession/MediaSession.cpp:100
>>          && std::isfinite(state->playbackRate)
> 
> Only infinite rate is valid?

Only a finite rate is valid.

>> Source/WebCore/Modules/mediasession/MediaSession.cpp:105
>>          return { };
> 
> I think you are missing some braces here, but I think an early return for illegal input would be clearer in any case.

Oof. Somehow my braces got removed.  The logic here got reversed due to an earlier review by Darin, but I could reverse the logic here with a `!(...)` and switch to an early exception.

[Peng Liu]

Comment on attachment 411875 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +

An extra empty line?

> LayoutTests/media/media-session/mock-actionHandlers.html:35
> +        run('internals.sendMediaSessionAction(navigator.mediaSession, {action: "play"})');

Nit. Maybe it is better to check whether 'internals' is available at the beginning of the test?

> LayoutTests/media/media-session/mock-currentPosition.html:10
> +		testDOMException('internals.currentMediaSessionPosition(navigator.mediaSession)', "DOMException.INVALID_STATE_ERR");

Ditto.

[Jer Noble]

Created attachment 411880 [details]
Patch for landing

[Jer Noble]

(In reply to Peng Liu from comment #5)
> Comment on attachment 411875 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411875&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +
> 
> An extra empty line?

Removed.

> > LayoutTests/media/media-session/mock-actionHandlers.html:35
> > +        run('internals.sendMediaSessionAction(navigator.mediaSession, {action: "play"})');
> 
> Nit. Maybe it is better to check whether 'internals' is available at the
> beginning of the test?
> 
> > LayoutTests/media/media-session/mock-currentPosition.html:10
> > +		testDOMException('internals.currentMediaSessionPosition(navigator.mediaSession)', "DOMException.INVALID_STATE_ERR");
> 
> Ditto.

Sure for both.

[Jer Noble]

Created attachment 411886 [details]
Patch for landing

[EWS]

Committed r268769: <https://trac.webkit.org/changeset/268769>

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