Bug 217963 - [media-session] Basic support for MediaSession.setPositionState() and MediaSession.setActionHandler()
Summary: [media-session] Basic support for MediaSession.setPositionState() and MediaSe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-20 09:36 PDT by Jer Noble
Modified: 2020-10-20 15:41 PDT (History)
10 users (show)

See Also:


Attachments
Patch (22.99 KB, patch)
2020-10-20 09:44 PDT, Jer Noble
eric.carlson: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (23.01 KB, patch)
2020-10-20 10:20 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (23.44 KB, patch)
2020-10-20 10:44 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2020-10-20 09:36:44 PDT
[media-session] Basic support for MediaSession.setPositionState() and MediaSession.setActionHandler()
Comment 1 Radar WebKit Bug Importer 2020-10-20 09:43:29 PDT
<rdar://problem/70488664>
Comment 2 Jer Noble 2020-10-20 09:44:05 PDT
Created attachment 411875 [details]
Patch
Comment 3 Eric Carlson 2020-10-20 10:01:23 PDT
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.
Comment 4 Jer Noble 2020-10-20 10:16:21 PDT
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.
Comment 5 Peng Liu 2020-10-20 10:17:58 PDT
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.
Comment 6 Jer Noble 2020-10-20 10:20:55 PDT
Created attachment 411880 [details]
Patch for landing
Comment 7 Jer Noble 2020-10-20 10:44:21 PDT
(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.
Comment 8 Jer Noble 2020-10-20 10:44:33 PDT
Created attachment 411886 [details]
Patch for landing
Comment 9 EWS 2020-10-20 15:41:51 PDT
Committed r268769: <https://trac.webkit.org/changeset/268769>

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