| Summary: | [media-session] Basic support for MediaSession.setPositionState() and MediaSession.setActionHandler() | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||
| Component: | New Bugs | Assignee: | Jer Noble <jer.noble> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, peng.liu6, philipj, sergio, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Jer Noble
2020-10-20 09:36:44 PDT
Created attachment 411875 [details]
Patch
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 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 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. Created attachment 411880 [details]
Patch for landing
(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. Created attachment 411886 [details]
Patch for landing
Committed r268769: <https://trac.webkit.org/changeset/268769> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411886 [details]. |