Summary: | Implement API in WebCore::Page that returns whether there's any audio playing on the page | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ada Chan <adachan> | ||||
Component: | Media | Assignee: | Ada Chan <adachan> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kangil.han, ossy, philipj, sergio | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 137655 | ||||||
Bug Blocks: | 137048 | ||||||
Attachments: |
|
Description
Ada Chan
2014-09-29 11:19:25 PDT
Steps 3 and 4 have changed. Instead of trying to maintain a map of Page -> MediaSessions, we'll keep a set of MediaSessions in the Document instead. The Page can go through its frames' documents to check whether there are any MediaSessions with audio that are playing. Created attachment 239179 [details]
Patch: Implement WebCore::Page::isPlayingAudio()
Comment on attachment 239179 [details] Patch: Implement WebCore::Page::isPlayingAudio() 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. Thanks for reviewing! (In reply to comment #3) > (From update of attachment 239179 [details]) > 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) Yes, I can do that. > > > 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? I actually had that before, but Jer reminded me that things in the platform layer should not know anything about the dom layer. That's why I did it this way. > > > 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 ? Same reason here. The MediaSession shouldn't update the Document directly, so I did it through the MediaSessionClient. > > > Source/WebCore/html/HTMLMediaElement.cpp:6018 > > + document().updateIsPlayingAudio(); > > Ditto. > > > Source/WebCore/html/HTMLMediaElement.h:706 > > + virtual void mediaStateDidChange() override; > > This isn't necessary. Eric, please let me know if you think there's another approach I can take to maintain the relationship between the Document and the MediaSessions that does not involve the MediaSession having direct knowledge of the Document. Thanks! (In reply to comment #5) > Eric, please let me know if you think there's another approach I can take to maintain the relationship > between the Document and the MediaSessions that does not involve the MediaSession having > direct knowledge of the Document. Thanks! You could put it int HTMLMediaSession, but I fine with you leaving it as-is if you would rather. I've decided to leave it as is for now. I still need to handle MediaSessions from AudioDestinations at some point and that doesn't use HTMLMediaSession. Committed: https://trac.webkit.org/changeset/174353 (In reply to comment #7) > I've decided to leave it as is for now. I still need to handle MediaSessions from AudioDestinations at some point and that doesn't use HTMLMediaSession. > > Committed: > https://trac.webkit.org/changeset/174353 It broke the !ENABLE(VIDEO) build, see bug137655 for details. |