[webkit-reviews] review granted: [Bug 137218] Implement API in WebCore::Page that returns whether there's any audio playing on the page : [Attachment 239179] Patch: Implement WebCore::Page::isPlayingAudio()

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Ada Chan
<adachan at apple.com>'s request for review:
Bug 137218: Implement API in WebCore::Page that returns whether there's any
audio playing on the page
https://bugs.webkit.org/show_bug.cgi?id=137218

Attachment 239179: Patch: Implement WebCore::Page::isPlayingAudio()
https://bugs.webkit.org/attachment.cgi?id=239179&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239179&action=review


> Source/WebCore/dom/Document.cpp:3293
> +    for (auto mediaSessionsIt = m_mediaSessions.begin(); mediaSessionsIt !=
m_mediaSessions.end(); ++mediaSessionsIt) {

Can you do this with a modern for loop?   for (auto mediaSession :
m_mediaSessions)

> Source/WebCore/html/HTMLMediaElement.cpp:447
> +    document.registerMediaSession(*m_mediaSession);

I think it would make more sense to have the session register/unregister with
the document. Maybe something like MediaSession::addedToDocument and
MediaSession::removedFromDocument?

> Source/WebCore/html/HTMLMediaElement.cpp:472
> +    document.unregisterMediaSession(*m_mediaSession);

Ditto.

> Source/WebCore/html/HTMLMediaElement.cpp:4326
> +    document().updateIsPlayingAudio();

The session should update the document. MediaSession::characteristicChanged ?

> Source/WebCore/html/HTMLMediaElement.cpp:6018
> +    document().updateIsPlayingAudio();

Ditto.

> Source/WebCore/html/HTMLMediaElement.h:706
> +    virtual void mediaStateDidChange() override;

This isn't necessary.