[webkit-reviews] review granted: [Bug 222158] [Cocoa] Add Experimental MediaSession coordinator : [Attachment 424087] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 222158: [Cocoa] Add Experimental MediaSession coordinator
https://bugs.webkit.org/show_bug.cgi?id=222158

Attachment 424087: Patch

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




--- Comment #20 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 424087
  --> https://bugs.webkit.org/attachment.cgi?id=424087
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."