Bug 222158

<rdar://problem/74508862>

Created attachment 420919 [details] WIP patch

Created attachment 420984 [details] WIP patch

Created attachment 421040 [details] WIP patch

Created attachment 421055 [details] WIP patch

Created attachment 421058 [details] WIP patch

Created attachment 421845 [details] Patch

Comment on attachment 421845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421845&action=review > Source/WebCore/Modules/mediasession/MediaSession.h:108 > + class Observer : public CanMakeWeakPtr<Observer> { There's already a WTF::Observer template class; so this nomenclature might get confusing, but I don't have a better naming suggestion. SessionObserver? I realize it's already MediaSession::Observer as a fully scoped name. > Source/WebCore/Modules/mediasession/MediaSession.idl:46 > + [Conditional=MEDIA_SESSION_COORDINATOR, EnabledBySetting=MediaSessionCoordinator] attribute MediaSessionReadyState readyState; Might be a tab instead of a space here. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:131 > + .action = MediaSessionAction::Seekto, I think this is the wrong action here. Should this be ::Settrack? > Source/WebCore/platform/graphics/PlatformMediaSessionCoordinator.h:52 > + using SeekCompletedCallback = CompletionHandler<void()>; > + virtual void seekTo(double, SeekCompletedCallback&&) = 0; > + > + using PlayCompletedCallback = CompletionHandler<void()>; > + virtual void play(PlayCompletedCallback&&) = 0; > + > + using PauseCompletedCallback = CompletionHandler<void()>; > + virtual void pause(PauseCompletedCallback&&) = 0; > + > + using SetTrackCompletedCallback = CompletionHandler<void()>; > + virtual void setTrack(const String, SetTrackCompletedCallback&&) = 0; I wonder if we want to make these callbacks signal whether the underlying operation was successful or not. Right now (and in the Mock) all these operations succeed with exactly the parameters passed in, but I could see a situation where the Coordinator has a slightly resulting seekTime because the client issued a seek beyond the duration of the resource. Or if coordination fails, we may want to not fire the action handler in response.

Comment on attachment 421845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421845&action=review >> Source/WebCore/Modules/mediasession/MediaSession.h:108 >> + class Observer : public CanMakeWeakPtr<Observer> { > > There's already a WTF::Observer template class; so this nomenclature might get confusing, but I don't have a better naming suggestion. SessionObserver? I realize it's already MediaSession::Observer as a fully scoped name. The cat is out of the bag here: I could at least 15 other nested classes named `Observer` going back to at least 2013 >> Source/WebCore/Modules/mediasession/MediaSession.idl:46 >> + [Conditional=MEDIA_SESSION_COORDINATOR, EnabledBySetting=MediaSessionCoordinator] attribute MediaSessionReadyState readyState; > > Might be a tab instead of a space here. Oops! >> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:131 >> + .action = MediaSessionAction::Seekto, > > I think this is the wrong action here. Should this be ::Settrack? Double oops! >> Source/WebCore/platform/graphics/PlatformMediaSessionCoordinator.h:52 >> + virtual void setTrack(const String, SetTrackCompletedCallback&&) = 0; > > I wonder if we want to make these callbacks signal whether the underlying operation was successful or not. Right now (and in the Mock) all these operations succeed with exactly the parameters passed in, but I could see a situation where the Coordinator has a slightly resulting seekTime because the client issued a seek beyond the duration of the resource. Or if coordination fails, we may want to not fire the action handler in response. That's a great idea. I'm not sure what state we can usefully pass back in the event of a failure, but I will add a `bool success` parameter for now.

Comment on attachment 421845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421845&action=review > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.idl:32 > + undefined seekTo(double time); HTMLMediaElement.fastSeek takes an unrestricted double. Not sure how much this is important but should we consider the same here? > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.idl:35 > + undefined setTrack(DOMString trackIdentifier); Should some of these return promises? play and pause are doing so in HTMLMediaElement. I could see seekTo take one as well, although HTMLMediaElement is event based there. Not sure about setTrack. If part of its processing is asynchronous, maybe it is worth being able writing something like: await coordinator.setTrack(1); await coordinator.seekTo(0.001);

Created attachment 423401 [details] Patch

Comment on attachment 423401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423401&action=review > Source/WebCore/ChangeLog:10 > + Please fill out the change log. > Source/WebCore/Modules/mediasession/MediaMetadataInit.h:62 > +#define DECODE(name, type) \ > + Optional<type> name; \ > + decoder >> name; \ > + if (!name) \ > + return WTF::nullopt; \ I don't think this makes sense here. If this is generally useful, we should add it to a central place, but we generally prefer to have this spelled out everywhere. > Source/WebCore/Modules/mediasession/MediaSession.idl:34 > +#if defined(ENABLE_MEDIA_SESSION_COORDINATOR) && ENABLE_MEDIA_SESSION_COORDINATOR > + : EventTarget > +#endif This will be wrong when the runtime feature for media session coordinator is disable but the compile time one is enabled. Perhaps we should just bite the bullet and make it always an event target. Otherwise, someone will have to add the way to runtime conditionally make an interhave have one ancestor or another. > Source/WebCore/Modules/mediasession/MediaSession.idl:44 > + [Conditional=MEDIA_SESSION_COORDINATOR, EnabledBySetting=MediaSessionCoordinator] readonly attribute MediaSessionCoordinator? coordinator; Where is this all specified? Is it really adding these to MediaSession proper? Or is it using partials / mixins? If partials and/or mixins, please add new IDL files for those and "includes" as necessary. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:43 > +class MediaSessionCoordinator > + : public RefCounted<MediaSessionCoordinator> > +, public MediaSession::Observer { These should all be on one line. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:5788 > + 072DBC0625DC6EDA00A1350E /* JSMediaSessionCoordinator.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = JSMediaSessionCoordinator.h; path = ../../../../../../WebKitBuild/Debug/DerivedSources/WebCore/JSMediaSessionCoordinator.h; sourceTree = "<group>"; }; > + 072DBC0725DC6EDA00A1350E /* JSMediaSessionCoordinator.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = JSMediaSessionCoordinator.cpp; path = ../../../../../../WebKitBuild/Debug/DerivedSources/WebCore/JSMediaSessionCoordinator.cpp; sourceTree = "<group>"; }; This looks incorrect. > Source/WebCore/testing/Internals.h:1141 > +#endif > + > }; Extra newline. > LayoutTests/TestExpectations:4643 > +media/media-session/mock-coordinator.html [ Skip ] Is this running anywhere then?

Comment on attachment 423401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423401&action=review > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:73 > + m_platformCoordinator->seekTo(time, [time, this, protectedThis = makeRefPtr(*this), identifier = identifier, promise = WTFMove(promise)] (Optional<Exception>&& exception) mutable { Nit. Can we s/identifier = identifier/identifier/g here? > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:74 > + Nit. This empty line can be removed. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:107 > + m_platformCoordinator->play([this, protectedThis = makeRefPtr(*this), identifier = identifier, promise = WTFMove(promise)] (Optional<Exception>&& exception) mutable { Ditto. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:139 > + m_platformCoordinator->pause([this, protectedThis = makeRefPtr(*this), identifier = identifier, promise = WTFMove(promise)] (Optional<Exception>&& exception) mutable { Ditto. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:171 > + m_platformCoordinator->setTrack(track, [track, this, protectedThis = makeRefPtr(*this), identifier = identifier, promise = WTFMove(promise)] (Optional<Exception>&& exception) mutable { Ditto.

Comment on attachment 423401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423401&action=review > Source/WebCore/Modules/mediasession/MediaMetadataInit.h:73 > + *title, WTFMove(*title) > Source/WebCore/Modules/mediasession/MediaMetadataInit.h:74 > + *artist, ditto. > Source/WebCore/Modules/mediasession/MediaMetadataInit.h:75 > + *album, ditto. > Source/WebCore/Modules/mediasession/MediaMetadataInit.h:77 > + *trackIdentifier, ditto. > Source/WebCore/Modules/mediasession/MediaMetadataInit.h:79 > + *artwork ditto. > Source/WebCore/testing/Internals.cpp:6077 > + m_mockMediaSessionCoordinator = &mock.get(); mock.ptr() > Source/WebCore/testing/MockMediaSessionCoordinator.h:44 > + MockMediaSessionCoordinator(ScriptExecutionContext&, RefPtr<StringCallback>&&); Why is this public? > Source/WebCore/testing/MockMediaSessionCoordinator.h:45 > + virtual ~MockMediaSessionCoordinator() = default; Why do we need this? > Source/WebCore/testing/MockMediaSessionCoordinator.h:50 > + nit: extra line

Created attachment 424070 [details] Patch

Created attachment 424077 [details] Patch

Created attachment 424079 [details] Patch

Created attachment 424084 [details] Patch

Created attachment 424087 [details] Patch

Comment on attachment 424087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424087&action=review r=me with some minor nits and notes. Overall, what seems to be missing is a client interface for PlatformMediaSessionCoordinator to allow a channel for that object to communicate "upwards" to the DOM, but this looks good as it is. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:81 > + protectedThis->logger().error(protectedThis->logChannel(), identifier, "coordinator.seekTo failed!"); I think that if you pass in `this` as well as `protectedThis` into the lambda, you can just use the standard ERROR_LOG macro here. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:115 > + protectedThis->logger().error(protectedThis->logChannel(), identifier, "coordinator.play failed!"); Ditto here. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:125 > + // FIXME: should the promise reject if there isn't a registered 'play' action handler? > + promise.resolve(); If the session has a default action to play() the active media element, this is probably fine. > Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:192 > + // FIXME: should the promise reject if there isn't a registered 'setTrack' action handler? > + promise.resolve(); This on the other hand, probably should reject. The proposed spec language could be something like: "If the Coordinator's Media Session does not have an action handler or a default action for "setTrack", reject the promise with an InvalidStateError."

Created attachment 424149 [details] Patch

Created attachment 424159 [details] Patch

Comment on attachment 421845 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421845&action=review >> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.idl:32 >> + undefined seekTo(double time); > > HTMLMediaElement.fastSeek takes an unrestricted double. Not sure how much this is important but should we consider the same here? Good point, fixed. >> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.idl:35 >> + undefined setTrack(DOMString trackIdentifier); > > Should some of these return promises? > play and pause are doing so in HTMLMediaElement. > I could see seekTo take one as well, although HTMLMediaElement is event based there. > > Not sure about setTrack. If part of its processing is asynchronous, maybe it is worth being able writing something like: > await coordinator.setTrack(1); > await coordinator.seekTo(0.001); Great idea, changed!

Comment on attachment 423401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423401&action=review >> Source/WebCore/ChangeLog:10 >> + > > Please fill out the change log. Fixed. >> Source/WebCore/Modules/mediasession/MediaMetadataInit.h:62 >> + return WTF::nullopt; \ > > I don't think this makes sense here. If this is generally useful, we should add it to a central place, but we generally prefer to have this spelled out everywhere. OK, changed. >> Source/WebCore/Modules/mediasession/MediaSession.idl:34 >> +#endif > > This will be wrong when the runtime feature for media session coordinator is disable but the compile time one is enabled. > > Perhaps we should just bite the bullet and make it always an event target. > > Otherwise, someone will have to add the way to runtime conditionally make an interhave have one ancestor or another. Good point, I changed it to always be an event target. >> Source/WebCore/Modules/mediasession/MediaSession.idl:44 >> + [Conditional=MEDIA_SESSION_COORDINATOR, EnabledBySetting=MediaSessionCoordinator] readonly attribute MediaSessionCoordinator? coordinator; > > Where is this all specified? Is it really adding these to MediaSession proper? Or is it using partials / mixins? If partials and/or mixins, please add new IDL files for those and "includes" as necessary. I split them out into mixins. >> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.h:43 >> +, public MediaSession::Observer { > > These should all be on one line. Fixed >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5788 >> + 072DBC0725DC6EDA00A1350E /* JSMediaSessionCoordinator.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; name = JSMediaSessionCoordinator.cpp; path = ../../../../../../WebKitBuild/Debug/DerivedSources/WebCore/JSMediaSessionCoordinator.cpp; sourceTree = "<group>"; }; > > This looks incorrect. Just Xcode being "helpful". Fixed. >> Source/WebCore/testing/Internals.h:1141 >> }; > > Extra newline. Removed >> LayoutTests/TestExpectations:4643 >> +media/media-session/mock-coordinator.html [ Skip ] > > Is this running anywhere then? Yes, on platforms where the feature is enabled.

Comment on attachment 423401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423401&action=review >> Source/WebCore/Modules/mediasession/MediaMetadataInit.h:73 >> + *title, > > WTFMove(*title) All fixed, thanks. >> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:73 >> + m_platformCoordinator->seekTo(time, [time, this, protectedThis = makeRefPtr(*this), identifier = identifier, promise = WTFMove(promise)] (Optional<Exception>&& exception) mutable { > > Nit. Can we s/identifier = identifier/identifier/g here? All fixed, thanks. >> Source/WebCore/testing/Internals.cpp:6077 >> + m_mockMediaSessionCoordinator = &mock.get(); > > mock.ptr() Changed. >> Source/WebCore/testing/MockMediaSessionCoordinator.h:44 >> + MockMediaSessionCoordinator(ScriptExecutionContext&, RefPtr<StringCallback>&&); > > Why is this public? It doesn't need to be, fixed. >> Source/WebCore/testing/MockMediaSessionCoordinator.h:45 >> + virtual ~MockMediaSessionCoordinator() = default; > > Why do we need this? Removed. >> Source/WebCore/testing/MockMediaSessionCoordinator.h:50 >> + > > nit: extra line Removed.

Comment on attachment 424087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424087&action=review >> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:125 >> + promise.resolve(); > > If the session has a default action to play() the active media element, this is probably fine. I'll fix these in a followup because it will require more changes to trigger a default action.

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Created attachment 424188 [details] Patch

Committed r274983: <https://commits.webkit.org/r274983> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424188 [details].