[webkit-reviews] review granted: [Bug 226328] MediaSession.coordinator should not be optional, relying on coordinator state change instead : [Attachment 430006] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 28 09:16:58 PDT 2021


Eric Carlson <eric.carlson at apple.com> has granted Jean-Yves Avenard [:jya]
<jya at apple.com>'s request for review:
Bug 226328: MediaSession.coordinator should not be optional, relying on
coordinator state change instead
https://bugs.webkit.org/show_bug.cgi?id=226328

Attachment 430006: Patch

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




--- Comment #18 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 430006
  --> https://bugs.webkit.org/attachment.cgi?id=430006
Patch

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

> Source/WebCore/Modules/mediasession/MediaSession.cpp:203
> +    m_coordinator->setMediaSession(this);

This shouldn't be necessary as you pass the session to
`MediaSessionCoordinator::create`.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:59
> +MediaSessionCoordinator::MediaSessionCoordinator(MediaSession& session,
RefPtr<MediaSessionCoordinatorPrivate>&& privateCoordinator)
> +    : ActiveDOMObject(session.scriptExecutionContext())

The only use of `session` here is to get the scriptExecutionContext, so you
could either just pass that directly or also call `setMediaSession()` and skip
the explicit call from MediaSession::createCoordinator.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:71
> +    if (privateCoordinator) {
> +	   m_privateCoordinator = WTFMove(privateCoordinator);
> +	   m_privateCoordinator->setLogger(m_logger.copyRef(),
m_logIdentifier);
> +	   m_privateCoordinator->setClient(makeWeakPtr(this));
> +	   coordinatorStateChanged(MediaSessionCoordinatorState::Waiting);
> +    }

I would put all of this logic in `setMediaSessionCoordinatorPrivate` and call
it from here.

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:93
> +    return m_hasCoordinatorsStateChangeEventListener;

I don't think we should fire events if there is no MediaSession (see below).

> Source/WebCore/Modules/mediasession/MediaSessionCoordinator.cpp:420
> +    if (m_hasCoordinatorsStateChangeEventListener)

I don't think there is any reason to fire an event if there is no MediaSession,
so this should also check `m_session`. Also, you might put the test into a
method and also call it from `virtualHasPendingActivity` so we're sure they use
the same logic if something changes.


More information about the webkit-reviews mailing list